diff mbox series

selinux: use GFP_ATOMIC in convert_context()

Message ID 20221018120111.1474581-1-gongruiqi1@huawei.com (mailing list archive)
State Handled Elsewhere
Headers show
Series selinux: use GFP_ATOMIC in convert_context() | expand

Commit Message

Gong Ruiqi Oct. 18, 2022, 12:01 p.m. UTC
The following BUG_ON was triggered on a hardware environment:

  SELinux: Converting 162 SID table entries...
  BUG: sleeping function called from invalid context at __might_sleep_rtos+0x60/0x74 0x0
  in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 5943, name: tar
  CPU: 7 PID: 5943 Comm: tar Tainted: P O 5.10.0 #1
  Call trace:
   dump_backtrace+0x0/0x1c8
   show_stack+0x18/0x28
   dump_stack+0xe8/0x15c
   ___might_sleep_rtos+0x168/0x17c
   __might_sleep_rtos+0x60/0x74
   __kmalloc_track_caller+0xa0/0x7dc
   kstrdup+0x54/0xac
   convert_context+0x48/0x2e4
   sidtab_context_to_sid+0x1c4/0x36c
   security_context_to_sid_core+0x168/0x238
   security_context_to_sid_default+0x14/0x24
   inode_doinit_use_xattr+0x164/0x1e4
   inode_doinit_with_dentry+0x1c0/0x488
   selinux_d_instantiate+0x20/0x34
   security_d_instantiate+0x70/0xbc
   d_splice_alias+0x4c/0x3c0
   ext4_lookup+0x1d8/0x200 [ext4]
   __lookup_slow+0x12c/0x1e4
   walk_component+0x100/0x200
   path_lookupat+0x88/0x118
   filename_lookup+0x98/0x130
   user_path_at_empty+0x48/0x60
   vfs_statx+0x84/0x140
   vfs_fstatat+0x20/0x30
   __se_sys_newfstatat+0x30/0x74
   __arm64_sys_newfstatat+0x1c/0x2c
   el0_svc_common.constprop.0+0x100/0x184
   do_el0_svc+0x1c/0x2c
   el0_svc+0x20/0x34
   el0_sync_handler+0x80/0x17c
   el0_sync+0x13c/0x140
  SELinux: Context system_u:object_r:pssp_rsyslog_log_t:s0:c0 is not valid (left unmapped).

It was found that convert_context() (hooked by convert->func) might
sleep in a critial section of spin_lock_irqsave in
sidtab_context_to_sid(). Fix this problem by changing the memory
allocation in convert_context() from GFP_KERNEL to GFP_ATOMIC.

Reported-by: Tan Ninghao <tanninghao1@huawei.com>
Fixes: ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve performance")
Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com>
---
 security/selinux/ss/services.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ondrej Mosnacek Oct. 18, 2022, 12:46 p.m. UTC | #1
On Tue, Oct 18, 2022 at 2:01 PM GONG, Ruiqi <gongruiqi1@huawei.com> wrote:
> The following BUG_ON was triggered on a hardware environment:
>
>   SELinux: Converting 162 SID table entries...
>   BUG: sleeping function called from invalid context at __might_sleep_rtos+0x60/0x74 0x0
>   in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 5943, name: tar
>   CPU: 7 PID: 5943 Comm: tar Tainted: P O 5.10.0 #1
>   Call trace:
>    dump_backtrace+0x0/0x1c8
>    show_stack+0x18/0x28
>    dump_stack+0xe8/0x15c
>    ___might_sleep_rtos+0x168/0x17c
>    __might_sleep_rtos+0x60/0x74
>    __kmalloc_track_caller+0xa0/0x7dc
>    kstrdup+0x54/0xac
>    convert_context+0x48/0x2e4
>    sidtab_context_to_sid+0x1c4/0x36c
>    security_context_to_sid_core+0x168/0x238
>    security_context_to_sid_default+0x14/0x24
>    inode_doinit_use_xattr+0x164/0x1e4
>    inode_doinit_with_dentry+0x1c0/0x488
>    selinux_d_instantiate+0x20/0x34
>    security_d_instantiate+0x70/0xbc
>    d_splice_alias+0x4c/0x3c0
>    ext4_lookup+0x1d8/0x200 [ext4]
>    __lookup_slow+0x12c/0x1e4
>    walk_component+0x100/0x200
>    path_lookupat+0x88/0x118
>    filename_lookup+0x98/0x130
>    user_path_at_empty+0x48/0x60
>    vfs_statx+0x84/0x140
>    vfs_fstatat+0x20/0x30
>    __se_sys_newfstatat+0x30/0x74
>    __arm64_sys_newfstatat+0x1c/0x2c
>    el0_svc_common.constprop.0+0x100/0x184
>    do_el0_svc+0x1c/0x2c
>    el0_svc+0x20/0x34
>    el0_sync_handler+0x80/0x17c
>    el0_sync+0x13c/0x140
>   SELinux: Context system_u:object_r:pssp_rsyslog_log_t:s0:c0 is not valid (left unmapped).
>
> It was found that convert_context() (hooked by convert->func) might
> sleep in a critial section of spin_lock_irqsave in
> sidtab_context_to_sid(). Fix this problem by changing the memory
> allocation in convert_context() from GFP_KERNEL to GFP_ATOMIC.

