| Message ID | 20240610082115.1693267-1-gnoack@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | landlock: Use bit-fields for storing handled layer access masks | expand |
Great! Looking at the generated data structures with pahole, it doesn't increase the whole size, and it should be fine with other (small) fields too. With this new struct, we don't need the landlock_get_* helpers anymore. We might want to keep the landlock_add_*() helpers as safeguards (because of the WARN_ON_ONCE) though. On Mon, Jun 10, 2024 at 08:21:15AM +0000, Günther Noack wrote: > 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 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/all/ebd680cc-25d6-ee14-4856-310f5e5e28e4@huawei-partners.com/ > Link: https://lore.kernel.org/all/ZmLEoBfHyUR3nKAV@google.com/ Please add [1] and [2] at the end of each link to reference them in the commit message. > Signed-off-by: Günther Noack <gnoack@google.com> > --- > 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 20fdb5ff3514..4eb643077a2a 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 e0a5fbf9201a..6ff232f58618 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 c7f1526784fd..0f1b5b4c8f6b 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, > -- > 2.45.2.505.gda0bf45e8d-goog > >
On Thu, Jun 13, 2024 at 11:20:38PM +0200, Mickaël Salaün wrote: > Great! Looking at the generated data structures with pahole, it doesn't > increase the whole size, and it should be fine with other (small) fields > too. > > With this new struct, we don't need the landlock_get_* helpers anymore. > We might want to keep the landlock_add_*() helpers as safeguards > (because of the WARN_ON_ONCE) though. I am unsure about removing these helper functions, due to the following reasons: * landlock_get_fs_access_mask is the place where we transparently add the "refer" access right. If we remove landlock_get_net_access_mask, it would be assymetric with keeping the same function for the file system restrictions. * landlock_init_layer_masks() is using landlock_get_fs_access_mask and landlock_get_net_access_mask through a function pointer. When these functions are gone, we would have to redefine them locally anyway. Options to refactor this function include: * split it in two separate functions landlock_init_fs_layer_masks and landlock_init_net_layer_masks. It would end up duplicating some of the bit manipulation code. * add another #if further down in the function Both variants seem not nice. Do you think this is worth doing? —Günther
On Fri, Jun 14, 2024 at 02:06:54PM +0200, Günther Noack wrote: > On Thu, Jun 13, 2024 at 11:20:38PM +0200, Mickaël Salaün wrote: > > Great! Looking at the generated data structures with pahole, it doesn't > > increase the whole size, and it should be fine with other (small) fields > > too. > > > > With this new struct, we don't need the landlock_get_* helpers anymore. > > We might want to keep the landlock_add_*() helpers as safeguards > > (because of the WARN_ON_ONCE) though. > > I am unsure about removing these helper functions, due to the following reasons: > > * landlock_get_fs_access_mask is the place where we transparently add the > "refer" access right. If we remove landlock_get_net_access_mask, it would be > assymetric with keeping the same function for the file system restrictions. > > * landlock_init_layer_masks() is using landlock_get_fs_access_mask and > landlock_get_net_access_mask through a function pointer. When these > functions are gone, we would have to redefine them locally anyway. > > Options to refactor this function include: > * split it in two separate functions landlock_init_fs_layer_masks and > landlock_init_net_layer_masks. It would end up duplicating some of the > bit manipulation code. > * add another #if further down in the function > > Both variants seem not nice. > > Do you think this is worth doing? No, I agree with you. It's applied to my next branch. Thanks! Mikhail, Tahera, please base your next patch series on this branch: https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/
diff --git a/security/landlock/limits.h b/security/landlock/limits.h index 20fdb5ff3514..4eb643077a2a 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 e0a5fbf9201a..6ff232f58618 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 c7f1526784fd..0f1b5b4c8f6b 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,
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 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/all/ebd680cc-25d6-ee14-4856-310f5e5e28e4@huawei-partners.com/ Link: https://lore.kernel.org/all/ZmLEoBfHyUR3nKAV@google.com/ Signed-off-by: Günther Noack <gnoack@google.com> --- security/landlock/limits.h | 2 -- security/landlock/ruleset.c | 4 ---- security/landlock/ruleset.h | 24 +++++++++--------------- 3 files changed, 9 insertions(+), 21 deletions(-)