| Message ID | 20221019025710.2482945-1-gongruiqi1@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] selinux: use GFP_ATOMIC in convert_context() | expand |
On Wed, Oct 19, 2022 at 4:56 AM GONG, Ruiqi <gongruiqi1@huawei.com> wrote: > > The following warning was triggered on a hardware environment: > > SELinux: Converting 162 SID table entries... > BUG: sleeping function called from invalid context at __might_sleep+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+0x168/0x17c > __might_sleep+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 within a critical section of spin_lock_irqsave in > sidtab_context_to_sid(), convert_context() (hooked by > sidtab_convert_params.func) might cause the process to sleep via > allocating memory with GFP_KERNEL, which is problematic. > > As Ondrej pointed out [1], convert_context()/sidtab_convert_params.func > has another caller sidtab_convert_tree(), which is okay with GFP_KERNEL. > Therefore, fix this problem by adding a gfp_t argument for > convert_context()/sidtab_convert_params.func and pass GFP_KERNEL/_ATOMIC > properly in individual callers. > > Link: https://lore.kernel.org/all/20221018120111.1474581-1-gongruiqi1@huawei.com/ [1] > 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> > --- > > v2: change as Ondrej suggests & redraft commit message This looks good, thanks! Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com> > [...]
On Tue, Oct 18, 2022 at 10:56 PM GONG, Ruiqi <gongruiqi1@huawei.com> wrote: > > The following warning was triggered on a hardware environment: > > SELinux: Converting 162 SID table entries... > BUG: sleeping function called from invalid context at __might_sleep+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+0x168/0x17c > __might_sleep+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 within a critical section of spin_lock_irqsave in > sidtab_context_to_sid(), convert_context() (hooked by > sidtab_convert_params.func) might cause the process to sleep via > allocating memory with GFP_KERNEL, which is problematic. > > As Ondrej pointed out [1], convert_context()/sidtab_convert_params.func > has another caller sidtab_convert_tree(), which is okay with GFP_KERNEL. > Therefore, fix this problem by adding a gfp_t argument for > convert_context()/sidtab_convert_params.func and pass GFP_KERNEL/_ATOMIC > properly in individual callers. > > Link: https://lore.kernel.org/all/20221018120111.1474581-1-gongruiqi1@huawei.com/ [1] > 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> > --- > > v2: change as Ondrej suggests & redraft commit message > > security/selinux/ss/services.c | 5 +++-- > security/selinux/ss/sidtab.c | 4 ++-- > security/selinux/ss/sidtab.h | 2 +- > 3 files changed, 6 insertions(+), 5 deletions(-) Merged into selinux/stable-6.1, thank you. Normally I would send this to Linus in a day or two, but due to some personal logistical challenges I may be a bit delayed in sending this up. I would ask for your patience and that everyone take this opportunity to do some additional testing :) Thanks everyone.
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index fe5fcf571c56..64a6a37dc36d 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2022,7 +2022,8 @@ static inline int convert_context_handle_invalid_context( * in `newc'. Verify that the context is valid * under the new policy. */ -static int convert_context(struct context *oldc, struct context *newc, void *p) +static int convert_context(struct context *oldc, struct context *newc, void *p, + gfp_t gfp_flags) { struct convert_context_args *args; struct ocontext *oc; @@ -2036,7 +2037,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_flags); if (!s) return -ENOMEM; diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c index a54b8652bfb5..db5cce385bf8 100644 --- a/security/selinux/ss/sidtab.c +++ b/security/selinux/ss/sidtab.c @@ -325,7 +325,7 @@ int sidtab_context_to_sid(struct sidtab *s, struct context *context, } rc = convert->func(context, &dst_convert->context, - convert->args); + convert->args, GFP_ATOMIC); if (rc) { context_destroy(&dst->context); goto out_unlock; @@ -404,7 +404,7 @@ static int sidtab_convert_tree(union sidtab_entry_inner *edst, while (i < SIDTAB_LEAF_ENTRIES && *pos < count) { rc = convert->func(&esrc->ptr_leaf->entries[i].context, &edst->ptr_leaf->entries[i].context, - convert->args); + convert->args, GFP_KERNEL); if (rc) return rc; (*pos)++; diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h index 4eff0e49dcb2..9fce0d553fe2 100644 --- a/security/selinux/ss/sidtab.h +++ b/security/selinux/ss/sidtab.h @@ -65,7 +65,7 @@ struct sidtab_isid_entry { }; struct sidtab_convert_params { - int (*func)(struct context *oldc, struct context *newc, void *args); + int (*func)(struct context *oldc, struct context *newc, void *args, gfp_t gfp_flags); void *args; struct sidtab *target; };
The following warning was triggered on a hardware environment: SELinux: Converting 162 SID table entries... BUG: sleeping function called from invalid context at __might_sleep+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+0x168/0x17c __might_sleep+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 within a critical section of spin_lock_irqsave in sidtab_context_to_sid(), convert_context() (hooked by sidtab_convert_params.func) might cause the process to sleep via allocating memory with GFP_KERNEL, which is problematic. As Ondrej pointed out [1], convert_context()/sidtab_convert_params.func has another caller sidtab_convert_tree(), which is okay with GFP_KERNEL. Therefore, fix this problem by adding a gfp_t argument for convert_context()/sidtab_convert_params.func and pass GFP_KERNEL/_ATOMIC properly in individual callers. Link: https://lore.kernel.org/all/20221018120111.1474581-1-gongruiqi1@huawei.com/ [1] 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> --- v2: change as Ondrej suggests & redraft commit message security/selinux/ss/services.c | 5 +++-- security/selinux/ss/sidtab.c | 4 ++-- security/selinux/ss/sidtab.h | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-)