Good catch! However, convert_context() (and
sidtab_convert_params::func) has two callers:
1. sidtab_context_to_sid(), which requires GFP_ATOMIC,
2. sidtab_convert_tree()/sidtab_convert(), where GFP_KERNEL would be okay.

So a more optimal fix would be to add a gfp_t argument to
convert_context()/sidtab_convert_params::func and pass
GFP_KERNEL/_ATOMIC as appropriate in the individual callers.

> Reported-by: Tan Ninghao <tanninghao1@huawei.com>
> Fixes: ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve performance")
> Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com>
> ---
>  security/selinux/ss/services.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index fe5fcf571c56..523876bb7df3 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2036,7 +2036,7 @@ static int convert_context(struct context *oldc, struct context *newc, void *p)
>         args = p;
>
>         if (oldc->str) {
> -               s = kstrdup(oldc->str, GFP_KERNEL);
> +               s = kstrdup(oldc->str, GFP_ATOMIC);
>                 if (!s)
>                         return -ENOMEM;
>
> --
> 2.25.1
>
Paul Moore Oct. 18, 2022, 7:58 p.m. UTC | #2
On Tue, Oct 18, 2022 at 8:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Tue, Oct 18, 2022 at 2:01 PM GONG, Ruiqi <gongruiqi1@huawei.com> wrote:
> > The following BUG_ON was triggered on a hardware environment:
> >
> >   SELinux: Converting 162 SID table entries...
> >   BUG: sleeping function called from invalid context at __might_sleep_rtos+0x60/0x74 0x0
> >   in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 5943, name: tar
> >   CPU: 7 PID: 5943 Comm: tar Tainted: P O 5.10.0 #1
> >   Call trace:
> >    dump_backtrace+0x0/0x1c8
> >    show_stack+0x18/0x28
> >    dump_stack+0xe8/0x15c
> >    ___might_sleep_rtos+0x168/0x17c
> >    __might_sleep_rtos+0x60/0x74
> >    __kmalloc_track_caller+0xa0/0x7dc
> >    kstrdup+0x54/0xac
> >    convert_context+0x48/0x2e4
> >    sidtab_context_to_sid+0x1c4/0x36c
> >    security_context_to_sid_core+0x168/0x238
> >    security_context_to_sid_default+0x14/0x24
> >    inode_doinit_use_xattr+0x164/0x1e4
> >    inode_doinit_with_dentry+0x1c0/0x488
> >    selinux_d_instantiate+0x20/0x34
> >    security_d_instantiate+0x70/0xbc
> >    d_splice_alias+0x4c/0x3c0
> >    ext4_lookup+0x1d8/0x200 [ext4]
> >    __lookup_slow+0x12c/0x1e4
> >    walk_component+0x100/0x200
> >    path_lookupat+0x88/0x118
> >    filename_lookup+0x98/0x130
> >    user_path_at_empty+0x48/0x60
> >    vfs_statx+0x84/0x140
> >    vfs_fstatat+0x20/0x30
> >    __se_sys_newfstatat+0x30/0x74
> >    __arm64_sys_newfstatat+0x1c/0x2c
> >    el0_svc_common.constprop.0+0x100/0x184
> >    do_el0_svc+0x1c/0x2c
> >    el0_svc+0x20/0x34
> >    el0_sync_handler+0x80/0x17c
> >    el0_sync+0x13c/0x140
> >   SELinux: Context system_u:object_r:pssp_rsyslog_log_t:s0:c0 is not valid (left unmapped).
> >
> > It was found that convert_context() (hooked by convert->func) might
> > sleep in a critial section of spin_lock_irqsave in
> > sidtab_context_to_sid(). Fix this problem by changing the memory
> > allocation in convert_context() from GFP_KERNEL to GFP_ATOMIC.
>
> Good catch! However, convert_context() (and
> sidtab_convert_params::func) has two callers:
> 1. sidtab_context_to_sid(), which requires GFP_ATOMIC,
> 2. sidtab_convert_tree()/sidtab_convert(), where GFP_KERNEL would be okay.
>
> So a more optimal fix would be to add a gfp_t argument to
> convert_context()/sidtab_convert_params::func and pass
> GFP_KERNEL/_ATOMIC as appropriate in the individual callers.

Agreed.  Please make the changes Ondrej is suggesting.
diff mbox series

Patch

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index fe5fcf571c56..523876bb7df3 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2036,7 +2036,7 @@  static int convert_context(struct context *oldc, struct context *newc, void *p)
 	args = p;
 
 	if (oldc->str) {
-		s = kstrdup(oldc->str, GFP_KERNEL);
+		s = kstrdup(oldc->str, GFP_ATOMIC);
 		if (!s)
 			return -ENOMEM;