| Message ID | 20240417074444.2834769-1-guozihua@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | ima: Avoid blocking in RCU read-side critical section | expand |
On Wed, Apr 17, 2024 at 12:49 AM GUO Zihua <guozihua@huawei.com> wrote: > > A panic happens in ima_match_policy: > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 > PGD 42f873067 P4D 0 > Oops: 0000 [#1] SMP NOPTI > CPU: 5 PID: 1286325 Comm: kubeletmonit.sh Kdump: loaded Tainted: P > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015 > RIP: 0010:ima_match_policy+0x84/0x450 > Code: 49 89 fc 41 89 cf 31 ed 89 44 24 14 eb 1c 44 39 7b 18 74 26 41 83 ff 05 74 20 48 8b 1b 48 3b 1d f2 b9 f4 00 0f 84 9c 01 00 00 <44> 85 73 10 74 ea 44 8b 6b 14 41 f6 c5 01 75 d4 41 f6 c5 02 74 0f > RSP: 0018:ff71570009e07a80 EFLAGS: 00010207 > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000200 > RDX: ffffffffad8dc7c0 RSI: 0000000024924925 RDI: ff3e27850dea2000 > RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffffabfce739 > R10: ff3e27810cc42400 R11: 0000000000000000 R12: ff3e2781825ef970 > R13: 00000000ff3e2785 R14: 000000000000000c R15: 0000000000000001 > FS: 00007f5195b51740(0000) GS:ff3e278b12d40000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000000000000010 CR3: 0000000626d24002 CR4: 0000000000361ee0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > ima_get_action+0x22/0x30 > process_measurement+0xb0/0x830 > ima_file_check+0x64/0x90 > path_openat+0x571/0x1720 > do_filp_open+0x9b/0x110 > do_sys_open+0x1bd/0x250 > do_syscall_64+0x5d/0x1d0 > entry_SYSCALL_64_after_hwframe+0x65/0xca > > (stack trace marked with ? is deleted) > > Commit c7423dbdbc9e ("ima: Handle -ESTALE returned by > ima_filter_rule_match()") introduced call to ima_lsm_copy_rule within a > RCU read-side critical section which contains kmalloc with GFP_KERNEL. > This implies a possible sleep and violates limitations of RCU read-side > critical sections on non-PREEMPT systems. > > Sleeping within RCU read-side critical section might cause > synchronize_rcu() returning early and break RCU protection, allowing a > UAF to happen. > > The root cause of this issue could be described as follows: > | Thread A | Thread B | > | |ima_match_policy | > | | rcu_read_lock | > |ima_lsm_update_rule | | > | synchronize_rcu | | > | | kmalloc(GFP_KERNEL)| > | | sleep | > ==> synchronize_rcu returns early > | kfree(entry) | | > | | entry = entry->next| > ==> UAF happens and entry now becomes NULL (or could be anything). > | | entry->action | > ==> Accessing entry might cause panic. > > To fix this issue, we are converting all kmalloc that is called within > RCU read-side critical section to use GFP_ATOMIC. > > Fixes: c7423dbdbc9e ("ima: Handle -ESTALE returned by ima_filter_rule_match()") > Cc: stable@vger.kernel.org > Signed-off-by: GUO Zihua <guozihua@huawei.com> > --- > security/integrity/ima/ima_policy.c | 2 +- > security/selinux/ss/services.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index c0556907c2e6..c0cf9b6a01f0 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -410,7 +410,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) > * Immutable elements are copied over as pointers and data; only > * lsm rules can change > */ > - nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL); > + nentry = kmemdup(entry, sizeof(*nentry), GFP_ATOMIC); > if (!nentry) > return NULL; > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index e88b1b6c4adb..b7cfad1a2964 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -3549,7 +3549,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) > return -EINVAL; > } > > - tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_KERNEL); > + tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_ATOMIC); I would suggest passing in gfp flags from the callers and only using GFP_ATOMIC for the particular call chain that requires atomic allocations, or re-factoring the caller to perform the allocating operations outside of the critical section. Sidebar: the refactoring of the SELinux policy loading logic may have made it possible to revisit the approaches here to permit holding a reference to the policy from which the rule was derived so that we don't have to return -ESTALE in this scenario. > if (!tmprule) > return -ENOMEM; > context_init(&tmprule->au_ctxt); > -- >
On 2024/4/20 5:41, Stephen Smalley wrote: > On Wed, Apr 17, 2024 at 12:49 AM GUO Zihua <guozihua@huawei.com> wrote: >> >> A panic happens in ima_match_policy: >> >> BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 >> PGD 42f873067 P4D 0 >> Oops: 0000 [#1] SMP NOPTI >> CPU: 5 PID: 1286325 Comm: kubeletmonit.sh Kdump: loaded Tainted: P >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015 >> RIP: 0010:ima_match_policy+0x84/0x450 >> Code: 49 89 fc 41 89 cf 31 ed 89 44 24 14 eb 1c 44 39 7b 18 74 26 41 83 ff 05 74 20 48 8b 1b 48 3b 1d f2 b9 f4 00 0f 84 9c 01 00 00 <44> 85 73 10 74 ea 44 8b 6b 14 41 f6 c5 01 75 d4 41 f6 c5 02 74 0f >> RSP: 0018:ff71570009e07a80 EFLAGS: 00010207 >> RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000200 >> RDX: ffffffffad8dc7c0 RSI: 0000000024924925 RDI: ff3e27850dea2000 >> RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffffabfce739 >> R10: ff3e27810cc42400 R11: 0000000000000000 R12: ff3e2781825ef970 >> R13: 00000000ff3e2785 R14: 000000000000000c R15: 0000000000000001 >> FS: 00007f5195b51740(0000) GS:ff3e278b12d40000(0000) knlGS:0000000000000000 >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> CR2: 0000000000000010 CR3: 0000000626d24002 CR4: 0000000000361ee0 >> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 >> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 >> Call Trace: >> ima_get_action+0x22/0x30 >> process_measurement+0xb0/0x830 >> ima_file_check+0x64/0x90 >> path_openat+0x571/0x1720 >> do_filp_open+0x9b/0x110 >> do_sys_open+0x1bd/0x250 >> do_syscall_64+0x5d/0x1d0 >> entry_SYSCALL_64_after_hwframe+0x65/0xca >> >> (stack trace marked with ? is deleted) >> >> Commit c7423dbdbc9e ("ima: Handle -ESTALE returned by >> ima_filter_rule_match()") introduced call to ima_lsm_copy_rule within a >> RCU read-side critical section which contains kmalloc with GFP_KERNEL. >> This implies a possible sleep and violates limitations of RCU read-side >> critical sections on non-PREEMPT systems. >> >> Sleeping within RCU read-side critical section might cause >> synchronize_rcu() returning early and break RCU protection, allowing a >> UAF to happen. >> >> The root cause of this issue could be described as follows: >> | Thread A | Thread B | >> | |ima_match_policy | >> | | rcu_read_lock | >> |ima_lsm_update_rule | | >> | synchronize_rcu | | >> | | kmalloc(GFP_KERNEL)| >> | | sleep | >> ==> synchronize_rcu returns early >> | kfree(entry) | | >> | | entry = entry->next| >> ==> UAF happens and entry now becomes NULL (or could be anything). >> | | entry->action | >> ==> Accessing entry might cause panic. >> >> To fix this issue, we are converting all kmalloc that is called within >> RCU read-side critical section to use GFP_ATOMIC. >> >> Fixes: c7423dbdbc9e ("ima: Handle -ESTALE returned by ima_filter_rule_match()") >> Cc: stable@vger.kernel.org >> Signed-off-by: GUO Zihua <guozihua@huawei.com> >> --- >> security/integrity/ima/ima_policy.c | 2 +- >> security/selinux/ss/services.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c >> index c0556907c2e6..c0cf9b6a01f0 100644 >> --- a/security/integrity/ima/ima_policy.c >> +++ b/security/integrity/ima/ima_policy.c >> @@ -410,7 +410,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) >> * Immutable elements are copied over as pointers and data; only >> * lsm rules can change >> */ >> - nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL); >> + nentry = kmemdup(entry, sizeof(*nentry), GFP_ATOMIC); >> if (!nentry) >> return NULL; >> >> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c >> index e88b1b6c4adb..b7cfad1a2964 100644 >> --- a/security/selinux/ss/services.c >> +++ b/security/selinux/ss/services.c >> @@ -3549,7 +3549,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) >> return -EINVAL; >> } >> >> - tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_KERNEL); >> + tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_ATOMIC); > > I would suggest passing in gfp flags from the callers and only using > GFP_ATOMIC for the particular call chain that requires atomic > allocations, or re-factoring the caller to perform the allocating > operations outside of the critical section. Well unfortunately the SELinux involved in this patch is called via security hooks. Adding new flags would mean define macros as well as all the callers and implementations would have to be changed. > Sidebar: the refactoring of the SELinux policy loading logic may have > made it possible to revisit the approaches here to permit holding a > reference to the policy from which the rule was derived so that we > don't have to return -ESTALE in this scenario. This would be nice if we are able to make this re-init happen in-place which would spare IMA from initializing a temporary rule and the list replace behavior. > >> if (!tmprule) >> return -ENOMEM; >> context_init(&tmprule->au_ctxt); >> -- > >>
On Sun, Apr 21, 2024 at 9:26 PM Guozihua (Scott) <guozihua@huawei.com> wrote: > > On 2024/4/20 5:41, Stephen Smalley wrote: > > On Wed, Apr 17, 2024 at 12:49 AM GUO Zihua <guozihua@huawei.com> wrote: > >> > >> A panic happens in ima_match_policy: > >> > >> BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 > >> PGD 42f873067 P4D 0 > >> Oops: 0000 [#1] SMP NOPTI > >> CPU: 5 PID: 1286325 Comm: kubeletmonit.sh Kdump: loaded Tainted: P > >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015 > >> RIP: 0010:ima_match_policy+0x84/0x450 > >> Code: 49 89 fc 41 89 cf 31 ed 89 44 24 14 eb 1c 44 39 7b 18 74 26 41 83 ff 05 74 20 48 8b 1b 48 3b 1d f2 b9 f4 00 0f 84 9c 01 00 00 <44> 85 73 10 74 ea 44 8b 6b 14 41 f6 c5 01 75 d4 41 f6 c5 02 74 0f > >> RSP: 0018:ff71570009e07a80 EFLAGS: 00010207 > >> RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000200 > >> RDX: ffffffffad8dc7c0 RSI: 0000000024924925 RDI: ff3e27850dea2000 > >> RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffffabfce739 > >> R10: ff3e27810cc42400 R11: 0000000000000000 R12: ff3e2781825ef970 > >> R13: 00000000ff3e2785 R14: 000000000000000c R15: 0000000000000001 > >> FS: 00007f5195b51740(0000) GS:ff3e278b12d40000(0000) knlGS:0000000000000000 > >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > >> CR2: 0000000000000010 CR3: 0000000626d24002 CR4: 0000000000361ee0 > >> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > >> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > >> Call Trace: > >> ima_get_action+0x22/0x30 > >> process_measurement+0xb0/0x830 > >> ima_file_check+0x64/0x90 > >> path_openat+0x571/0x1720 > >> do_filp_open+0x9b/0x110 > >> do_sys_open+0x1bd/0x250 > >> do_syscall_64+0x5d/0x1d0 > >> entry_SYSCALL_64_after_hwframe+0x65/0xca > >> > >> (stack trace marked with ? is deleted) > >> > >> Commit c7423dbdbc9e ("ima: Handle -ESTALE returned by > >> ima_filter_rule_match()") introduced call to ima_lsm_copy_rule within a > >> RCU read-side critical section which contains kmalloc with GFP_KERNEL. > >> This implies a possible sleep and violates limitations of RCU read-side > >> critical sections on non-PREEMPT systems. > >> > >> Sleeping within RCU read-side critical section might cause > >> synchronize_rcu() returning early and break RCU protection, allowing a > >> UAF to happen. > >> > >> The root cause of this issue could be described as follows: > >> | Thread A | Thread B | > >> | |ima_match_policy | > >> | | rcu_read_lock | > >> |ima_lsm_update_rule | | > >> | synchronize_rcu | | > >> | | kmalloc(GFP_KERNEL)| > >> | | sleep | > >> ==> synchronize_rcu returns early > >> | kfree(entry) | | > >> | | entry = entry->next| > >> ==> UAF happens and entry now becomes NULL (or could be anything). > >> | | entry->action | > >> ==> Accessing entry might cause panic. > >> > >> To fix this issue, we are converting all kmalloc that is called within > >> RCU read-side critical section to use GFP_ATOMIC. > >> > >> Fixes: c7423dbdbc9e ("ima: Handle -ESTALE returned by ima_filter_rule_match()") > >> Cc: stable@vger.kernel.org > >> Signed-off-by: GUO Zihua <guozihua@huawei.com> > >> --- > >> security/integrity/ima/ima_policy.c | 2 +- > >> security/selinux/ss/services.c | 2 +- > >> 2 files changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > >> index c0556907c2e6..c0cf9b6a01f0 100644 > >> --- a/security/integrity/ima/ima_policy.c > >> +++ b/security/integrity/ima/ima_policy.c > >> @@ -410,7 +410,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) > >> * Immutable elements are copied over as pointers and data; only > >> * lsm rules can change > >> */ > >> - nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL); > >> + nentry = kmemdup(entry, sizeof(*nentry), GFP_ATOMIC); > >> if (!nentry) > >> return NULL; > >> > >> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > >> index e88b1b6c4adb..b7cfad1a2964 100644 > >> --- a/security/selinux/ss/services.c > >> +++ b/security/selinux/ss/services.c > >> @@ -3549,7 +3549,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) > >> return -EINVAL; > >> } > >> > >> - tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_KERNEL); > >> + tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_ATOMIC); > > > > I would suggest passing in gfp flags from the callers and only using > > GFP_ATOMIC for the particular call chain that requires atomic > > allocations, or re-factoring the caller to perform the allocating > > operations outside of the critical section. > > Well unfortunately the SELinux involved in this patch is called via > security hooks. Adding new flags would mean define macros as well as all > the callers and implementations would have to be changed. That is ok if it is what is required to fix this correctly. However, if you can re-factor the caller to move the allocation outside of the RCU read lock, or eliminate the need for an allocation altogether, that would be better. > > Sidebar: the refactoring of the SELinux policy loading logic may have > > made it possible to revisit the approaches here to permit holding a > > reference to the policy from which the rule was derived so that we > > don't have to return -ESTALE in this scenario. > > This would be nice if we are able to make this re-init happen in-place > which would spare IMA from initializing a temporary rule and the list > replace behavior. Yes, that would be best if possible.
On 2024/4/22 20:18, Stephen Smalley wrote: > On Sun, Apr 21, 2024 at 9:26 PM Guozihua (Scott) <guozihua@huawei.com> wrote: >> >> On 2024/4/20 5:41, Stephen Smalley wrote: >>> On Wed, Apr 17, 2024 at 12:49 AM GUO Zihua <guozihua@huawei.com> wrote: >>>> >>>> A panic happens in ima_match_policy: >>>> >>>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 >>>> PGD 42f873067 P4D 0 >>>> Oops: 0000 [#1] SMP NOPTI >>>> CPU: 5 PID: 1286325 Comm: kubeletmonit.sh Kdump: loaded Tainted: P >>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015 >>>> RIP: 0010:ima_match_policy+0x84/0x450 >>>> Code: 49 89 fc 41 89 cf 31 ed 89 44 24 14 eb 1c 44 39 7b 18 74 26 41 83 ff 05 74 20 48 8b 1b 48 3b 1d f2 b9 f4 00 0f 84 9c 01 00 00 <44> 85 73 10 74 ea 44 8b 6b 14 41 f6 c5 01 75 d4 41 f6 c5 02 74 0f >>>> RSP: 0018:ff71570009e07a80 EFLAGS: 00010207 >>>> RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000200 >>>> RDX: ffffffffad8dc7c0 RSI: 0000000024924925 RDI: ff3e27850dea2000 >>>> RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffffabfce739 >>>> R10: ff3e27810cc42400 R11: 0000000000000000 R12: ff3e2781825ef970 >>>> R13: 00000000ff3e2785 R14: 000000000000000c R15: 0000000000000001 >>>> FS: 00007f5195b51740(0000) GS:ff3e278b12d40000(0000) knlGS:0000000000000000 >>>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >>>> CR2: 0000000000000010 CR3: 0000000626d24002 CR4: 0000000000361ee0 >>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 >>>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 >>>> Call Trace: >>>> ima_get_action+0x22/0x30 >>>> process_measurement+0xb0/0x830 >>>> ima_file_check+0x64/0x90 >>>> path_openat+0x571/0x1720 >>>> do_filp_open+0x9b/0x110 >>>> do_sys_open+0x1bd/0x250 >>>> do_syscall_64+0x5d/0x1d0 >>>> entry_SYSCALL_64_after_hwframe+0x65/0xca >>>> >>>> (stack trace marked with ? is deleted) >>>> >>>> Commit c7423dbdbc9e ("ima: Handle -ESTALE returned by >>>> ima_filter_rule_match()") introduced call to ima_lsm_copy_rule within a >>>> RCU read-side critical section which contains kmalloc with GFP_KERNEL. >>>> This implies a possible sleep and violates limitations of RCU read-side >>>> critical sections on non-PREEMPT systems. >>>> >>>> Sleeping within RCU read-side critical section might cause >>>> synchronize_rcu() returning early and break RCU protection, allowing a >>>> UAF to happen. >>>> >>>> The root cause of this issue could be described as follows: >>>> | Thread A | Thread B | >>>> | |ima_match_policy | >>>> | | rcu_read_lock | >>>> |ima_lsm_update_rule | | >>>> | synchronize_rcu | | >>>> | | kmalloc(GFP_KERNEL)| >>>> | | sleep | >>>> ==> synchronize_rcu returns early >>>> | kfree(entry) | | >>>> | | entry = entry->next| >>>> ==> UAF happens and entry now becomes NULL (or could be anything). >>>> | | entry->action | >>>> ==> Accessing entry might cause panic. >>>> >>>> To fix this issue, we are converting all kmalloc that is called within >>>> RCU read-side critical section to use GFP_ATOMIC. >>>> >>>> Fixes: c7423dbdbc9e ("ima: Handle -ESTALE returned by ima_filter_rule_match()") >>>> Cc: stable@vger.kernel.org >>>> Signed-off-by: GUO Zihua <guozihua@huawei.com> >>>> --- >>>> security/integrity/ima/ima_policy.c | 2 +- >>>> security/selinux/ss/services.c | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c >>>> index c0556907c2e6..c0cf9b6a01f0 100644 >>>> --- a/security/integrity/ima/ima_policy.c >>>> +++ b/security/integrity/ima/ima_policy.c >>>> @@ -410,7 +410,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) >>>> * Immutable elements are copied over as pointers and data; only >>>> * lsm rules can change >>>> */ >>>> - nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL); >>>> + nentry = kmemdup(entry, sizeof(*nentry), GFP_ATOMIC); >>>> if (!nentry) >>>> return NULL; >>>> >>>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c >>>> index e88b1b6c4adb..b7cfad1a2964 100644 >>>> --- a/security/selinux/ss/services.c >>>> +++ b/security/selinux/ss/services.c >>>> @@ -3549,7 +3549,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) >>>> return -EINVAL; >>>> } >>>> >>>> - tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_KERNEL); >>>> + tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_ATOMIC); >>> >>> I would suggest passing in gfp flags from the callers and only using >>> GFP_ATOMIC for the particular call chain that requires atomic >>> allocations, or re-factoring the caller to perform the allocating >>> operations outside of the critical section. >> >> Well unfortunately the SELinux involved in this patch is called via >> security hooks. Adding new flags would mean define macros as well as all >> the callers and implementations would have to be changed. > > That is ok if it is what is required to fix this correctly. However, > if you can re-factor the caller to move the allocation outside of the > RCU read lock, or eliminate the need for an allocation altogether, > that would be better. This does not seems to be possible for now. The RCU lock circulates the whole list access while there might be more LSM related rules within the list. Besides we cannot release the RCU lock before allocation, this would trigger an UAF immediately. Will send a v2 adding a GFP mask for security hook as well as ima_lsm_copy_rule. > >>> Sidebar: the refactoring of the SELinux policy loading logic may have >>> made it possible to revisit the approaches here to permit holding a >>> reference to the policy from which the rule was derived so that we >>> don't have to return -ESTALE in this scenario. >> >> This would be nice if we are able to make this re-init happen in-place >> which would spare IMA from initializing a temporary rule and the list >> replace behavior. > > Yes, that would be best if possible.
On Fri, 2024-04-19 at 14:41 -0700, Stephen Smalley wrote: > On Wed, Apr 17, 2024 at 12:49 AM GUO Zihua <guozihua@huawei.com> wrote: > > > > A panic happens in ima_match_policy: > > > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 > > PGD 42f873067 P4D 0 > > Oops: 0000 [#1] SMP NOPTI > > CPU: 5 PID: 1286325 Comm: kubeletmonit.sh Kdump: loaded Tainted: P > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015 > > RIP: 0010:ima_match_policy+0x84/0x450 > > Code: 49 89 fc 41 89 cf 31 ed 89 44 24 14 eb 1c 44 39 7b 18 74 26 41 83 ff 05 74 20 48 8b 1b 48 3b 1d f2 b9 f4 00 0f 84 9c 01 00 00 <44> 85 73 10 74 ea 44 8b 6b 14 41 f6 c5 01 75 d4 41 f6 c5 02 74 0f > > RSP: 0018:ff71570009e07a80 EFLAGS: 00010207 > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000200 > > RDX: ffffffffad8dc7c0 RSI: 0000000024924925 RDI: ff3e27850dea2000 > > RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffffabfce739 > > R10: ff3e27810cc42400 R11: 0000000000000000 R12: ff3e2781825ef970 > > R13: 00000000ff3e2785 R14: 000000000000000c R15: 0000000000000001 > > FS: 00007f5195b51740(0000) GS:ff3e278b12d40000(0000) knlGS:0000000000000000 > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > CR2: 0000000000000010 CR3: 0000000626d24002 CR4: 0000000000361ee0 > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > > Call Trace: > > ima_get_action+0x22/0x30 > > process_measurement+0xb0/0x830 > > ima_file_check+0x64/0x90 > > path_openat+0x571/0x1720 > > do_filp_open+0x9b/0x110 > > do_sys_open+0x1bd/0x250 > > do_syscall_64+0x5d/0x1d0 > > entry_SYSCALL_64_after_hwframe+0x65/0xca > > > > (stack trace marked with ? is deleted) > > > > Commit c7423dbdbc9e ("ima: Handle -ESTALE returned by > > ima_filter_rule_match()") introduced call to ima_lsm_copy_rule within a > > RCU read-side critical section which contains kmalloc with GFP_KERNEL. > > This implies a possible sleep and violates limitations of RCU read-side > > critical sections on non-PREEMPT systems. > > > > Sleeping within RCU read-side critical section might cause > > synchronize_rcu() returning early and break RCU protection, allowing a > > UAF to happen. > > > > The root cause of this issue could be described as follows: > > > Thread A | Thread B | > > > |ima_match_policy | > > > | rcu_read_lock | > > > ima_lsm_update_rule | | > > > synchronize_rcu | | > > > | kmalloc(GFP_KERNEL)| > > > | sleep | > > ==> synchronize_rcu returns early > > > kfree(entry) | | > > > | entry = entry->next| > > ==> UAF happens and entry now becomes NULL (or could be anything). > > > | entry->action | > > ==> Accessing entry might cause panic. > > > > To fix this issue, we are converting all kmalloc that is called within > > RCU read-side critical section to use GFP_ATOMIC. > > > > Fixes: c7423dbdbc9e ("ima: Handle -ESTALE returned by ima_filter_rule_match()") > > Cc: stable@vger.kernel.org > > Signed-off-by: GUO Zihua <guozihua@huawei.com> > > --- > > security/integrity/ima/ima_policy.c | 2 +- > > security/selinux/ss/services.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > > index c0556907c2e6..c0cf9b6a01f0 100644 > > --- a/security/integrity/ima/ima_policy.c > > +++ b/security/integrity/ima/ima_policy.c > > @@ -410,7 +410,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) > > * Immutable elements are copied over as pointers and data; only > > * lsm rules can change > > */ > > - nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL); > > + nentry = kmemdup(entry, sizeof(*nentry), GFP_ATOMIC); > > if (!nentry) > > return NULL; > > > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > > index e88b1b6c4adb..b7cfad1a2964 100644 > > --- a/security/selinux/ss/services.c > > +++ b/security/selinux/ss/services.c > > @@ -3549,7 +3549,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) > > return -EINVAL; > > } > > > > - tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_KERNEL); > > + tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_ATOMIC); > > I would suggest passing in gfp flags from the callers and only using > GFP_ATOMIC for the particular call chain that requires atomic > allocations, or re-factoring the caller to perform the allocating > operations outside of the critical section. > Sidebar: the refactoring of the SELinux policy loading logic may have > made it possible to revisit the approaches here to permit holding a > reference to the policy from which the rule was derived so that we > don't have to return -ESTALE in this scenario. That would be really nice, would simplify the code for us. I was wondering if the label comparison would be still reliable after a policy update. I was thinking for example the case where a type is removed from the newer policy, and that type was used in an IMA rule. Thanks Roberto > > if (!tmprule) > > return -ENOMEM; > > context_init(&tmprule->au_ctxt); > > -- > > >
On Wed, 2024-05-08 at 10:32 +0200, Roberto Sassu wrote: > On Fri, 2024-04-19 at 14:41 -0700, Stephen Smalley wrote: > > On Wed, Apr 17, 2024 at 12:49 AM GUO Zihua <guozihua@huawei.com> wrote: > > > A panic happens in ima_match_policy: > > > > > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 > > > PGD 42f873067 P4D 0 > > > Oops: 0000 [#1] SMP NOPTI > > > CPU: 5 PID: 1286325 Comm: kubeletmonit.sh Kdump: loaded Tainted: P > > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015 > > > RIP: 0010:ima_match_policy+0x84/0x450 > > > Code: 49 89 fc 41 89 cf 31 ed 89 44 24 14 eb 1c 44 39 7b 18 74 26 41 83 ff 05 74 20 48 8b 1b 48 3b 1d f2 b9 f4 00 0f 84 9c 01 00 00 <44> 85 73 10 74 ea 44 8b 6b 14 41 f6 c5 01 75 d4 41 f6 c5 02 74 0f > > > RSP: 0018:ff71570009e07a80 EFLAGS: 00010207 > > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000200 > > > RDX: ffffffffad8dc7c0 RSI: 0000000024924925 RDI: ff3e27850dea2000 > > > RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffffabfce739 > > > R10: ff3e27810cc42400 R11: 0000000000000000 R12: ff3e2781825ef970 > > > R13: 00000000ff3e2785 R14: 000000000000000c R15: 0000000000000001 > > > FS: 00007f5195b51740(0000) GS:ff3e278b12d40000(0000) knlGS:0000000000000000 > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > > CR2: 0000000000000010 CR3: 0000000626d24002 CR4: 0000000000361ee0 > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > > > Call Trace: > > > ima_get_action+0x22/0x30 > > > process_measurement+0xb0/0x830 > > > ima_file_check+0x64/0x90 > > > path_openat+0x571/0x1720 > > > do_filp_open+0x9b/0x110 > > > do_sys_open+0x1bd/0x250 > > > do_syscall_64+0x5d/0x1d0 > > > entry_SYSCALL_64_after_hwframe+0x65/0xca > > > > > > (stack trace marked with ? is deleted) > > > > > > Commit c7423dbdbc9e ("ima: Handle -ESTALE returned by > > > ima_filter_rule_match()") introduced call to ima_lsm_copy_rule within a > > > RCU read-side critical section which contains kmalloc with GFP_KERNEL. > > > This implies a possible sleep and violates limitations of RCU read-side > > > critical sections on non-PREEMPT systems. > > > > > > Sleeping within RCU read-side critical section might cause > > > synchronize_rcu() returning early and break RCU protection, allowing a > > > UAF to happen. > > > > > > The root cause of this issue could be described as follows: > > > > Thread A | Thread B | > > > > |ima_match_policy | > > > > | rcu_read_lock | > > > > ima_lsm_update_rule | | > > > > synchronize_rcu | | > > > > | kmalloc(GFP_KERNEL)| > > > > | sleep | > > > ==> synchronize_rcu returns early > > > > kfree(entry) | | > > > > | entry = entry->next| > > > ==> UAF happens and entry now becomes NULL (or could be anything). > > > > | entry->action | > > > ==> Accessing entry might cause panic. > > > > > > To fix this issue, we are converting all kmalloc that is called within > > > RCU read-side critical section to use GFP_ATOMIC. > > > > > > Fixes: c7423dbdbc9e ("ima: Handle -ESTALE returned by ima_filter_rule_match()") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: GUO Zihua <guozihua@huawei.com> > > > --- > > > security/integrity/ima/ima_policy.c | 2 +- > > > security/selinux/ss/services.c | 2 +- > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > > > index c0556907c2e6..c0cf9b6a01f0 100644 > > > --- a/security/integrity/ima/ima_policy.c > > > +++ b/security/integrity/ima/ima_policy.c > > > @@ -410,7 +410,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) > > > * Immutable elements are copied over as pointers and data; only > > > * lsm rules can change > > > */ > > > - nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL); > > > + nentry = kmemdup(entry, sizeof(*nentry), GFP_ATOMIC); > > > if (!nentry) > > > return NULL; > > > > > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > > > index e88b1b6c4adb..b7cfad1a2964 100644 > > > --- a/security/selinux/ss/services.c > > > +++ b/security/selinux/ss/services.c > > > @@ -3549,7 +3549,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) > > > return -EINVAL; > > > } > > > > > > - tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_KERNEL); > > > + tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_ATOMIC); > > > > I would suggest passing in gfp flags from the callers and only using > > GFP_ATOMIC for the particular call chain that requires atomic > > allocations, or re-factoring the caller to perform the allocating > > operations outside of the critical section. > > Sidebar: the refactoring of the SELinux policy loading logic may have > > made it possible to revisit the approaches here to permit holding a > > reference to the policy from which the rule was derived so that we > > don't have to return -ESTALE in this scenario. > > That would be really nice, would simplify the code for us. > > I was wondering if the label comparison would be still reliable after a > policy update. I was thinking for example the case where a type is > removed from the newer policy, and that type was used in an IMA rule. Yes, I was concerned too. We followed Stephen's suggestion to ignore the IMA policy rules containing an unknown SELinux label. The original discussion can be found here: https://lore.kernel.org/linux-integrity/1432f617-424e-044c-4f78-47f1100262ae@tycho.nsa.gov/ thanks, Mimi
On Wed, May 8, 2024 at 4:33 AM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > > On Fri, 2024-04-19 at 14:41 -0700, Stephen Smalley wrote: > > On Wed, Apr 17, 2024 at 12:49 AM GUO Zihua <guozihua@huawei.com> wrote: > > > > > > A panic happens in ima_match_policy: > > > > > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 > > > PGD 42f873067 P4D 0 > > > Oops: 0000 [#1] SMP NOPTI > > > CPU: 5 PID: 1286325 Comm: kubeletmonit.sh Kdump: loaded Tainted: P > > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015 > > > RIP: 0010:ima_match_policy+0x84/0x450 > > > Code: 49 89 fc 41 89 cf 31 ed 89 44 24 14 eb 1c 44 39 7b 18 74 26 41 83 ff 05 74 20 48 8b 1b 48 3b 1d f2 b9 f4 00 0f 84 9c 01 00 00 <44> 85 73 10 74 ea 44 8b 6b 14 41 f6 c5 01 75 d4 41 f6 c5 02 74 0f > > > RSP: 0018:ff71570009e07a80 EFLAGS: 00010207 > > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000200 > > > RDX: ffffffffad8dc7c0 RSI: 0000000024924925 RDI: ff3e27850dea2000 > > > RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffffabfce739 > > > R10: ff3e27810cc42400 R11: 0000000000000000 R12: ff3e2781825ef970 > > > R13: 00000000ff3e2785 R14: 000000000000000c R15: 0000000000000001 > > > FS: 00007f5195b51740(0000) GS:ff3e278b12d40000(0000) knlGS:0000000000000000 > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > > CR2: 0000000000000010 CR3: 0000000626d24002 CR4: 0000000000361ee0 > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > > > Call Trace: > > > ima_get_action+0x22/0x30 > > > process_measurement+0xb0/0x830 > > > ima_file_check+0x64/0x90 > > > path_openat+0x571/0x1720 > > > do_filp_open+0x9b/0x110 > > > do_sys_open+0x1bd/0x250 > > > do_syscall_64+0x5d/0x1d0 > > > entry_SYSCALL_64_after_hwframe+0x65/0xca > > > > > > (stack trace marked with ? is deleted) > > > > > > Commit c7423dbdbc9e ("ima: Handle -ESTALE returned by > > > ima_filter_rule_match()") introduced call to ima_lsm_copy_rule within a > > > RCU read-side critical section which contains kmalloc with GFP_KERNEL. > > > This implies a possible sleep and violates limitations of RCU read-side > > > critical sections on non-PREEMPT systems. > > > > > > Sleeping within RCU read-side critical section might cause > > > synchronize_rcu() returning early and break RCU protection, allowing a > > > UAF to happen. > > > > > > The root cause of this issue could be described as follows: > > > > Thread A | Thread B | > > > > |ima_match_policy | > > > > | rcu_read_lock | > > > > ima_lsm_update_rule | | > > > > synchronize_rcu | | > > > > | kmalloc(GFP_KERNEL)| > > > > | sleep | > > > ==> synchronize_rcu returns early > > > > kfree(entry) | | > > > > | entry = entry->next| > > > ==> UAF happens and entry now becomes NULL (or could be anything). > > > > | entry->action | > > > ==> Accessing entry might cause panic. > > > > > > To fix this issue, we are converting all kmalloc that is called within > > > RCU read-side critical section to use GFP_ATOMIC. > > > > > > Fixes: c7423dbdbc9e ("ima: Handle -ESTALE returned by ima_filter_rule_match()") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: GUO Zihua <guozihua@huawei.com> > > > --- > > > security/integrity/ima/ima_policy.c | 2 +- > > > security/selinux/ss/services.c | 2 +- > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > > > index c0556907c2e6..c0cf9b6a01f0 100644 > > > --- a/security/integrity/ima/ima_policy.c > > > +++ b/security/integrity/ima/ima_policy.c > > > @@ -410,7 +410,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) > > > * Immutable elements are copied over as pointers and data; only > > > * lsm rules can change > > > */ > > > - nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL); > > > + nentry = kmemdup(entry, sizeof(*nentry), GFP_ATOMIC); > > > if (!nentry) > > > return NULL; > > > > > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > > > index e88b1b6c4adb..b7cfad1a2964 100644 > > > --- a/security/selinux/ss/services.c > > > +++ b/security/selinux/ss/services.c > > > @@ -3549,7 +3549,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) > > > return -EINVAL; > > > } > > > > > > - tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_KERNEL); > > > + tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_ATOMIC); > > > > I would suggest passing in gfp flags from the callers and only using > > GFP_ATOMIC for the particular call chain that requires atomic > > allocations, or re-factoring the caller to perform the allocating > > operations outside of the critical section. > > Sidebar: the refactoring of the SELinux policy loading logic may have > > made it possible to revisit the approaches here to permit holding a > > reference to the policy from which the rule was derived so that we > > don't have to return -ESTALE in this scenario. > > That would be really nice, would simplify the code for us. > > I was wondering if the label comparison would be still reliable after a > policy update. I was thinking for example the case where a type is > removed from the newer policy, and that type was used in an IMA rule. The basic idea would be to add a refcount to struct selinux_policy, add a reference to the corresponding selinux_policy in selinux_audit_rule (same as for ima), and defer the freeing of the selinux_policy until the refcount goes to zero. And have selinux_audit_rule_match() use the policy from the audit rule instead of the global state/active policy. That said, looking at it again, I'm not confident that would work since we'd likely end up looking up a SID from one policy in the SID table of another...
On Wed, May 8, 2024 at 11:15 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Wed, May 8, 2024 at 4:33 AM Roberto Sassu > <roberto.sassu@huaweicloud.com> wrote: > > > > On Fri, 2024-04-19 at 14:41 -0700, Stephen Smalley wrote: > > > On Wed, Apr 17, 2024 at 12:49 AM GUO Zihua <guozihua@huawei.com> wrote: > > > > > > > > A panic happens in ima_match_policy: > > > > > > > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 > > > > PGD 42f873067 P4D 0 > > > > Oops: 0000 [#1] SMP NOPTI > > > > CPU: 5 PID: 1286325 Comm: kubeletmonit.sh Kdump: loaded Tainted: P > > > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015 > > > > RIP: 0010:ima_match_policy+0x84/0x450 > > > > Code: 49 89 fc 41 89 cf 31 ed 89 44 24 14 eb 1c 44 39 7b 18 74 26 41 83 ff 05 74 20 48 8b 1b 48 3b 1d f2 b9 f4 00 0f 84 9c 01 00 00 <44> 85 73 10 74 ea 44 8b 6b 14 41 f6 c5 01 75 d4 41 f6 c5 02 74 0f > > > > RSP: 0018:ff71570009e07a80 EFLAGS: 00010207 > > > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000200 > > > > RDX: ffffffffad8dc7c0 RSI: 0000000024924925 RDI: ff3e27850dea2000 > > > > RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffffabfce739 > > > > R10: ff3e27810cc42400 R11: 0000000000000000 R12: ff3e2781825ef970 > > > > R13: 00000000ff3e2785 R14: 000000000000000c R15: 0000000000000001 > > > > FS: 00007f5195b51740(0000) GS:ff3e278b12d40000(0000) knlGS:0000000000000000 > > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > > > CR2: 0000000000000010 CR3: 0000000626d24002 CR4: 0000000000361ee0 > > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > > > > Call Trace: > > > > ima_get_action+0x22/0x30 > > > > process_measurement+0xb0/0x830 > > > > ima_file_check+0x64/0x90 > > > > path_openat+0x571/0x1720 > > > > do_filp_open+0x9b/0x110 > > > > do_sys_open+0x1bd/0x250 > > > > do_syscall_64+0x5d/0x1d0 > > > > entry_SYSCALL_64_after_hwframe+0x65/0xca > > > > > > > > (stack trace marked with ? is deleted) > > > > > > > > Commit c7423dbdbc9e ("ima: Handle -ESTALE returned by > > > > ima_filter_rule_match()") introduced call to ima_lsm_copy_rule within a > > > > RCU read-side critical section which contains kmalloc with GFP_KERNEL. > > > > This implies a possible sleep and violates limitations of RCU read-side > > > > critical sections on non-PREEMPT systems. > > > > > > > > Sleeping within RCU read-side critical section might cause > > > > synchronize_rcu() returning early and break RCU protection, allowing a > > > > UAF to happen. > > > > > > > > The root cause of this issue could be described as follows: > > > > > Thread A | Thread B | > > > > > |ima_match_policy | > > > > > | rcu_read_lock | > > > > > ima_lsm_update_rule | | > > > > > synchronize_rcu | | > > > > > | kmalloc(GFP_KERNEL)| > > > > > | sleep | > > > > ==> synchronize_rcu returns early > > > > > kfree(entry) | | > > > > > | entry = entry->next| > > > > ==> UAF happens and entry now becomes NULL (or could be anything). > > > > > | entry->action | > > > > ==> Accessing entry might cause panic. > > > > > > > > To fix this issue, we are converting all kmalloc that is called within > > > > RCU read-side critical section to use GFP_ATOMIC. > > > > > > > > Fixes: c7423dbdbc9e ("ima: Handle -ESTALE returned by ima_filter_rule_match()") > > > > Cc: stable@vger.kernel.org > > > > Signed-off-by: GUO Zihua <guozihua@huawei.com> > > > > --- > > > > security/integrity/ima/ima_policy.c | 2 +- > > > > security/selinux/ss/services.c | 2 +- > > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > > > > index c0556907c2e6..c0cf9b6a01f0 100644 > > > > --- a/security/integrity/ima/ima_policy.c > > > > +++ b/security/integrity/ima/ima_policy.c > > > > @@ -410,7 +410,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) > > > > * Immutable elements are copied over as pointers and data; only > > > > * lsm rules can change > > > > */ > > > > - nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL); > > > > + nentry = kmemdup(entry, sizeof(*nentry), GFP_ATOMIC); > > > > if (!nentry) > > > > return NULL; > > > > > > > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > > > > index e88b1b6c4adb..b7cfad1a2964 100644 > > > > --- a/security/selinux/ss/services.c > > > > +++ b/security/selinux/ss/services.c > > > > @@ -3549,7 +3549,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) > > > > return -EINVAL; > > > > } > > > > > > > > - tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_KERNEL); > > > > + tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_ATOMIC); > > > > > > I would suggest passing in gfp flags from the callers and only using > > > GFP_ATOMIC for the particular call chain that requires atomic > > > allocations, or re-factoring the caller to perform the allocating > > > operations outside of the critical section. > > > Sidebar: the refactoring of the SELinux policy loading logic may have > > > made it possible to revisit the approaches here to permit holding a > > > reference to the policy from which the rule was derived so that we > > > don't have to return -ESTALE in this scenario. > > > > That would be really nice, would simplify the code for us. > > > > I was wondering if the label comparison would be still reliable after a > > policy update. I was thinking for example the case where a type is > > removed from the newer policy, and that type was used in an IMA rule. > > The basic idea would be to add a refcount to struct selinux_policy, > add a reference to the corresponding selinux_policy in > selinux_audit_rule (same as for ima), and defer the freeing of the > selinux_policy until the refcount goes to zero. And have > selinux_audit_rule_match() use the policy from the audit rule instead > of the global state/active policy. That said, looking at it again, I'm > not confident that would work since we'd likely end up looking up a > SID from one policy in the SID table of another... Actually, that might be ok since on policy reload we convert over all existing SIDs, so the SIDs themselves don't change. So we'd look up the SID in the old policy's SID table, obtaining its old user/role/type/level values, and compare against the values in the audit rule created from the old policy. In theory, that should work. YMMV.
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index c0556907c2e6..c0cf9b6a01f0 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -410,7 +410,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) * Immutable elements are copied over as pointers and data; only * lsm rules can change */ - nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL); + nentry = kmemdup(entry, sizeof(*nentry), GFP_ATOMIC); if (!nentry) return NULL; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index e88b1b6c4adb..b7cfad1a2964 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -3549,7 +3549,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) return -EINVAL; } - tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_KERNEL); + tmprule = kzalloc(sizeof(struct selinux_audit_rule), GFP_ATOMIC); if (!tmprule) return -ENOMEM; context_init(&tmprule->au_ctxt);
A panic happens in ima_match_policy: BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 PGD 42f873067 P4D 0 Oops: 0000 [#1] SMP NOPTI CPU: 5 PID: 1286325 Comm: kubeletmonit.sh Kdump: loaded Tainted: P Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015 RIP: 0010:ima_match_policy+0x84/0x450 Code: 49 89 fc 41 89 cf 31 ed 89 44 24 14 eb 1c 44 39 7b 18 74 26 41 83 ff 05 74 20 48 8b 1b 48 3b 1d f2 b9 f4 00 0f 84 9c 01 00 00 <44> 85 73 10 74 ea 44 8b 6b 14 41 f6 c5 01 75 d4 41 f6 c5 02 74 0f RSP: 0018:ff71570009e07a80 EFLAGS: 00010207 RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000200 RDX: ffffffffad8dc7c0 RSI: 0000000024924925 RDI: ff3e27850dea2000 RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffffabfce739 R10: ff3e27810cc42400 R11: 0000000000000000 R12: ff3e2781825ef970 R13: 00000000ff3e2785 R14: 000000000000000c R15: 0000000000000001 FS: 00007f5195b51740(0000) GS:ff3e278b12d40000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000010 CR3: 0000000626d24002 CR4: 0000000000361ee0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: ima_get_action+0x22/0x30 process_measurement+0xb0/0x830 ima_file_check+0x64/0x90 path_openat+0x571/0x1720 do_filp_open+0x9b/0x110 do_sys_open+0x1bd/0x250 do_syscall_64+0x5d/0x1d0 entry_SYSCALL_64_after_hwframe+0x65/0xca (stack trace marked with ? is deleted) Commit c7423dbdbc9e ("ima: Handle -ESTALE returned by ima_filter_rule_match()") introduced call to ima_lsm_copy_rule within a RCU read-side critical section which contains kmalloc with GFP_KERNEL. This implies a possible sleep and violates limitations of RCU read-side critical sections on non-PREEMPT systems. Sleeping within RCU read-side critical section might cause synchronize_rcu() returning early and break RCU protection, allowing a UAF to happen. The root cause of this issue could be described as follows: | Thread A | Thread B | | |ima_match_policy | | | rcu_read_lock | |ima_lsm_update_rule | | | synchronize_rcu | | | | kmalloc(GFP_KERNEL)| | | sleep | ==> synchronize_rcu returns early | kfree(entry) | | | | entry = entry->next| ==> UAF happens and entry now becomes NULL (or could be anything). | | entry->action | ==> Accessing entry might cause panic. To fix this issue, we are converting all kmalloc that is called within RCU read-side critical section to use GFP_ATOMIC. Fixes: c7423dbdbc9e ("ima: Handle -ESTALE returned by ima_filter_rule_match()") Cc: stable@vger.kernel.org Signed-off-by: GUO Zihua <guozihua@huawei.com> --- security/integrity/ima/ima_policy.c | 2 +- security/selinux/ss/services.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)