From 395a02d04ed4d9b9ae2d7cadfd9b14a64fe240b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Noack?= <gnoack@google.com> Date: Mon, 10 Jun 2024 08:21:15 +0000 Subject: [PATCH] landlock: Use bit-fields for storing handled layer access masks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When defined using bit-fields, the compiler takes care of packing the bits in a memory-efficient way and frees us from defining LANDLOCK_SHIFT_ACCESS_* by hand. The exact memory layout does not matter in our use case. The manual definition of LANDLOCK_SHIFT_ACCESS_* has resulted in bugs in at least two recent patch sets [1] [2] where new kinds of handled access rights were introduced. Cc: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> Cc: Tahera Fahimi <fahimitahera@gmail.com> Link: https://lore.kernel.org/r/ebd680cc-25d6-ee14-4856-310f5e5e28e4@huawei-partners.com [1] Link: https://lore.kernel.org/r/ZmLEoBfHyUR3nKAV@google.com [2] Signed-off-by: Günther Noack <gnoack@google.com> Link: https://lore.kernel.org/r/20240610082115.1693267-1-gnoack@google.com Signed-off-by: Mickaël Salaün <mic@digikod.net> --- security/landlock/limits.h | 2 -- security/landlock/ruleset.c | 4 ---- security/landlock/ruleset.h | 24 +++++++++--------------- 3 files changed, 9 insertions(+), 21 deletions(-) diff --git a/security/landlock/limits.h b/security/landlock/limits.h index 20fdb5ff35148..4eb643077a2a6 100644 --- a/security/landlock/limits.h +++ b/security/landlock/limits.h @@ -21,12 +21,10 @@ #define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_IOCTL_DEV #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1) #define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS) -#define LANDLOCK_SHIFT_ACCESS_FS 0 #define LANDLOCK_LAST_ACCESS_NET LANDLOCK_ACCESS_NET_CONNECT_TCP #define LANDLOCK_MASK_ACCESS_NET ((LANDLOCK_LAST_ACCESS_NET << 1) - 1) #define LANDLOCK_NUM_ACCESS_NET __const_hweight64(LANDLOCK_MASK_ACCESS_NET) -#define LANDLOCK_SHIFT_ACCESS_NET LANDLOCK_NUM_ACCESS_FS /* clang-format on */ diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c index e0a5fbf9201ad..6ff232f586183 100644 --- a/security/landlock/ruleset.c +++ b/security/landlock/ruleset.c @@ -169,13 +169,9 @@ static void build_check_ruleset(void) .num_rules = ~0, .num_layers = ~0, }; - typeof(ruleset.access_masks[0]) access_masks = ~0; BUILD_BUG_ON(ruleset.num_rules < LANDLOCK_MAX_NUM_RULES); BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS); - BUILD_BUG_ON(access_masks < - ((LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) | - (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET))); } /** diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h index c7f1526784fd1..0f1b5b4c8f6b4 100644 --- a/security/landlock/ruleset.h +++ b/security/landlock/ruleset.h @@ -39,10 +39,10 @@ static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET); static_assert(sizeof(unsigned long) >= sizeof(access_mask_t)); /* Ruleset access masks. */ -typedef u32 access_masks_t; -/* Makes sure all ruleset access rights can be stored. */ -static_assert(BITS_PER_TYPE(access_masks_t) >= - LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET); +struct access_masks { + access_mask_t fs : LANDLOCK_NUM_ACCESS_FS; + access_mask_t net : LANDLOCK_NUM_ACCESS_NET; +}; typedef u16 layer_mask_t; /* Makes sure all layers can be checked. */ @@ -226,7 +226,7 @@ struct landlock_ruleset { * layers are set once and never changed for the * lifetime of the ruleset. */ - access_masks_t access_masks[]; + struct access_masks access_masks[]; }; }; }; @@ -265,8 +265,7 @@ landlock_add_fs_access_mask(struct landlock_ruleset *const ruleset, /* Should already be checked in sys_landlock_create_ruleset(). */ WARN_ON_ONCE(fs_access_mask != fs_mask); - ruleset->access_masks[layer_level] |= - (fs_mask << LANDLOCK_SHIFT_ACCESS_FS); + ruleset->access_masks[layer_level].fs |= fs_mask; } static inline void @@ -278,17 +277,14 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset, /* Should already be checked in sys_landlock_create_ruleset(). */ WARN_ON_ONCE(net_access_mask != net_mask); - ruleset->access_masks[layer_level] |= - (net_mask << LANDLOCK_SHIFT_ACCESS_NET); + ruleset->access_masks[layer_level].net |= net_mask; } static inline access_mask_t landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset, const u16 layer_level) { - return (ruleset->access_masks[layer_level] >> - LANDLOCK_SHIFT_ACCESS_FS) & - LANDLOCK_MASK_ACCESS_FS; + return ruleset->access_masks[layer_level].fs; } static inline access_mask_t @@ -304,9 +300,7 @@ static inline access_mask_t landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset, const u16 layer_level) { - return (ruleset->access_masks[layer_level] >> - LANDLOCK_SHIFT_ACCESS_NET) & - LANDLOCK_MASK_ACCESS_NET; + return ruleset->access_masks[layer_level].net; } bool landlock_unmask_layers(const struct landlock_rule *const rule, -- GitLab