* [PATCH v2] selinux: use GFP_ATOMIC in convert_context()
@ 2022-10-19 2:57 GONG, Ruiqi
2022-10-19 8:42 ` Ondrej Mosnacek
2022-10-19 14:07 ` Paul Moore
0 siblings, 2 replies; 3+ messages in thread
From: GONG, Ruiqi @ 2022-10-19 2:57 UTC (permalink / raw)
To: Paul Moore, Stephen Smalley, Eric Paris
Cc: Ondrej Mosnacek, selinux, linux-security-module, linux-kernel,
Xiu Jianfeng, gongruiqi1
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(-)
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;
};
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] selinux: use GFP_ATOMIC in convert_context()
2022-10-19 2:57 [PATCH v2] selinux: use GFP_ATOMIC in convert_context() GONG, Ruiqi
@ 2022-10-19 8:42 ` Ondrej Mosnacek
2022-10-19 14:07 ` Paul Moore
1 sibling, 0 replies; 3+ messages in thread
From: Ondrej Mosnacek @ 2022-10-19 8:42 UTC (permalink / raw)
To: GONG, Ruiqi
Cc: Paul Moore, Stephen Smalley, Eric Paris, selinux,
linux-security-module, linux-kernel, Xiu Jianfeng
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>
> [...]
--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] selinux: use GFP_ATOMIC in convert_context()
2022-10-19 2:57 [PATCH v2] selinux: use GFP_ATOMIC in convert_context() GONG, Ruiqi
2022-10-19 8:42 ` Ondrej Mosnacek
@ 2022-10-19 14:07 ` Paul Moore
1 sibling, 0 replies; 3+ messages in thread
From: Paul Moore @ 2022-10-19 14:07 UTC (permalink / raw)
To: GONG, Ruiqi
Cc: Stephen Smalley, Eric Paris, Ondrej Mosnacek, selinux,
linux-security-module, linux-kernel, Xiu Jianfeng
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.
--
paul-moore.com
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-10-19 14:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-19 2:57 [PATCH v2] selinux: use GFP_ATOMIC in convert_context() GONG, Ruiqi
2022-10-19 8:42 ` Ondrej Mosnacek
2022-10-19 14:07 ` Paul Moore
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).