diff mbox series

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

Message ID 20220927093159.20056-2-matthew.ruffell@canonical.com
State New
Headers show
Series LSM: Configuring Too Many LSMs Causes Kernel Panic on Boot | expand

Commit Message

Matthew Ruffell Sept. 27, 2022, 9:31 a.m. UTC 
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@canonical.com>
---
 security/landlock/cred.c   | 5 -----
 security/landlock/fs.c     | 5 -----
 security/landlock/ptrace.c | 5 -----
 security/landlock/setup.c  | 5 +++++
 security/landlock/setup.h  | 1 +
 5 files changed, 6 insertions(+), 15 deletions(-)

Comments

Stefan Bader Sept. 27, 2022, 9:48 a.m. UTC | #1 
On 27.09.22 11:31, 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@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>   security/landlock/cred.c   | 5 -----
>   security/landlock/fs.c     | 5 -----
>   security/landlock/ptrace.c | 5 -----
>   security/landlock/setup.c  | 5 +++++
>   security/landlock/setup.h  | 1 +
>   5 files changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/security/landlock/cred.c b/security/landlock/cred.c
> index e3bd04cc7177..2eb1d65f10d6 100644
> --- a/security/landlock/cred.c
> +++ b/security/landlock/cred.c
> @@ -14,11 +14,6 @@
>   #include "ruleset.h"
>   #include "setup.h"
>   
> -static struct lsm_id landlock_lsmid __lsm_ro_after_init = {
> -	.lsm  = "landlock",
> -	.slot = LSMBLOB_NEEDED
> -};
> -
>   static int hook_cred_prepare(struct cred *const new,
>   			     const struct cred *const old, const gfp_t gfp)
>   {
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index b81db9d184bd..d8842a2ac58a 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -37,11 +37,6 @@
>   #include "ruleset.h"
>   #include "setup.h"
>   
> -static struct lsm_id landlock_lsmid __lsm_ro_after_init = {
> -	.lsm  = "landlock",
> -	.slot = LSMBLOB_NEEDED
> -};
> -
>   /* Underlying object management */
>   
>   static void release_inode(struct landlock_object *const object)
> diff --git a/security/landlock/ptrace.c b/security/landlock/ptrace.c
> index 0f3bb8ea12db..eab35808f395 100644
> --- a/security/landlock/ptrace.c
> +++ b/security/landlock/ptrace.c
> @@ -20,11 +20,6 @@
>   #include "ruleset.h"
>   #include "setup.h"
>   
> -static struct lsm_id landlock_lsmid __lsm_ro_after_init = {
> -       .lsm  = "landlock",
> -       .slot = LSMBLOB_NEEDED
> -};
> -
>   /**
>    * domain_scope_le - Checks domain ordering for scoped ptrace
>    *
> 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();
> diff --git a/security/landlock/setup.h b/security/landlock/setup.h
> index 1daffab1ab4b..38bce5b172dc 100644
> --- a/security/landlock/setup.h
> +++ b/security/landlock/setup.h
> @@ -14,5 +14,6 @@
>   extern bool landlock_initialized;
>   
>   extern struct lsm_blob_sizes landlock_blob_sizes;
> +extern struct lsm_id landlock_lsmid;
>   
>   #endif /* _SECURITY_LANDLOCK_SETUP_H */
Andrea Righi Sept. 27, 2022, 9:51 a.m. UTC | #2 
On Tue, Sep 27, 2022 at 10:31:59PM +1300, 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@canonical.com>

Looks good to me, but it seems to conflict after applying the new
apparmor pull request for kinetic (maybe these changes are already
integrated in the PR). I'll double check for kinetic and unstable, in
the meantime it makes sense to me to have this in jammy, therefore:

Acked-by: Andrea Righi <andrea.righi@canonical.com>
Andrea Righi Sept. 27, 2022, 10:09 a.m. UTC | #3 
On Tue, Sep 27, 2022 at 11:51:15AM +0200, Andrea Righi wrote:
> On Tue, Sep 27, 2022 at 10:31:59PM +1300, 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@canonical.com>
> 
> Looks good to me, but it seems to conflict after applying the new
> apparmor pull request for kinetic (maybe these changes are already
> integrated in the PR). I'll double check for kinetic and unstable, in
> the meantime it makes sense to me to have this in jammy, therefore:
> 
> Acked-by: Andrea Righi <andrea.righi@canonical.com>

This is not needed anymore with the new apparmor (6.1) and lsm stacking
(v37) patch set, so NACK-ing for kinetic:linux and
kinetic:linux-unstable.

-Andrea
diff mbox series

Patch

diff --git a/security/landlock/cred.c b/security/landlock/cred.c
index e3bd04cc7177..2eb1d65f10d6 100644
--- a/security/landlock/cred.c
+++ b/security/landlock/cred.c
@@ -14,11 +14,6 @@ 
 #include "ruleset.h"
 #include "setup.h"
 
-static struct lsm_id landlock_lsmid __lsm_ro_after_init = {
-	.lsm  = "landlock",
-	.slot = LSMBLOB_NEEDED
-};
-
 static int hook_cred_prepare(struct cred *const new,
 			     const struct cred *const old, const gfp_t gfp)
 {
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index b81db9d184bd..d8842a2ac58a 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -37,11 +37,6 @@ 
 #include "ruleset.h"
 #include "setup.h"
 
-static struct lsm_id landlock_lsmid __lsm_ro_after_init = {
-	.lsm  = "landlock",
-	.slot = LSMBLOB_NEEDED
-};
-
 /* Underlying object management */
 
 static void release_inode(struct landlock_object *const object)
diff --git a/security/landlock/ptrace.c b/security/landlock/ptrace.c
index 0f3bb8ea12db..eab35808f395 100644
--- a/security/landlock/ptrace.c
+++ b/security/landlock/ptrace.c
@@ -20,11 +20,6 @@ 
 #include "ruleset.h"
 #include "setup.h"
 
-static struct lsm_id landlock_lsmid __lsm_ro_after_init = {
-       .lsm  = "landlock",
-       .slot = LSMBLOB_NEEDED
-};
-
 /**
  * domain_scope_le - Checks domain ordering for scoped ptrace
  *
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();
diff --git a/security/landlock/setup.h b/security/landlock/setup.h
index 1daffab1ab4b..38bce5b172dc 100644
--- a/security/landlock/setup.h
+++ b/security/landlock/setup.h
@@ -14,5 +14,6 @@ 
 extern bool landlock_initialized;
 
 extern struct lsm_blob_sizes landlock_blob_sizes;
+extern struct lsm_id landlock_lsmid;
 
 #endif /* _SECURITY_LANDLOCK_SETUP_H */