NACK[J]: [SRU][J][K][Unstable][PATCH 1/1] UBUNTU: SAUCE: LSM: Change Landlock from LSMBLOB_NEEDED to LSMBLOB_NOT_NEEDED

Fri Sep 9 07:38:48 UTC 2022

Hi Stefan, Andrea,

Will you re-review the patch, or is there no chance for my appeal to
be accepted?

Would you like me to re-submit by incrementing LSMBLOB_ENTRIES
instead, something like below:

diff --git a/include/linux/security.h b/include/linux/security.h
index 5c1c4933d4b2..602c543fcb14 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -175,6 +175,7 @@ static inline void lsmcontext_init(struct
lsmcontext *cp, char *context,
        (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
        (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
        (IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) + \
+       (IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0) + \
        (IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0))

 struct lsmblob {

Thanks,
Matthew

On Fri, Sep 2, 2022 at 7:38 PM Matthew Ruffell
<matthew.ruffell at canonical.com> wrote:
>
> Hi Stefan, Andrea,
>
> If you look at security/landlock/setup.c at current mainline 6.0-rc3,
> we see the same code:
>
>  20 struct lsm_blob_sizes landlock_blob_sizes __lsm_ro_after_init = {
>  21     .lbs_cred = sizeof(struct landlock_cred_security),
>  22     .lbs_inode = sizeof(struct landlock_inode_security),
>  23     .lbs_superblock = sizeof(struct landlock_superblock_security),
>  24 };
>
> If you look at Casey's V37 of the patchset from 27 June 2022:
>
> https://lore.kernel.org/selinux/20220628005611.13106-4-casey@schaufler-ca.com/
>
> diff --git a/security/landlock/setup.c b/security/landlock/setup.c
> index f8e8e980454c..759e00b9436c 100644
> --- a/security/landlock/setup.c
> +++ b/security/landlock/setup.c
> @@ -23,6 +23,11 @@ struct lsm_blob_sizes landlock_blob_sizes
> __lsm_ro_after_init = {
>   .lbs_superblock = sizeof(struct landlock_superblock_security),
>  };
>
> +struct lsm_id landlock_lsmid __lsm_ro_after_init = {
> + .lsm = LANDLOCK_NAME,
> + .slot = LSMBLOB_NOT_NEEDED,
> +};
> +
>  static int __init landlock_init(void)
>  {
>   landlock_add_cred_hooks();
>
> It is still marked as LSMBLOB_NOT_NEEDED. The code hunk in question,
> .lbs_superblock, is even visible.
>
> Now, looking at the code in landlock_blob_sizes, it uses struct
> landlock_cred_security, struct landlock_inode_security and struct
> landlock_superblock_security, with no mention of struct lsmblob.
>
> Reading Casey's response in:
>
> >>> diff --git a/security/landlock/setup.c b/security/landlock/setup.c
> >>> index f8e8e980454c..4a12666a4090 100644
> >>> --- a/security/landlock/setup.c
> >>> +++ b/security/landlock/setup.c
> >>> @@ -23,6 +23,10 @@ struct lsm_blob_sizes landlock_blob_sizes __lsm_ro_after_init = {
> >>>   .lbs_superblock = sizeof(struct landlock_superblock_security),
> >>>  };
> >>>
> >>> +struct lsm_id landlock_lsmid __lsm_ro_after_init = {
> >>> + .lsm = LANDLOCK_NAME,
> >> It is missing: .slot = LSMBLOB_NEEDED,
> >
> > Landlock does not provide any of the hooks that use a struct lsmblob.
> > That would be secid_to_secctx, secctx_to_secid, inode_getsecid,
> > cred_getsecid, kernel_act_as task_getsecid_subj task_getsecid_obj and
> > ipc_getsecid. Setting .slot = LSMBLOB_NEEDED indicates that the LSM
> > uses a slot in struct lsmblob. Landlock does not need a slot.
>
> I still think that it should be safe to set Landlock to LSMBLOB_NOT_NEEDED.
>
> I still think we should use the patch as-is for Kinetic and Unstable and onward.
>
> I am open to incrementing LSMBLOB_ENTRIES for Jammy to keep regression
> risk down,
> but it should be unneccessary, and it deviates from upstream.
>
> Do we have any plans to pick up the most recent V37 patchset, so we can follow
> closer to upstream? Or are we going to keep the old patchset from 2020 until
> this eventually gets mainlined?
>
> Please re-review, and if you think we should just increment LSMBLOB_ENTRIES
> instead, I will send a new patch.
>
> Thanks,
> Matthew
>
> On Fri, Sep 2, 2022 at 7:32 PM Andrea Righi <andrea.righi at canonical.com> wrote:
> >
> > On Fri, Sep 02, 2022 at 07:54:44AM +0200, Stefan Bader wrote:
> > > On 29.08.22 07:54, Matthew Ruffell wrote:
> > > > BugLink: https://bugs.launchpad.net/bugs/1987998
> > > >
> > > > The Landlock LSM does not register any hooks which use struct lsmblob, and does
> > > > not require a slot in the secid array of struct lsmblob.
> > > >
> > > > Change LSMBLOB_NEEDED to LSMBLOB_NOT_NEEDED.
> > > >
> > > > This is required to fix a panic on boot where too many LSMs can be configured,
> > > > since while we currently mark Landlock as LSMBLOB_NEEDED, we do not actually
> > > > make LSMBLOB_ENTRIES large enough to fit it, and we panic when more than 2
> > > > LSMs are configured, like:
> > > >
> > > > GRUB_CMDLINE_LINUX_DEFAULT="lsm=landlock,bpf,apparmor"
> > > >
> > > > LSM: Security Framework initializing
> > > > landlock: Up and running.
> > > > LSM support for eBPF active
> > > > Kernel panic - not syncing: security_add_hooks Too many LSMs registered.
> > > > CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.15.0-46-generic #49-Ubuntu
> > > > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
> > > > Call Trace:
> > > >   <TASK>
> > > >   show_stack+0x52/0x5c
> > > >   dump_stack_lvl+0x4a/0x63
> > > >   dump_stack+0x10/0x16
> > > >   panic+0x149/0x321
> > > >   security_add_hooks+0x45/0x13a
> > > >   apparmor_init+0x189/0x1ef
> > > >   initialize_lsm+0x54/0x74
> > > >   ordered_lsm_init+0x379/0x392
> > > >   security_init+0x40/0x49
> > > >   start_kernel+0x466/0x4dc
> > > >   x86_64_start_reservations+0x24/0x2a
> > > >   x86_64_start_kernel+0xe4/0xef
> > > >   secondary_startup_64_no_verify+0xc2/0xcb
> > > >   </TASK>
> > > > ---[ end Kernel panic - not syncing: security_add_hooks Too many LSMs registered. ]---
> > > >
> > > > Also refactor the Landlock support by going to just one struct lsm_id, and
> > > > extern it from setup.h, following upstream development.
> > > >
> > > > Fixes: f17b27a2790e ("UBUNTU: SAUCE: LSM: Create and manage the lsmblob data structure.") ubuntu-jammy
> > > > Signed-off-by: Matthew Ruffell <matthew.ruffell at canonical.com>
> > > > ---
> > >
> > > Forwarding feedback from security:
> > >
> > > So unfortunately landlock does use LSMBLOBS in 5.15 it is using cred, inode
> > > and superblock blobs
> > >
> > > see security/landlock/setup.c:
> > >
> > > struct lsm_blob_sizes landlock_blob_sizes __lsm_ro_after_init = {
> > >     .lbs_cred = sizeof(struct landlock_cred_security),
> > >     .lbs_inode = sizeof(struct landlock_inode_security),
> > >     .lbs_superblock = sizeof(struct landlock_superblock_security),
> > > };
> > >
> > > so NAK this will break things.
> > >
> > > We need to increase LSMBLOB_ENTRIES
> > >
> > > -Stefan
> >
> > Thanks for checking this Stefan. I also dropped this patch from
> > kinetic:linux and kinetic:linux-unstable.
> >
> > -Andrea

