* [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-20 9:18 ` Ethan Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Ethan Zhao @ 2015-01-20 9:18 UTC (permalink / raw)
To: james.l.morris, serge, eparis, sds, paul
Cc: selinux, linux-security-module, linux-kernel, ethan.kernel, Ethan Zhao
A NULL pointer dereference was observed as following panic:
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
...
Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
task ffff88022ac70e40)
Stack:
ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
ffffffff8125818e
0000000000000001 0000000037f78004 ffff880237f2bcd8
ffffffff81273619
ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
ffffffff8125c206
Call Trace:
[<ffffffff8125818e>] ? ipcperms+0xae/0x110
[<ffffffff81273619>] selinux_sem_semop+0x19/0x20
[<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
[<ffffffff8125c206>] sys_semtimedop+0x346/0x750
[<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
[<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
[<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
[<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
[<ffffffff8109a763>] ? up_read+0x23/0x40
[<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
[<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
[<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
[<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
[<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
[<ffffffff81622f45>] ? sysret_check+0x22/0x5d
[<ffffffff8125c620>] sys_semop+0x10/0x20
[<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04 89 55 d8
<0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
RSP <ffff880237f2bc98>
CR2: 0000000000000000
The root cause is semtimedop() was called after semget() without checking its
return value in process opcmon. and semget() failed to check permission in
function avc_has_perm() then sem_perm->security was freed shown as following:
sys_semget()
->newary()
->security_sem_alloc()
->sem_alloc_security()
selinux_sem_alloc_security()
->ipc_alloc_security() {
->rc = avc_has_perm()
if (rc) {
ipc_free_security(&sma->sem_perm);
return rc;
So ipc_perms->security was NULL, then semtimedop() was called as
following:
sys_semtimedop() / semop()
->selinux_sem_semop()
->ipc_has_perm()
->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
^- NULL pointer dereference happens
The test kernel was running on VMware.
This patch use to fix this serious security issue could be triggered by user space.
This patch was tested with v3.19-rc5.
Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
---
security/selinux/hooks.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6da7532..bbe76f5 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
u32 sid = current_sid();
isec = ipc_perms->security;
+ if (!isec)
+ return -EACCES;
ad.type = LSM_AUDIT_DATA_IPC;
ad.u.ipc_id = ipc_perms->key;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-20 9:18 ` Ethan Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Ethan Zhao @ 2015-01-20 9:18 UTC (permalink / raw)
To: james.l.morris, serge, eparis, sds, paul
Cc: ethan.kernel, linux-security-module, Ethan Zhao, linux-kernel, selinux
A NULL pointer dereference was observed as following panic:
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
...
Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
task ffff88022ac70e40)
Stack:
ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
ffffffff8125818e
0000000000000001 0000000037f78004 ffff880237f2bcd8
ffffffff81273619
ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
ffffffff8125c206
Call Trace:
[<ffffffff8125818e>] ? ipcperms+0xae/0x110
[<ffffffff81273619>] selinux_sem_semop+0x19/0x20
[<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
[<ffffffff8125c206>] sys_semtimedop+0x346/0x750
[<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
[<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
[<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
[<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
[<ffffffff8109a763>] ? up_read+0x23/0x40
[<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
[<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
[<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
[<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
[<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
[<ffffffff81622f45>] ? sysret_check+0x22/0x5d
[<ffffffff8125c620>] sys_semop+0x10/0x20
[<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04 89 55 d8
<0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
RSP <ffff880237f2bc98>
CR2: 0000000000000000
The root cause is semtimedop() was called after semget() without checking its
return value in process opcmon. and semget() failed to check permission in
function avc_has_perm() then sem_perm->security was freed shown as following:
sys_semget()
->newary()
->security_sem_alloc()
->sem_alloc_security()
selinux_sem_alloc_security()
->ipc_alloc_security() {
->rc = avc_has_perm()
if (rc) {
ipc_free_security(&sma->sem_perm);
return rc;
So ipc_perms->security was NULL, then semtimedop() was called as
following:
sys_semtimedop() / semop()
->selinux_sem_semop()
->ipc_has_perm()
->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
^- NULL pointer dereference happens
The test kernel was running on VMware.
This patch use to fix this serious security issue could be triggered by user space.
This patch was tested with v3.19-rc5.
Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
---
security/selinux/hooks.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6da7532..bbe76f5 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
u32 sid = current_sid();
isec = ipc_perms->security;
+ if (!isec)
+ return -EACCES;
ad.type = LSM_AUDIT_DATA_IPC;
ad.u.ipc_id = ipc_perms->key;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-20 9:18 ` Ethan Zhao
@ 2015-01-20 14:10 ` Stephen Smalley
-1 siblings, 0 replies; 35+ messages in thread
From: Stephen Smalley @ 2015-01-20 14:10 UTC (permalink / raw)
To: Ethan Zhao, james.l.morris, serge, eparis, paul
Cc: selinux, linux-security-module, linux-kernel, ethan.kernel, manfred
On 01/20/2015 04:18 AM, Ethan Zhao wrote:
> A NULL pointer dereference was observed as following panic:
>
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
> ...
> Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
> task ffff88022ac70e40)
> Stack:
> ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
> ffffffff8125818e
> 0000000000000001 0000000037f78004 ffff880237f2bcd8
> ffffffff81273619
> ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
> ffffffff8125c206
> Call Trace:
> [<ffffffff8125818e>] ? ipcperms+0xae/0x110
> [<ffffffff81273619>] selinux_sem_semop+0x19/0x20
> [<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
> [<ffffffff8125c206>] sys_semtimedop+0x346/0x750
> [<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
> [<ffffffff8109a763>] ? up_read+0x23/0x40
> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
> [<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
> [<ffffffff81622f45>] ? sysret_check+0x22/0x5d
> [<ffffffff8125c620>] sys_semop+0x10/0x20
> [<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
> Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
> 45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04 89 55 d8
> <0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
> RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
> RSP <ffff880237f2bc98>
> CR2: 0000000000000000
>
> The root cause is semtimedop() was called after semget() without checking its
> return value in process opcmon. and semget() failed to check permission in
> function avc_has_perm() then sem_perm->security was freed shown as following:
>
> sys_semget()
> ->newary()
> ->security_sem_alloc()
> ->sem_alloc_security()
> selinux_sem_alloc_security()
> ->ipc_alloc_security() {
> ->rc = avc_has_perm()
> if (rc) {
> ipc_free_security(&sma->sem_perm);
> return rc;
We free the security structure here to avoid a memory leak on a
failed/denied semaphore set creation. In this situation, we return an
error to the caller (ultimately to newary), it does an
ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
caller. Thus, it never calls ipc_addid() and the semaphore set is not
created. So how then can you call semtimedop() on it?
> So ipc_perms->security was NULL, then semtimedop() was called as
> following:
>
> sys_semtimedop() / semop()
> ->selinux_sem_semop()
> ->ipc_has_perm()
> ->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
> ^- NULL pointer dereference happens
>
> The test kernel was running on VMware.
> This patch use to fix this serious security issue could be triggered by user space.
> This patch was tested with v3.19-rc5.
>
> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
> ---
> security/selinux/hooks.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6da7532..bbe76f5 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
> u32 sid = current_sid();
>
> isec = ipc_perms->security;
> + if (!isec)
> + return -EACCES;
>
> ad.type = LSM_AUDIT_DATA_IPC;
> ad.u.ipc_id = ipc_perms->key;
>
That is not the correct fix; it just hides a bug. If we reach
ipc_has_perm() with a NULL isec, it is a bug in the ipc code.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-20 14:10 ` Stephen Smalley
0 siblings, 0 replies; 35+ messages in thread
From: Stephen Smalley @ 2015-01-20 14:10 UTC (permalink / raw)
To: Ethan Zhao, james.l.morris, serge, eparis, paul
Cc: ethan.kernel, linux-security-module, manfred, linux-kernel, selinux
On 01/20/2015 04:18 AM, Ethan Zhao wrote:
> A NULL pointer dereference was observed as following panic:
>
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
> ...
> Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
> task ffff88022ac70e40)
> Stack:
> ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
> ffffffff8125818e
> 0000000000000001 0000000037f78004 ffff880237f2bcd8
> ffffffff81273619
> ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
> ffffffff8125c206
> Call Trace:
> [<ffffffff8125818e>] ? ipcperms+0xae/0x110
> [<ffffffff81273619>] selinux_sem_semop+0x19/0x20
> [<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
> [<ffffffff8125c206>] sys_semtimedop+0x346/0x750
> [<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
> [<ffffffff8109a763>] ? up_read+0x23/0x40
> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
> [<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
> [<ffffffff81622f45>] ? sysret_check+0x22/0x5d
> [<ffffffff8125c620>] sys_semop+0x10/0x20
> [<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
> Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
> 45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04 89 55 d8
> <0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
> RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
> RSP <ffff880237f2bc98>
> CR2: 0000000000000000
>
> The root cause is semtimedop() was called after semget() without checking its
> return value in process opcmon. and semget() failed to check permission in
> function avc_has_perm() then sem_perm->security was freed shown as following:
>
> sys_semget()
> ->newary()
> ->security_sem_alloc()
> ->sem_alloc_security()
> selinux_sem_alloc_security()
> ->ipc_alloc_security() {
> ->rc = avc_has_perm()
> if (rc) {
> ipc_free_security(&sma->sem_perm);
> return rc;
We free the security structure here to avoid a memory leak on a
failed/denied semaphore set creation. In this situation, we return an
error to the caller (ultimately to newary), it does an
ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
caller. Thus, it never calls ipc_addid() and the semaphore set is not
created. So how then can you call semtimedop() on it?
> So ipc_perms->security was NULL, then semtimedop() was called as
> following:
>
> sys_semtimedop() / semop()
> ->selinux_sem_semop()
> ->ipc_has_perm()
> ->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
> ^- NULL pointer dereference happens
>
> The test kernel was running on VMware.
> This patch use to fix this serious security issue could be triggered by user space.
> This patch was tested with v3.19-rc5.
>
> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
> ---
> security/selinux/hooks.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6da7532..bbe76f5 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
> u32 sid = current_sid();
>
> isec = ipc_perms->security;
> + if (!isec)
> + return -EACCES;
>
> ad.type = LSM_AUDIT_DATA_IPC;
> ad.u.ipc_id = ipc_perms->key;
>
That is not the correct fix; it just hides a bug. If we reach
ipc_has_perm() with a NULL isec, it is a bug in the ipc code.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-20 14:10 ` Stephen Smalley
@ 2015-01-20 18:49 ` Manfred Spraul
-1 siblings, 0 replies; 35+ messages in thread
From: Manfred Spraul @ 2015-01-20 18:49 UTC (permalink / raw)
To: Stephen Smalley, Ethan Zhao, james.l.morris, serge, eparis, paul
Cc: selinux, linux-security-module, linux-kernel, ethan.kernel
Hi,
On 01/20/2015 03:10 PM, Stephen Smalley wrote:
> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>> A NULL pointer dereference was observed as following panic:
>>
>> BUG: unable to handle kernel NULL pointer dereference at (null)
>> IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>> ...
>> Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
>> task ffff88022ac70e40)
>> Stack:
>> ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
>> ffffffff8125818e
>> 0000000000000001 0000000037f78004 ffff880237f2bcd8
>> ffffffff81273619
>> ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
>> ffffffff8125c206
>> Call Trace:
>> [<ffffffff8125818e>] ? ipcperms+0xae/0x110
>> [<ffffffff81273619>] selinux_sem_semop+0x19/0x20
>> [<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
>> [<ffffffff8125c206>] sys_semtimedop+0x346/0x750
>> [<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>> [<ffffffff8109a763>] ? up_read+0x23/0x40
>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>> [<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>> [<ffffffff81622f45>] ? sysret_check+0x22/0x5d
>> [<ffffffff8125c620>] sys_semop+0x10/0x20
>> [<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
>> Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
>> 45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04 89 55 d8
>> <0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
>> RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>> RSP <ffff880237f2bc98>
>> CR2: 0000000000000000
>>
>> The root cause is semtimedop() was called after semget() without checking its
>> return value in process opcmon. and semget() failed to check permission in
>> function avc_has_perm() then sem_perm->security was freed shown as following:
>>
>> sys_semget()
>> ->newary()
>> ->security_sem_alloc()
>> ->sem_alloc_security()
>> selinux_sem_alloc_security()
>> ->ipc_alloc_security() {
>> ->rc = avc_has_perm()
>> if (rc) {
>> ipc_free_security(&sma->sem_perm);
>> return rc;
> We free the security structure here to avoid a memory leak on a
> failed/denied semaphore set creation. In this situation, we return an
> error to the caller (ultimately to newary), it does an
> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
> caller. Thus, it never calls ipc_addid() and the semaphore set is not
> created. So how then can you call semtimedop() on it?
My only idea would be a race of semtimedop() with IPC_RMID:
If a rcu grace period happens between sem_obtain_object_check() and the
ipc_has_perm() call, the the observed NULL pointer assignment would happen.
>> So ipc_perms->security was NULL, then semtimedop() was called as
>> following:
>>
>> sys_semtimedop() / semop()
>> ->selinux_sem_semop()
>> ->ipc_has_perm()
>> ->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
>> ^- NULL pointer dereference happens
>>
>> The test kernel was running on VMware.
Can you give some more details?
- which RCU type?
- PREEMPT?
--
Manfred
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-20 18:49 ` Manfred Spraul
0 siblings, 0 replies; 35+ messages in thread
From: Manfred Spraul @ 2015-01-20 18:49 UTC (permalink / raw)
To: Stephen Smalley, Ethan Zhao, james.l.morris, serge, eparis, paul
Cc: ethan.kernel, linux-security-module, linux-kernel, selinux
Hi,
On 01/20/2015 03:10 PM, Stephen Smalley wrote:
> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>> A NULL pointer dereference was observed as following panic:
>>
>> BUG: unable to handle kernel NULL pointer dereference at (null)
>> IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>> ...
>> Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
>> task ffff88022ac70e40)
>> Stack:
>> ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
>> ffffffff8125818e
>> 0000000000000001 0000000037f78004 ffff880237f2bcd8
>> ffffffff81273619
>> ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
>> ffffffff8125c206
>> Call Trace:
>> [<ffffffff8125818e>] ? ipcperms+0xae/0x110
>> [<ffffffff81273619>] selinux_sem_semop+0x19/0x20
>> [<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
>> [<ffffffff8125c206>] sys_semtimedop+0x346/0x750
>> [<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>> [<ffffffff8109a763>] ? up_read+0x23/0x40
>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>> [<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>> [<ffffffff81622f45>] ? sysret_check+0x22/0x5d
>> [<ffffffff8125c620>] sys_semop+0x10/0x20
>> [<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
>> Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
>> 45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04 89 55 d8
>> <0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
>> RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>> RSP <ffff880237f2bc98>
>> CR2: 0000000000000000
>>
>> The root cause is semtimedop() was called after semget() without checking its
>> return value in process opcmon. and semget() failed to check permission in
>> function avc_has_perm() then sem_perm->security was freed shown as following:
>>
>> sys_semget()
>> ->newary()
>> ->security_sem_alloc()
>> ->sem_alloc_security()
>> selinux_sem_alloc_security()
>> ->ipc_alloc_security() {
>> ->rc = avc_has_perm()
>> if (rc) {
>> ipc_free_security(&sma->sem_perm);
>> return rc;
> We free the security structure here to avoid a memory leak on a
> failed/denied semaphore set creation. In this situation, we return an
> error to the caller (ultimately to newary), it does an
> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
> caller. Thus, it never calls ipc_addid() and the semaphore set is not
> created. So how then can you call semtimedop() on it?
My only idea would be a race of semtimedop() with IPC_RMID:
If a rcu grace period happens between sem_obtain_object_check() and the
ipc_has_perm() call, the the observed NULL pointer assignment would happen.
>> So ipc_perms->security was NULL, then semtimedop() was called as
>> following:
>>
>> sys_semtimedop() / semop()
>> ->selinux_sem_semop()
>> ->ipc_has_perm()
>> ->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
>> ^- NULL pointer dereference happens
>>
>> The test kernel was running on VMware.
Can you give some more details?
- which RCU type?
- PREEMPT?
--
Manfred
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-20 18:49 ` Manfred Spraul
(?)
@ 2015-01-20 21:01 ` Stephen Smalley
2015-01-20 21:06 ` Eric Paris
-1 siblings, 1 reply; 35+ messages in thread
From: Stephen Smalley @ 2015-01-20 21:01 UTC (permalink / raw)
To: Manfred Spraul, Ethan Zhao, james.l.morris, serge, eparis, paul
Cc: ethan.kernel, linux-security-module, linux-kernel, selinux
On 01/20/2015 01:49 PM, Manfred Spraul wrote:
> Hi,
>
> On 01/20/2015 03:10 PM, Stephen Smalley wrote:
>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>> A NULL pointer dereference was observed as following panic:
>>>
>>> BUG: unable to handle kernel NULL pointer dereference at (null)
>>> IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>>> ...
>>> Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
>>> task ffff88022ac70e40)
>>> Stack:
>>> ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
>>> ffffffff8125818e
>>> 0000000000000001 0000000037f78004 ffff880237f2bcd8
>>> ffffffff81273619
>>> ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
>>> ffffffff8125c206
>>> Call Trace:
>>> [<ffffffff8125818e>] ? ipcperms+0xae/0x110
>>> [<ffffffff81273619>] selinux_sem_semop+0x19/0x20
>>> [<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
>>> [<ffffffff8125c206>] sys_semtimedop+0x346/0x750
>>> [<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>> [<ffffffff8109a763>] ? up_read+0x23/0x40
>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>> [<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
>>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>>> [<ffffffff81622f45>] ? sysret_check+0x22/0x5d
>>> [<ffffffff8125c620>] sys_semop+0x10/0x20
>>> [<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
>>> Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
>>> 45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04
>>> 89 55 d8
>>> <0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
>>> RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>>> RSP <ffff880237f2bc98>
>>> CR2: 0000000000000000
>>>
>>> The root cause is semtimedop() was called after semget() without
>>> checking its
>>> return value in process opcmon. and semget() failed to check
>>> permission in
>>> function avc_has_perm() then sem_perm->security was freed shown as
>>> following:
>>>
>>> sys_semget()
>>> ->newary()
>>> ->security_sem_alloc()
>>> ->sem_alloc_security()
>>> selinux_sem_alloc_security()
>>> ->ipc_alloc_security() {
>>> ->rc = avc_has_perm()
>>> if (rc) {
>>> ipc_free_security(&sma->sem_perm);
>>> return rc;
>> We free the security structure here to avoid a memory leak on a
>> failed/denied semaphore set creation. In this situation, we return an
>> error to the caller (ultimately to newary), it does an
>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>> created. So how then can you call semtimedop() on it?
> My only idea would be a race of semtimedop() with IPC_RMID:
> If a rcu grace period happens between sem_obtain_object_check() and the
> ipc_has_perm() call, the the observed NULL pointer assignment would happen.
We only free and clear the ipc_perms->security field on a failure during
newary() -> security_sem_alloc(), in which case we fail with an error
before the ipc_addid() call has occurred, or during sem_rcu_free() ->
security_sem_free() just prior to calling ipc_rcu_free(). So I don't
see how ipc_perms->security can be NULL in ipc_has_perm(). We could rcu
free the ipc_perms->security field but I don't see why that would be
correct/necessary.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-20 21:01 ` Stephen Smalley
@ 2015-01-20 21:06 ` Eric Paris
0 siblings, 0 replies; 35+ messages in thread
From: Eric Paris @ 2015-01-20 21:06 UTC (permalink / raw)
To: Stephen Smalley
Cc: Manfred Spraul, Ethan Zhao, james.l.morris, serge, paul,
ethan.kernel, linux-security-module, linux-kernel, selinux
What kernel version was this? Didn't we have this problem and solve it
upstream some time ago? IPC could be allocated with a valid security
context, the ipc would be freed. the isec was free'd syncronously, but
then the ipc could stick around until some rcu period or some usage flag
got to 0, then it got freed...
this seems so familiar, but it was a while ago
On Tue, 2015-01-20 at 16:01 -0500, Stephen Smalley wrote:
> On 01/20/2015 01:49 PM, Manfred Spraul wrote:
> > Hi,
> >
> > On 01/20/2015 03:10 PM, Stephen Smalley wrote:
> >> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
> >>> A NULL pointer dereference was observed as following panic:
> >>>
> >>> BUG: unable to handle kernel NULL pointer dereference at (null)
> >>> IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
> >>> ...
> >>> Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
> >>> task ffff88022ac70e40)
> >>> Stack:
> >>> ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
> >>> ffffffff8125818e
> >>> 0000000000000001 0000000037f78004 ffff880237f2bcd8
> >>> ffffffff81273619
> >>> ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
> >>> ffffffff8125c206
> >>> Call Trace:
> >>> [<ffffffff8125818e>] ? ipcperms+0xae/0x110
> >>> [<ffffffff81273619>] selinux_sem_semop+0x19/0x20
> >>> [<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
> >>> [<ffffffff8125c206>] sys_semtimedop+0x346/0x750
> >>> [<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
> >>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
> >>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
> >>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
> >>> [<ffffffff8109a763>] ? up_read+0x23/0x40
> >>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
> >>> [<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
> >>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
> >>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
> >>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
> >>> [<ffffffff81622f45>] ? sysret_check+0x22/0x5d
> >>> [<ffffffff8125c620>] sys_semop+0x10/0x20
> >>> [<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
> >>> Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
> >>> 45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04
> >>> 89 55 d8
> >>> <0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
> >>> RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
> >>> RSP <ffff880237f2bc98>
> >>> CR2: 0000000000000000
> >>>
> >>> The root cause is semtimedop() was called after semget() without
> >>> checking its
> >>> return value in process opcmon. and semget() failed to check
> >>> permission in
> >>> function avc_has_perm() then sem_perm->security was freed shown as
> >>> following:
> >>>
> >>> sys_semget()
> >>> ->newary()
> >>> ->security_sem_alloc()
> >>> ->sem_alloc_security()
> >>> selinux_sem_alloc_security()
> >>> ->ipc_alloc_security() {
> >>> ->rc = avc_has_perm()
> >>> if (rc) {
> >>> ipc_free_security(&sma->sem_perm);
> >>> return rc;
> >> We free the security structure here to avoid a memory leak on a
> >> failed/denied semaphore set creation. In this situation, we return an
> >> error to the caller (ultimately to newary), it does an
> >> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
> >> caller. Thus, it never calls ipc_addid() and the semaphore set is not
> >> created. So how then can you call semtimedop() on it?
> > My only idea would be a race of semtimedop() with IPC_RMID:
> > If a rcu grace period happens between sem_obtain_object_check() and the
> > ipc_has_perm() call, the the observed NULL pointer assignment would happen.
>
> We only free and clear the ipc_perms->security field on a failure during
> newary() -> security_sem_alloc(), in which case we fail with an error
> before the ipc_addid() call has occurred, or during sem_rcu_free() ->
> security_sem_free() just prior to calling ipc_rcu_free(). So I don't
> see how ipc_perms->security can be NULL in ipc_has_perm(). We could rcu
> free the ipc_perms->security field but I don't see why that would be
> correct/necessary.
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-20 21:06 ` Eric Paris
0 siblings, 0 replies; 35+ messages in thread
From: Eric Paris @ 2015-01-20 21:06 UTC (permalink / raw)
To: Stephen Smalley
Cc: ethan.kernel, Ethan Zhao, Manfred Spraul, linux-kernel,
linux-security-module, james.l.morris, selinux
What kernel version was this? Didn't we have this problem and solve it
upstream some time ago? IPC could be allocated with a valid security
context, the ipc would be freed. the isec was free'd syncronously, but
then the ipc could stick around until some rcu period or some usage flag
got to 0, then it got freed...
this seems so familiar, but it was a while ago
On Tue, 2015-01-20 at 16:01 -0500, Stephen Smalley wrote:
> On 01/20/2015 01:49 PM, Manfred Spraul wrote:
> > Hi,
> >
> > On 01/20/2015 03:10 PM, Stephen Smalley wrote:
> >> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
> >>> A NULL pointer dereference was observed as following panic:
> >>>
> >>> BUG: unable to handle kernel NULL pointer dereference at (null)
> >>> IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
> >>> ...
> >>> Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
> >>> task ffff88022ac70e40)
> >>> Stack:
> >>> ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
> >>> ffffffff8125818e
> >>> 0000000000000001 0000000037f78004 ffff880237f2bcd8
> >>> ffffffff81273619
> >>> ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
> >>> ffffffff8125c206
> >>> Call Trace:
> >>> [<ffffffff8125818e>] ? ipcperms+0xae/0x110
> >>> [<ffffffff81273619>] selinux_sem_semop+0x19/0x20
> >>> [<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
> >>> [<ffffffff8125c206>] sys_semtimedop+0x346/0x750
> >>> [<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
> >>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
> >>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
> >>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
> >>> [<ffffffff8109a763>] ? up_read+0x23/0x40
> >>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
> >>> [<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
> >>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
> >>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
> >>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
> >>> [<ffffffff81622f45>] ? sysret_check+0x22/0x5d
> >>> [<ffffffff8125c620>] sys_semop+0x10/0x20
> >>> [<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
> >>> Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
> >>> 45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04
> >>> 89 55 d8
> >>> <0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
> >>> RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
> >>> RSP <ffff880237f2bc98>
> >>> CR2: 0000000000000000
> >>>
> >>> The root cause is semtimedop() was called after semget() without
> >>> checking its
> >>> return value in process opcmon. and semget() failed to check
> >>> permission in
> >>> function avc_has_perm() then sem_perm->security was freed shown as
> >>> following:
> >>>
> >>> sys_semget()
> >>> ->newary()
> >>> ->security_sem_alloc()
> >>> ->sem_alloc_security()
> >>> selinux_sem_alloc_security()
> >>> ->ipc_alloc_security() {
> >>> ->rc = avc_has_perm()
> >>> if (rc) {
> >>> ipc_free_security(&sma->sem_perm);
> >>> return rc;
> >> We free the security structure here to avoid a memory leak on a
> >> failed/denied semaphore set creation. In this situation, we return an
> >> error to the caller (ultimately to newary), it does an
> >> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
> >> caller. Thus, it never calls ipc_addid() and the semaphore set is not
> >> created. So how then can you call semtimedop() on it?
> > My only idea would be a race of semtimedop() with IPC_RMID:
> > If a rcu grace period happens between sem_obtain_object_check() and the
> > ipc_has_perm() call, the the observed NULL pointer assignment would happen.
>
> We only free and clear the ipc_perms->security field on a failure during
> newary() -> security_sem_alloc(), in which case we fail with an error
> before the ipc_addid() call has occurred, or during sem_rcu_free() ->
> security_sem_free() just prior to calling ipc_rcu_free(). So I don't
> see how ipc_perms->security can be NULL in ipc_has_perm(). We could rcu
> free the ipc_perms->security field but I don't see why that would be
> correct/necessary.
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-20 21:06 ` Eric Paris
@ 2015-01-20 21:09 ` Stephen Smalley
-1 siblings, 0 replies; 35+ messages in thread
From: Stephen Smalley @ 2015-01-20 21:09 UTC (permalink / raw)
To: Eric Paris
Cc: Manfred Spraul, Ethan Zhao, james.l.morris, serge, paul,
ethan.kernel, linux-security-module, linux-kernel, selinux
On 01/20/2015 04:06 PM, Eric Paris wrote:
> What kernel version was this? Didn't we have this problem and solve it
> upstream some time ago? IPC could be allocated with a valid security
> context, the ipc would be freed. the isec was free'd syncronously, but
> then the ipc could stick around until some rcu period or some usage flag
> got to 0, then it got freed...
>
> this seems so familiar, but it was a while ago
Don't know the kernel version.
Are you thinking of the inode security bug? Because in that case the
vfs changed underneath us to rcu free the inode but left the
security_inode_free() call outside of the rcu callback (due to the rcu
callback only being used in the default case where the filesystem does
not implement its own destroy_inode() method). In this case, we don't
seem to have that situation AFAICS.
>
> On Tue, 2015-01-20 at 16:01 -0500, Stephen Smalley wrote:
>> On 01/20/2015 01:49 PM, Manfred Spraul wrote:
>>> Hi,
>>>
>>> On 01/20/2015 03:10 PM, Stephen Smalley wrote:
>>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>>>> A NULL pointer dereference was observed as following panic:
>>>>>
>>>>> BUG: unable to handle kernel NULL pointer dereference at (null)
>>>>> IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>>>>> ...
>>>>> Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
>>>>> task ffff88022ac70e40)
>>>>> Stack:
>>>>> ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
>>>>> ffffffff8125818e
>>>>> 0000000000000001 0000000037f78004 ffff880237f2bcd8
>>>>> ffffffff81273619
>>>>> ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
>>>>> ffffffff8125c206
>>>>> Call Trace:
>>>>> [<ffffffff8125818e>] ? ipcperms+0xae/0x110
>>>>> [<ffffffff81273619>] selinux_sem_semop+0x19/0x20
>>>>> [<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
>>>>> [<ffffffff8125c206>] sys_semtimedop+0x346/0x750
>>>>> [<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
>>>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>>>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>>>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>>>> [<ffffffff8109a763>] ? up_read+0x23/0x40
>>>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>>>> [<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
>>>>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>>>>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>>>>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>>>>> [<ffffffff81622f45>] ? sysret_check+0x22/0x5d
>>>>> [<ffffffff8125c620>] sys_semop+0x10/0x20
>>>>> [<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
>>>>> Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
>>>>> 45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04
>>>>> 89 55 d8
>>>>> <0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
>>>>> RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>>>>> RSP <ffff880237f2bc98>
>>>>> CR2: 0000000000000000
>>>>>
>>>>> The root cause is semtimedop() was called after semget() without
>>>>> checking its
>>>>> return value in process opcmon. and semget() failed to check
>>>>> permission in
>>>>> function avc_has_perm() then sem_perm->security was freed shown as
>>>>> following:
>>>>>
>>>>> sys_semget()
>>>>> ->newary()
>>>>> ->security_sem_alloc()
>>>>> ->sem_alloc_security()
>>>>> selinux_sem_alloc_security()
>>>>> ->ipc_alloc_security() {
>>>>> ->rc = avc_has_perm()
>>>>> if (rc) {
>>>>> ipc_free_security(&sma->sem_perm);
>>>>> return rc;
>>>> We free the security structure here to avoid a memory leak on a
>>>> failed/denied semaphore set creation. In this situation, we return an
>>>> error to the caller (ultimately to newary), it does an
>>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>>>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>>>> created. So how then can you call semtimedop() on it?
>>> My only idea would be a race of semtimedop() with IPC_RMID:
>>> If a rcu grace period happens between sem_obtain_object_check() and the
>>> ipc_has_perm() call, the the observed NULL pointer assignment would happen.
>>
>> We only free and clear the ipc_perms->security field on a failure during
>> newary() -> security_sem_alloc(), in which case we fail with an error
>> before the ipc_addid() call has occurred, or during sem_rcu_free() ->
>> security_sem_free() just prior to calling ipc_rcu_free(). So I don't
>> see how ipc_perms->security can be NULL in ipc_has_perm(). We could rcu
>> free the ipc_perms->security field but I don't see why that would be
>> correct/necessary.
>>
>
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-20 21:09 ` Stephen Smalley
0 siblings, 0 replies; 35+ messages in thread
From: Stephen Smalley @ 2015-01-20 21:09 UTC (permalink / raw)
To: Eric Paris
Cc: Ethan Zhao, Manfred Spraul, linux-kernel, linux-security-module,
james.l.morris, ethan.kernel, selinux
On 01/20/2015 04:06 PM, Eric Paris wrote:
> What kernel version was this? Didn't we have this problem and solve it
> upstream some time ago? IPC could be allocated with a valid security
> context, the ipc would be freed. the isec was free'd syncronously, but
> then the ipc could stick around until some rcu period or some usage flag
> got to 0, then it got freed...
>
> this seems so familiar, but it was a while ago
Don't know the kernel version.
Are you thinking of the inode security bug? Because in that case the
vfs changed underneath us to rcu free the inode but left the
security_inode_free() call outside of the rcu callback (due to the rcu
callback only being used in the default case where the filesystem does
not implement its own destroy_inode() method). In this case, we don't
seem to have that situation AFAICS.
>
> On Tue, 2015-01-20 at 16:01 -0500, Stephen Smalley wrote:
>> On 01/20/2015 01:49 PM, Manfred Spraul wrote:
>>> Hi,
>>>
>>> On 01/20/2015 03:10 PM, Stephen Smalley wrote:
>>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>>>> A NULL pointer dereference was observed as following panic:
>>>>>
>>>>> BUG: unable to handle kernel NULL pointer dereference at (null)
>>>>> IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>>>>> ...
>>>>> Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
>>>>> task ffff88022ac70e40)
>>>>> Stack:
>>>>> ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
>>>>> ffffffff8125818e
>>>>> 0000000000000001 0000000037f78004 ffff880237f2bcd8
>>>>> ffffffff81273619
>>>>> ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
>>>>> ffffffff8125c206
>>>>> Call Trace:
>>>>> [<ffffffff8125818e>] ? ipcperms+0xae/0x110
>>>>> [<ffffffff81273619>] selinux_sem_semop+0x19/0x20
>>>>> [<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
>>>>> [<ffffffff8125c206>] sys_semtimedop+0x346/0x750
>>>>> [<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
>>>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>>>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>>>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>>>> [<ffffffff8109a763>] ? up_read+0x23/0x40
>>>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>>>> [<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
>>>>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>>>>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>>>>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>>>>> [<ffffffff81622f45>] ? sysret_check+0x22/0x5d
>>>>> [<ffffffff8125c620>] sys_semop+0x10/0x20
>>>>> [<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
>>>>> Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
>>>>> 45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04
>>>>> 89 55 d8
>>>>> <0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
>>>>> RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>>>>> RSP <ffff880237f2bc98>
>>>>> CR2: 0000000000000000
>>>>>
>>>>> The root cause is semtimedop() was called after semget() without
>>>>> checking its
>>>>> return value in process opcmon. and semget() failed to check
>>>>> permission in
>>>>> function avc_has_perm() then sem_perm->security was freed shown as
>>>>> following:
>>>>>
>>>>> sys_semget()
>>>>> ->newary()
>>>>> ->security_sem_alloc()
>>>>> ->sem_alloc_security()
>>>>> selinux_sem_alloc_security()
>>>>> ->ipc_alloc_security() {
>>>>> ->rc = avc_has_perm()
>>>>> if (rc) {
>>>>> ipc_free_security(&sma->sem_perm);
>>>>> return rc;
>>>> We free the security structure here to avoid a memory leak on a
>>>> failed/denied semaphore set creation. In this situation, we return an
>>>> error to the caller (ultimately to newary), it does an
>>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>>>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>>>> created. So how then can you call semtimedop() on it?
>>> My only idea would be a race of semtimedop() with IPC_RMID:
>>> If a rcu grace period happens between sem_obtain_object_check() and the
>>> ipc_has_perm() call, the the observed NULL pointer assignment would happen.
>>
>> We only free and clear the ipc_perms->security field on a failure during
>> newary() -> security_sem_alloc(), in which case we fail with an error
>> before the ipc_addid() call has occurred, or during sem_rcu_free() ->
>> security_sem_free() just prior to calling ipc_rcu_free(). So I don't
>> see how ipc_perms->security can be NULL in ipc_has_perm(). We could rcu
>> free the ipc_perms->security field but I don't see why that would be
>> correct/necessary.
>>
>
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-20 18:49 ` Manfred Spraul
@ 2015-01-21 1:30 ` ethan zhao
-1 siblings, 0 replies; 35+ messages in thread
From: ethan zhao @ 2015-01-21 1:30 UTC (permalink / raw)
To: Manfred Spraul
Cc: Stephen Smalley, james.l.morris, serge, eparis, paul, selinux,
linux-security-module, linux-kernel, ethan.kernel
Stephen,
On 2015/1/21 2:49, Manfred Spraul wrote:
> Hi,
>
> On 01/20/2015 03:10 PM, Stephen Smalley wrote:
>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>> A NULL pointer dereference was observed as following panic:
>>>
>>> BUG: unable to handle kernel NULL pointer dereference at (null)
>>> IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>>> ...
>>> Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
>>> task ffff88022ac70e40)
>>> Stack:
>>> ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
>>> ffffffff8125818e
>>> 0000000000000001 0000000037f78004 ffff880237f2bcd8
>>> ffffffff81273619
>>> ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
>>> ffffffff8125c206
>>> Call Trace:
>>> [<ffffffff8125818e>] ? ipcperms+0xae/0x110
>>> [<ffffffff81273619>] selinux_sem_semop+0x19/0x20
>>> [<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
>>> [<ffffffff8125c206>] sys_semtimedop+0x346/0x750
>>> [<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>> [<ffffffff8109a763>] ? up_read+0x23/0x40
>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>> [<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
>>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>>> [<ffffffff81622f45>] ? sysret_check+0x22/0x5d
>>> [<ffffffff8125c620>] sys_semop+0x10/0x20
>>> [<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
>>> Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
>>> 45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04
>>> 89 55 d8
>>> <0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
>>> RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>>> RSP <ffff880237f2bc98>
>>> CR2: 0000000000000000
>>>
>>> The root cause is semtimedop() was called after semget() without
>>> checking its
>>> return value in process opcmon. and semget() failed to check
>>> permission in
>>> function avc_has_perm() then sem_perm->security was freed shown as
>>> following:
>>>
>>> sys_semget()
>>> ->newary()
>>> ->security_sem_alloc()
>>> ->sem_alloc_security()
>>> selinux_sem_alloc_security()
>>> ->ipc_alloc_security() {
>>> ->rc = avc_has_perm()
>>> if (rc) {
>>> ipc_free_security(&sma->sem_perm);
>>> return rc;
>> We free the security structure here to avoid a memory leak on a
>> failed/denied semaphore set creation. In this situation, we return an
>> error to the caller (ultimately to newary), it does an
>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>> created. So how then can you call semtimedop() on it?
> My only idea would be a race of semtimedop() with IPC_RMID:
> If a rcu grace period happens between sem_obtain_object_check() and
> the ipc_has_perm() call, the the observed NULL pointer assignment
> would happen.
>
>>> So ipc_perms->security was NULL, then semtimedop() was called as
>>> following:
>>>
>>> sys_semtimedop() / semop()
>>> ->selinux_sem_semop()
>>> ->ipc_has_perm()
>>> ->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
>>> ^- NULL pointer
>>> dereference happens
>>>
>>> The test kernel was running on VMware.
> Can you give some more details?
> - which RCU type?
> - PREEMPT?
Let me figure out more detail about it and share with you.
Thanks,
Ethan
>
> --
> Manfred
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-21 1:30 ` ethan zhao
0 siblings, 0 replies; 35+ messages in thread
From: ethan zhao @ 2015-01-21 1:30 UTC (permalink / raw)
To: Manfred Spraul
Cc: ethan.kernel, james.l.morris, linux-kernel,
linux-security-module, selinux, Stephen Smalley
Stephen,
On 2015/1/21 2:49, Manfred Spraul wrote:
> Hi,
>
> On 01/20/2015 03:10 PM, Stephen Smalley wrote:
>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>> A NULL pointer dereference was observed as following panic:
>>>
>>> BUG: unable to handle kernel NULL pointer dereference at (null)
>>> IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>>> ...
>>> Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
>>> task ffff88022ac70e40)
>>> Stack:
>>> ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
>>> ffffffff8125818e
>>> 0000000000000001 0000000037f78004 ffff880237f2bcd8
>>> ffffffff81273619
>>> ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
>>> ffffffff8125c206
>>> Call Trace:
>>> [<ffffffff8125818e>] ? ipcperms+0xae/0x110
>>> [<ffffffff81273619>] selinux_sem_semop+0x19/0x20
>>> [<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
>>> [<ffffffff8125c206>] sys_semtimedop+0x346/0x750
>>> [<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>> [<ffffffff8109a763>] ? up_read+0x23/0x40
>>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>>> [<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
>>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>>> [<ffffffff81622f45>] ? sysret_check+0x22/0x5d
>>> [<ffffffff8125c620>] sys_semop+0x10/0x20
>>> [<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
>>> Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
>>> 45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04
>>> 89 55 d8
>>> <0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
>>> RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>>> RSP <ffff880237f2bc98>
>>> CR2: 0000000000000000
>>>
>>> The root cause is semtimedop() was called after semget() without
>>> checking its
>>> return value in process opcmon. and semget() failed to check
>>> permission in
>>> function avc_has_perm() then sem_perm->security was freed shown as
>>> following:
>>>
>>> sys_semget()
>>> ->newary()
>>> ->security_sem_alloc()
>>> ->sem_alloc_security()
>>> selinux_sem_alloc_security()
>>> ->ipc_alloc_security() {
>>> ->rc = avc_has_perm()
>>> if (rc) {
>>> ipc_free_security(&sma->sem_perm);
>>> return rc;
>> We free the security structure here to avoid a memory leak on a
>> failed/denied semaphore set creation. In this situation, we return an
>> error to the caller (ultimately to newary), it does an
>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>> created. So how then can you call semtimedop() on it?
> My only idea would be a race of semtimedop() with IPC_RMID:
> If a rcu grace period happens between sem_obtain_object_check() and
> the ipc_has_perm() call, the the observed NULL pointer assignment
> would happen.
>
>>> So ipc_perms->security was NULL, then semtimedop() was called as
>>> following:
>>>
>>> sys_semtimedop() / semop()
>>> ->selinux_sem_semop()
>>> ->ipc_has_perm()
>>> ->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
>>> ^- NULL pointer
>>> dereference happens
>>>
>>> The test kernel was running on VMware.
> Can you give some more details?
> - which RCU type?
> - PREEMPT?
Let me figure out more detail about it and share with you.
Thanks,
Ethan
>
> --
> Manfred
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-20 14:10 ` Stephen Smalley
@ 2015-01-21 3:53 ` Ethan Zhao
-1 siblings, 0 replies; 35+ messages in thread
From: Ethan Zhao @ 2015-01-21 3:53 UTC (permalink / raw)
To: Stephen Smalley
Cc: Ethan Zhao, james.l.morris, serge, eparis, paul, selinux,
linux-security-module, LKML, ethan.kernel, manfred
On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>> A NULL pointer dereference was observed as following panic:
>>
>> BUG: unable to handle kernel NULL pointer dereference at (null)
>> IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>> ...
>> Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
>> task ffff88022ac70e40)
>> Stack:
>> ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
>> ffffffff8125818e
>> 0000000000000001 0000000037f78004 ffff880237f2bcd8
>> ffffffff81273619
>> ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
>> ffffffff8125c206
>> Call Trace:
>> [<ffffffff8125818e>] ? ipcperms+0xae/0x110
>> [<ffffffff81273619>] selinux_sem_semop+0x19/0x20
>> [<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
>> [<ffffffff8125c206>] sys_semtimedop+0x346/0x750
>> [<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>> [<ffffffff8109a763>] ? up_read+0x23/0x40
>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>> [<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>> [<ffffffff81622f45>] ? sysret_check+0x22/0x5d
>> [<ffffffff8125c620>] sys_semop+0x10/0x20
>> [<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
>> Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
>> 45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04 89 55 d8
>> <0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
>> RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>> RSP <ffff880237f2bc98>
>> CR2: 0000000000000000
>>
>> The root cause is semtimedop() was called after semget() without checking its
>> return value in process opcmon. and semget() failed to check permission in
>> function avc_has_perm() then sem_perm->security was freed shown as following:
>>
>> sys_semget()
>> ->newary()
>> ->security_sem_alloc()
>> ->sem_alloc_security()
>> selinux_sem_alloc_security()
>> ->ipc_alloc_security() {
>> ->rc = avc_has_perm()
>> if (rc) {
>> ipc_free_security(&sma->sem_perm);
>> return rc;
>
> We free the security structure here to avoid a memory leak on a
> failed/denied semaphore set creation. In this situation, we return an
> error to the caller (ultimately to newary), it does an
> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
> caller. Thus, it never calls ipc_addid() and the semaphore set is not
> created. So how then can you call semtimedop() on it?
Seems it wouldn't happen after commit e8577d1f0329d4842e8302e289fb2c22156abef4 ?
Thanks,
Ethan
>
>> So ipc_perms->security was NULL, then semtimedop() was called as
>> following:
>>
>> sys_semtimedop() / semop()
>> ->selinux_sem_semop()
>> ->ipc_has_perm()
>> ->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
>> ^- NULL pointer dereference happens
>>
>> The test kernel was running on VMware.
>> This patch use to fix this serious security issue could be triggered by user space.
>> This patch was tested with v3.19-rc5.
>>
>> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
>> ---
>> security/selinux/hooks.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 6da7532..bbe76f5 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
>> u32 sid = current_sid();
>>
>> isec = ipc_perms->security;
>> + if (!isec)
>> + return -EACCES;
>>
>> ad.type = LSM_AUDIT_DATA_IPC;
>> ad.u.ipc_id = ipc_perms->key;
>>
>
> That is not the correct fix; it just hides a bug. If we reach
> ipc_has_perm() with a NULL isec, it is a bug in the ipc code.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-21 3:53 ` Ethan Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Ethan Zhao @ 2015-01-21 3:53 UTC (permalink / raw)
To: Stephen Smalley
Cc: ethan.kernel, Ethan Zhao, manfred, james.l.morris, LKML,
linux-security-module, selinux
On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>> A NULL pointer dereference was observed as following panic:
>>
>> BUG: unable to handle kernel NULL pointer dereference at (null)
>> IP: [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>> ...
>> Process opcmon (pid: 30712, threadinfo ffff880237f2a000,
>> task ffff88022ac70e40)
>> Stack:
>> ffff880237f2bc04 ffffffff01020953 ffff880237f2bce8
>> ffffffff8125818e
>> 0000000000000001 0000000037f78004 ffff880237f2bcd8
>> ffffffff81273619
>> ffff880237f2bce8 ffffffff8126e3e6 ffff880237f2bf68
>> ffffffff8125c206
>> Call Trace:
>> [<ffffffff8125818e>] ? ipcperms+0xae/0x110
>> [<ffffffff81273619>] selinux_sem_semop+0x19/0x20
>> [<ffffffff8126e3e6>] security_sem_semop+0x16/0x20
>> [<ffffffff8125c206>] sys_semtimedop+0x346/0x750
>> [<ffffffff81188c0c>] ? handle_pte_fault+0x1dc/0x200
>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>> [<ffffffff8109a763>] ? up_read+0x23/0x40
>> [<ffffffff8161d830>] ? __do_page_fault+0x280/0x500
>> [<ffffffff81182f1c>] ? might_fault+0x5c/0xb0
>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>> [<ffffffff810d97d0>] ? __lock_release+0x90/0x1b0
>> [<ffffffff81081f96>] ? sys_newuname+0x66/0xf0
>> [<ffffffff81622f45>] ? sysret_check+0x22/0x5d
>> [<ffffffff8125c620>] sys_semop+0x10/0x20
>> [<ffffffff81622f19>] system_call_fastpath+0x16/0x1b
>> Code: b8 00 00 48 8b 80 48 06 00 00 41 8b 54 24 40 4c 8d
>> 45 d0 89 d9 45 31 c9 48 8b 40 70 8b 78 04 49 8b 44 24 60 c6 45 d0 04 89 55 d8
>> <0f> b7 10 8b 70 04 e8 0a dc ff ff 48 83 c4 20 5b 41 5c c9 c3 90
>> RIP [<ffffffff812735eb>] ipc_has_perm+0x4b/0x60
>> RSP <ffff880237f2bc98>
>> CR2: 0000000000000000
>>
>> The root cause is semtimedop() was called after semget() without checking its
>> return value in process opcmon. and semget() failed to check permission in
>> function avc_has_perm() then sem_perm->security was freed shown as following:
>>
>> sys_semget()
>> ->newary()
>> ->security_sem_alloc()
>> ->sem_alloc_security()
>> selinux_sem_alloc_security()
>> ->ipc_alloc_security() {
>> ->rc = avc_has_perm()
>> if (rc) {
>> ipc_free_security(&sma->sem_perm);
>> return rc;
>
> We free the security structure here to avoid a memory leak on a
> failed/denied semaphore set creation. In this situation, we return an
> error to the caller (ultimately to newary), it does an
> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
> caller. Thus, it never calls ipc_addid() and the semaphore set is not
> created. So how then can you call semtimedop() on it?
Seems it wouldn't happen after commit e8577d1f0329d4842e8302e289fb2c22156abef4 ?
Thanks,
Ethan
>
>> So ipc_perms->security was NULL, then semtimedop() was called as
>> following:
>>
>> sys_semtimedop() / semop()
>> ->selinux_sem_semop()
>> ->ipc_has_perm()
>> ->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
>> ^- NULL pointer dereference happens
>>
>> The test kernel was running on VMware.
>> This patch use to fix this serious security issue could be triggered by user space.
>> This patch was tested with v3.19-rc5.
>>
>> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
>> ---
>> security/selinux/hooks.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 6da7532..bbe76f5 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
>> u32 sid = current_sid();
>>
>> isec = ipc_perms->security;
>> + if (!isec)
>> + return -EACCES;
>>
>> ad.type = LSM_AUDIT_DATA_IPC;
>> ad.u.ipc_id = ipc_perms->key;
>>
>
> That is not the correct fix; it just hides a bug. If we reach
> ipc_has_perm() with a NULL isec, it is a bug in the ipc code.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-21 3:53 ` Ethan Zhao
@ 2015-01-21 5:30 ` Manfred Spraul
-1 siblings, 0 replies; 35+ messages in thread
From: Manfred Spraul @ 2015-01-21 5:30 UTC (permalink / raw)
To: Ethan Zhao, Stephen Smalley
Cc: Ethan Zhao, james.l.morris, serge, eparis, paul, selinux,
linux-security-module, LKML, ethan.kernel
On 01/21/2015 04:53 AM, Ethan Zhao wrote:
> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>> sys_semget()
>>> ->newary()
>>> ->security_sem_alloc()
>>> ->sem_alloc_security()
>>> selinux_sem_alloc_security()
>>> ->ipc_alloc_security() {
>>> ->rc = avc_has_perm()
>>> if (rc) {
>>> ipc_free_security(&sma->sem_perm);
>>> return rc;
>> We free the security structure here to avoid a memory leak on a
>> failed/denied semaphore set creation. In this situation, we return an
>> error to the caller (ultimately to newary), it does an
>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>> created. So how then can you call semtimedop() on it?
> Seems it wouldn't happen after commit e8577d1f0329d4842e8302e289fb2c22156abef4 ?
That was my first guess when I read the bug report - but it can't be the
fix, because security_sem_alloc() is before the ipc_addid(), with or
without the patch.
thread A:
thread B:
semtimedop()
-> sem_obtain_object_check()
semctl(IPC_RMID)
-> freeary()
-> ipc_rcu_putref()
-> call_rcu()
-> somehow a grace period
-> sem_rcu_free()
-> security_sem_free()
Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes
if the pointer is NULL?
--
Manfred
> Thanks,
> Ethan
>>> So ipc_perms->security was NULL, then semtimedop() was called as
>>> following:
>>>
>>> sys_semtimedop() / semop()
>>> ->selinux_sem_semop()
>>> ->ipc_has_perm()
>>> ->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
>>> ^- NULL pointer dereference happens
>>>
>>> The test kernel was running on VMware.
>>> This patch use to fix this serious security issue could be triggered by user space.
>>> This patch was tested with v3.19-rc5.
>>>
>>> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
>>> ---
>>> security/selinux/hooks.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 6da7532..bbe76f5 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
>>> u32 sid = current_sid();
>>>
>>> isec = ipc_perms->security;
>>> + if (!isec)
>>> + return -EACCES;
>>>
>>> ad.type = LSM_AUDIT_DATA_IPC;
>>> ad.u.ipc_id = ipc_perms->key;
>>>
>> That is not the correct fix; it just hides a bug. If we reach
>> ipc_has_perm() with a NULL isec, it is a bug in the ipc code.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-21 5:30 ` Manfred Spraul
0 siblings, 0 replies; 35+ messages in thread
From: Manfred Spraul @ 2015-01-21 5:30 UTC (permalink / raw)
To: Ethan Zhao, Stephen Smalley
Cc: ethan.kernel, Ethan Zhao, james.l.morris, LKML,
linux-security-module, selinux
On 01/21/2015 04:53 AM, Ethan Zhao wrote:
> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>> sys_semget()
>>> ->newary()
>>> ->security_sem_alloc()
>>> ->sem_alloc_security()
>>> selinux_sem_alloc_security()
>>> ->ipc_alloc_security() {
>>> ->rc = avc_has_perm()
>>> if (rc) {
>>> ipc_free_security(&sma->sem_perm);
>>> return rc;
>> We free the security structure here to avoid a memory leak on a
>> failed/denied semaphore set creation. In this situation, we return an
>> error to the caller (ultimately to newary), it does an
>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>> created. So how then can you call semtimedop() on it?
> Seems it wouldn't happen after commit e8577d1f0329d4842e8302e289fb2c22156abef4 ?
That was my first guess when I read the bug report - but it can't be the
fix, because security_sem_alloc() is before the ipc_addid(), with or
without the patch.
thread A:
thread B:
semtimedop()
-> sem_obtain_object_check()
semctl(IPC_RMID)
-> freeary()
-> ipc_rcu_putref()
-> call_rcu()
-> somehow a grace period
-> sem_rcu_free()
-> security_sem_free()
Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes
if the pointer is NULL?
--
Manfred
> Thanks,
> Ethan
>>> So ipc_perms->security was NULL, then semtimedop() was called as
>>> following:
>>>
>>> sys_semtimedop() / semop()
>>> ->selinux_sem_semop()
>>> ->ipc_has_perm()
>>> ->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
>>> ^- NULL pointer dereference happens
>>>
>>> The test kernel was running on VMware.
>>> This patch use to fix this serious security issue could be triggered by user space.
>>> This patch was tested with v3.19-rc5.
>>>
>>> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
>>> ---
>>> security/selinux/hooks.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 6da7532..bbe76f5 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
>>> u32 sid = current_sid();
>>>
>>> isec = ipc_perms->security;
>>> + if (!isec)
>>> + return -EACCES;
>>>
>>> ad.type = LSM_AUDIT_DATA_IPC;
>>> ad.u.ipc_id = ipc_perms->key;
>>>
>> That is not the correct fix; it just hides a bug. If we reach
>> ipc_has_perm() with a NULL isec, it is a bug in the ipc code.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-21 5:30 ` Manfred Spraul
@ 2015-01-22 2:44 ` Ethan Zhao
-1 siblings, 0 replies; 35+ messages in thread
From: Ethan Zhao @ 2015-01-22 2:44 UTC (permalink / raw)
To: Manfred Spraul
Cc: Stephen Smalley, Ethan Zhao, james.l.morris, serge, Eric Paris,
Paul Moore, selinux, linux-security-module, LKML, ethan.kernel
On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
<manfred@colorfullife.com> wrote:
> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
>>
>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
>> wrote:
>>>
>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>>>
>>>> sys_semget()
>>>> ->newary()
>>>> ->security_sem_alloc()
>>>> ->sem_alloc_security()
>>>> selinux_sem_alloc_security()
>>>> ->ipc_alloc_security() {
>>>> ->rc = avc_has_perm()
>>>> if (rc) {
>>>>
>>>> ipc_free_security(&sma->sem_perm);
>>>> return rc;
>>>
>>> We free the security structure here to avoid a memory leak on a
>>> failed/denied semaphore set creation. In this situation, we return an
>>> error to the caller (ultimately to newary), it does an
>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>>> created. So how then can you call semtimedop() on it?
>>
>> Seems it wouldn't happen after commit
>> e8577d1f0329d4842e8302e289fb2c22156abef4 ?
>
> That was my first guess when I read the bug report - but it can't be the
> fix, because security_sem_alloc() is before the ipc_addid(), with or without
> the patch.
>
> thread A:
> thread B:
>
> semtimedop()
> -> sem_obtain_object_check()
> semctl(IPC_RMID)
> -> freeary()
> -> ipc_rcu_putref()
> -> call_rcu()
> -> somehow a grace period
> -> sem_rcu_free()
> -> security_sem_free()
>
> Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes if
> the pointer is NULL?
I tried to ask for vmcore and do more analysis, basically, the race condition
still exists and open a hole to be DoS.
Thanks,
Ethan
>
> --
> Manfred
>
>
>> Thanks,
>> Ethan
>>>>
>>>> So ipc_perms->security was NULL, then semtimedop() was called as
>>>> following:
>>>>
>>>> sys_semtimedop() / semop()
>>>> ->selinux_sem_semop()
>>>> ->ipc_has_perm()
>>>> ->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
>>>> ^- NULL pointer dereference
>>>> happens
>>>>
>>>> The test kernel was running on VMware.
>>>> This patch use to fix this serious security issue could be triggered by
>>>> user space.
>>>> This patch was tested with v3.19-rc5.
>>>>
>>>> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
>>>> ---
>>>> security/selinux/hooks.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>> index 6da7532..bbe76f5 100644
>>>> --- a/security/selinux/hooks.c
>>>> +++ b/security/selinux/hooks.c
>>>> @@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm
>>>> *ipc_perms,
>>>> u32 sid = current_sid();
>>>>
>>>> isec = ipc_perms->security;
>>>> + if (!isec)
>>>> + return -EACCES;
>>>>
>>>> ad.type = LSM_AUDIT_DATA_IPC;
>>>> ad.u.ipc_id = ipc_perms->key;
>>>>
>>> That is not the correct fix; it just hides a bug. If we reach
>>> ipc_has_perm() with a NULL isec, it is a bug in the ipc code.
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>>> in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-22 2:44 ` Ethan Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Ethan Zhao @ 2015-01-22 2:44 UTC (permalink / raw)
To: Manfred Spraul
Cc: ethan.kernel, Ethan Zhao, LKML, linux-security-module,
james.l.morris, Stephen Smalley, selinux
On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
<manfred@colorfullife.com> wrote:
> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
>>
>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
>> wrote:
>>>
>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>>>
>>>> sys_semget()
>>>> ->newary()
>>>> ->security_sem_alloc()
>>>> ->sem_alloc_security()
>>>> selinux_sem_alloc_security()
>>>> ->ipc_alloc_security() {
>>>> ->rc = avc_has_perm()
>>>> if (rc) {
>>>>
>>>> ipc_free_security(&sma->sem_perm);
>>>> return rc;
>>>
>>> We free the security structure here to avoid a memory leak on a
>>> failed/denied semaphore set creation. In this situation, we return an
>>> error to the caller (ultimately to newary), it does an
>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>>> created. So how then can you call semtimedop() on it?
>>
>> Seems it wouldn't happen after commit
>> e8577d1f0329d4842e8302e289fb2c22156abef4 ?
>
> That was my first guess when I read the bug report - but it can't be the
> fix, because security_sem_alloc() is before the ipc_addid(), with or without
> the patch.
>
> thread A:
> thread B:
>
> semtimedop()
> -> sem_obtain_object_check()
> semctl(IPC_RMID)
> -> freeary()
> -> ipc_rcu_putref()
> -> call_rcu()
> -> somehow a grace period
> -> sem_rcu_free()
> -> security_sem_free()
>
> Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes if
> the pointer is NULL?
I tried to ask for vmcore and do more analysis, basically, the race condition
still exists and open a hole to be DoS.
Thanks,
Ethan
>
> --
> Manfred
>
>
>> Thanks,
>> Ethan
>>>>
>>>> So ipc_perms->security was NULL, then semtimedop() was called as
>>>> following:
>>>>
>>>> sys_semtimedop() / semop()
>>>> ->selinux_sem_semop()
>>>> ->ipc_has_perm()
>>>> ->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
>>>> ^- NULL pointer dereference
>>>> happens
>>>>
>>>> The test kernel was running on VMware.
>>>> This patch use to fix this serious security issue could be triggered by
>>>> user space.
>>>> This patch was tested with v3.19-rc5.
>>>>
>>>> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
>>>> ---
>>>> security/selinux/hooks.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>> index 6da7532..bbe76f5 100644
>>>> --- a/security/selinux/hooks.c
>>>> +++ b/security/selinux/hooks.c
>>>> @@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm
>>>> *ipc_perms,
>>>> u32 sid = current_sid();
>>>>
>>>> isec = ipc_perms->security;
>>>> + if (!isec)
>>>> + return -EACCES;
>>>>
>>>> ad.type = LSM_AUDIT_DATA_IPC;
>>>> ad.u.ipc_id = ipc_perms->key;
>>>>
>>> That is not the correct fix; it just hides a bug. If we reach
>>> ipc_has_perm() with a NULL isec, it is a bug in the ipc code.
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>>> in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-22 2:44 ` Ethan Zhao
@ 2015-01-22 18:15 ` Manfred Spraul
-1 siblings, 0 replies; 35+ messages in thread
From: Manfred Spraul @ 2015-01-22 18:15 UTC (permalink / raw)
To: Ethan Zhao
Cc: Stephen Smalley, Ethan Zhao, james.l.morris, serge, Eric Paris,
Paul Moore, selinux, linux-security-module, LKML
[-- Attachment #1: Type: text/plain, Size: 2726 bytes --]
On 01/22/2015 03:44 AM, Ethan Zhao wrote:
> On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
> <manfred@colorfullife.com> wrote:
>> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
>>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
>>> wrote:
>>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>>>> sys_semget()
>>>>> ->newary()
>>>>> ->security_sem_alloc()
>>>>> ->sem_alloc_security()
>>>>> selinux_sem_alloc_security()
>>>>> ->ipc_alloc_security() {
>>>>> ->rc = avc_has_perm()
>>>>> if (rc) {
>>>>>
>>>>> ipc_free_security(&sma->sem_perm);
>>>>> return rc;
>>>> We free the security structure here to avoid a memory leak on a
>>>> failed/denied semaphore set creation. In this situation, we return an
>>>> error to the caller (ultimately to newary), it does an
>>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>>>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>>>> created. So how then can you call semtimedop() on it?
>>> Seems it wouldn't happen after commit
>>> e8577d1f0329d4842e8302e289fb2c22156abef4 ?
>> That was my first guess when I read the bug report - but it can't be the
>> fix, because security_sem_alloc() is before the ipc_addid(), with or without
>> the patch.
>>
>> thread A:
>> thread B:
>>
>> semtimedop()
>> -> sem_obtain_object_check()
>> semctl(IPC_RMID)
>> -> freeary()
>> -> ipc_rcu_putref()
>> -> call_rcu()
>> -> somehow a grace period
>> -> sem_rcu_free()
>> -> security_sem_free()
>>
>> Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes if
>> the pointer is NULL?
> I tried to ask for vmcore and do more analysis, basically, the race condition
> still exists and open a hole to be DoS.
Is the issue reproducable?
If yes, can you try something like the attached patch?
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm
> *ipc_perms,
> u32 sid = current_sid();
>
> isec = ipc_perms->security;
> + if (!isec)
> + return -EACCES;
>
> ad.type = LSM_AUDIT_DATA_IPC;
> ad.u.ipc_id = ipc_perms->key;
ipc_has_perm() runs without any spinlocks/semaphores, only rcu_read_lock().
Testing for ipc_perms->security!=NULL does solve the issue that
ipc_perm->key could be an access to kfree'd memory: Nothing prevents
that the kfree could happen just after the test.
I.e.: The patch can't be the right solution.
--
Manfred
[-- Attachment #2: patch-sem_ipc_has_perm --]
[-- Type: text/plain, Size: 2351 bytes --]
diff --git a/ipc/sem.c b/ipc/sem.c
index 6115146..80371dc 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -248,6 +248,7 @@ static void sem_rcu_free(struct rcu_head *head)
struct ipc_rcu *p = container_of(head, struct ipc_rcu, rcu);
struct sem_array *sma = ipc_rcu_to_struct(p);
+pr_info("sem_rcu_free: sma %p\n",sma);
security_sem_free(sma);
ipc_rcu_free(head);
}
@@ -529,6 +530,7 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
sma->sem_nsems = nsems;
sma->sem_ctime = get_seconds();
+pr_info("newary: sma %p becomes visible\n",sma);
id = ipc_addid(&sem_ids(ns), &sma->sem_perm, ns->sc_semmni);
if (id < 0) {
ipc_rcu_putref(sma, sem_rcu_free);
@@ -1118,6 +1120,7 @@ static void freeary(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp)
/* Remove the semaphore set from the IDR */
sem_rmid(ns, sma);
+pr_info("freeary: sma %p unlinked\n",sma);
sem_unlock(sma, -1);
rcu_read_unlock();
@@ -1860,6 +1863,9 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
if (ipcperms(ns, &sma->sem_perm, alter ? S_IWUGO : S_IRUGO))
goto out_rcu_wakeup;
+ if (sma->sem_perm.security == NULL) {
+ pr_info("sma %p: sem_perm.security == NULL\n", sma);
+ }
error = security_sem_semop(sma, sops, nsops, alter);
if (error)
goto out_rcu_wakeup;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6da7532..1499787 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5088,6 +5088,7 @@ static int ipc_alloc_security(struct task_struct *task,
isec->sclass = sclass;
isec->sid = sid;
perm->security = isec;
+pr_info("ipc_alloc_security for perm %p.\n", perm);
return 0;
}
@@ -5096,6 +5097,7 @@ static void ipc_free_security(struct kern_ipc_perm *perm)
{
struct ipc_security_struct *isec = perm->security;
perm->security = NULL;
+pr_info("ipc_free_security for perm %p.\n", perm);
kfree(isec);
}
@@ -5129,6 +5131,12 @@ static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
u32 sid = current_sid();
isec = ipc_perms->security;
+ if (isec == NULL) {
+ struct sem_array *sma = container_of(ipc_perms, struct sem_array, sem_perm);
+
+ pr_info("sma %p, sem_base %p, deleted %d with NULL isec\n",
+ sma, sma->sem_base, sma->sem_perm.deleted);
+ }
ad.type = LSM_AUDIT_DATA_IPC;
ad.u.ipc_id = ipc_perms->key;
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-22 18:15 ` Manfred Spraul
0 siblings, 0 replies; 35+ messages in thread
From: Manfred Spraul @ 2015-01-22 18:15 UTC (permalink / raw)
To: Ethan Zhao
Cc: Ethan Zhao, LKML, linux-security-module, james.l.morris,
Stephen Smalley, selinux
[-- Attachment #1: Type: text/plain, Size: 2726 bytes --]
On 01/22/2015 03:44 AM, Ethan Zhao wrote:
> On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
> <manfred@colorfullife.com> wrote:
>> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
>>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
>>> wrote:
>>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>>>> sys_semget()
>>>>> ->newary()
>>>>> ->security_sem_alloc()
>>>>> ->sem_alloc_security()
>>>>> selinux_sem_alloc_security()
>>>>> ->ipc_alloc_security() {
>>>>> ->rc = avc_has_perm()
>>>>> if (rc) {
>>>>>
>>>>> ipc_free_security(&sma->sem_perm);
>>>>> return rc;
>>>> We free the security structure here to avoid a memory leak on a
>>>> failed/denied semaphore set creation. In this situation, we return an
>>>> error to the caller (ultimately to newary), it does an
>>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>>>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>>>> created. So how then can you call semtimedop() on it?
>>> Seems it wouldn't happen after commit
>>> e8577d1f0329d4842e8302e289fb2c22156abef4 ?
>> That was my first guess when I read the bug report - but it can't be the
>> fix, because security_sem_alloc() is before the ipc_addid(), with or without
>> the patch.
>>
>> thread A:
>> thread B:
>>
>> semtimedop()
>> -> sem_obtain_object_check()
>> semctl(IPC_RMID)
>> -> freeary()
>> -> ipc_rcu_putref()
>> -> call_rcu()
>> -> somehow a grace period
>> -> sem_rcu_free()
>> -> security_sem_free()
>>
>> Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes if
>> the pointer is NULL?
> I tried to ask for vmcore and do more analysis, basically, the race condition
> still exists and open a hole to be DoS.
Is the issue reproducable?
If yes, can you try something like the attached patch?
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm
> *ipc_perms,
> u32 sid = current_sid();
>
> isec = ipc_perms->security;
> + if (!isec)
> + return -EACCES;
>
> ad.type = LSM_AUDIT_DATA_IPC;
> ad.u.ipc_id = ipc_perms->key;
ipc_has_perm() runs without any spinlocks/semaphores, only rcu_read_lock().
Testing for ipc_perms->security!=NULL does solve the issue that
ipc_perm->key could be an access to kfree'd memory: Nothing prevents
that the kfree could happen just after the test.
I.e.: The patch can't be the right solution.
--
Manfred
[-- Attachment #2: patch-sem_ipc_has_perm --]
[-- Type: text/plain, Size: 2351 bytes --]
diff --git a/ipc/sem.c b/ipc/sem.c
index 6115146..80371dc 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -248,6 +248,7 @@ static void sem_rcu_free(struct rcu_head *head)
struct ipc_rcu *p = container_of(head, struct ipc_rcu, rcu);
struct sem_array *sma = ipc_rcu_to_struct(p);
+pr_info("sem_rcu_free: sma %p\n",sma);
security_sem_free(sma);
ipc_rcu_free(head);
}
@@ -529,6 +530,7 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
sma->sem_nsems = nsems;
sma->sem_ctime = get_seconds();
+pr_info("newary: sma %p becomes visible\n",sma);
id = ipc_addid(&sem_ids(ns), &sma->sem_perm, ns->sc_semmni);
if (id < 0) {
ipc_rcu_putref(sma, sem_rcu_free);
@@ -1118,6 +1120,7 @@ static void freeary(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp)
/* Remove the semaphore set from the IDR */
sem_rmid(ns, sma);
+pr_info("freeary: sma %p unlinked\n",sma);
sem_unlock(sma, -1);
rcu_read_unlock();
@@ -1860,6 +1863,9 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
if (ipcperms(ns, &sma->sem_perm, alter ? S_IWUGO : S_IRUGO))
goto out_rcu_wakeup;
+ if (sma->sem_perm.security == NULL) {
+ pr_info("sma %p: sem_perm.security == NULL\n", sma);
+ }
error = security_sem_semop(sma, sops, nsops, alter);
if (error)
goto out_rcu_wakeup;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6da7532..1499787 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5088,6 +5088,7 @@ static int ipc_alloc_security(struct task_struct *task,
isec->sclass = sclass;
isec->sid = sid;
perm->security = isec;
+pr_info("ipc_alloc_security for perm %p.\n", perm);
return 0;
}
@@ -5096,6 +5097,7 @@ static void ipc_free_security(struct kern_ipc_perm *perm)
{
struct ipc_security_struct *isec = perm->security;
perm->security = NULL;
+pr_info("ipc_free_security for perm %p.\n", perm);
kfree(isec);
}
@@ -5129,6 +5131,12 @@ static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
u32 sid = current_sid();
isec = ipc_perms->security;
+ if (isec == NULL) {
+ struct sem_array *sma = container_of(ipc_perms, struct sem_array, sem_perm);
+
+ pr_info("sma %p, sem_base %p, deleted %d with NULL isec\n",
+ sma, sma->sem_base, sma->sem_perm.deleted);
+ }
ad.type = LSM_AUDIT_DATA_IPC;
ad.u.ipc_id = ipc_perms->key;
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-22 2:44 ` Ethan Zhao
@ 2015-01-22 19:05 ` Stephen Smalley
-1 siblings, 0 replies; 35+ messages in thread
From: Stephen Smalley @ 2015-01-22 19:05 UTC (permalink / raw)
To: Ethan Zhao
Cc: Manfred Spraul, Stephen Smalley, Ethan Zhao, James Morris,
Serge E. Hallyn, Eric Paris, Paul Moore, selinux,
linux-security-module, LKML, ethan.kernel
On Wed, Jan 21, 2015 at 9:44 PM, Ethan Zhao <ethan.kernel@gmail.com> wrote:
> On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
> <manfred@colorfullife.com> wrote:
>> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
>>>
>>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
>>> wrote:
>>>>
>>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>>>>
>>>>> sys_semget()
>>>>> ->newary()
>>>>> ->security_sem_alloc()
>>>>> ->sem_alloc_security()
>>>>> selinux_sem_alloc_security()
>>>>> ->ipc_alloc_security() {
>>>>> ->rc = avc_has_perm()
>>>>> if (rc) {
>>>>>
>>>>> ipc_free_security(&sma->sem_perm);
>>>>> return rc;
>>>>
>>>> We free the security structure here to avoid a memory leak on a
>>>> failed/denied semaphore set creation. In this situation, we return an
>>>> error to the caller (ultimately to newary), it does an
>>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>>>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>>>> created. So how then can you call semtimedop() on it?
>>>
>>> Seems it wouldn't happen after commit
>>> e8577d1f0329d4842e8302e289fb2c22156abef4 ?
>>
>> That was my first guess when I read the bug report - but it can't be the
>> fix, because security_sem_alloc() is before the ipc_addid(), with or without
>> the patch.
>>
>> thread A:
>> thread B:
>>
>> semtimedop()
>> -> sem_obtain_object_check()
>> semctl(IPC_RMID)
>> -> freeary()
>> -> ipc_rcu_putref()
>> -> call_rcu()
>> -> somehow a grace period
>> -> sem_rcu_free()
>> -> security_sem_free()
>>
>> Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes if
>> the pointer is NULL?
>
> I tried to ask for vmcore and do more analysis, basically, the race condition
> still exists and open a hole to be DoS.
You said the patch was tested with 3.19-rc5. But did you reproduce
the bug on that kernel version before the patch? If not, what kernel
version were you running when you triggered the bug?
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-22 19:05 ` Stephen Smalley
0 siblings, 0 replies; 35+ messages in thread
From: Stephen Smalley @ 2015-01-22 19:05 UTC (permalink / raw)
To: Ethan Zhao
Cc: ethan.kernel, Ethan Zhao, Manfred Spraul, LKML,
linux-security-module, James Morris, Stephen Smalley, selinux
On Wed, Jan 21, 2015 at 9:44 PM, Ethan Zhao <ethan.kernel@gmail.com> wrote:
> On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
> <manfred@colorfullife.com> wrote:
>> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
>>>
>>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
>>> wrote:
>>>>
>>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>>>>
>>>>> sys_semget()
>>>>> ->newary()
>>>>> ->security_sem_alloc()
>>>>> ->sem_alloc_security()
>>>>> selinux_sem_alloc_security()
>>>>> ->ipc_alloc_security() {
>>>>> ->rc = avc_has_perm()
>>>>> if (rc) {
>>>>>
>>>>> ipc_free_security(&sma->sem_perm);
>>>>> return rc;
>>>>
>>>> We free the security structure here to avoid a memory leak on a
>>>> failed/denied semaphore set creation. In this situation, we return an
>>>> error to the caller (ultimately to newary), it does an
>>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>>>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>>>> created. So how then can you call semtimedop() on it?
>>>
>>> Seems it wouldn't happen after commit
>>> e8577d1f0329d4842e8302e289fb2c22156abef4 ?
>>
>> That was my first guess when I read the bug report - but it can't be the
>> fix, because security_sem_alloc() is before the ipc_addid(), with or without
>> the patch.
>>
>> thread A:
>> thread B:
>>
>> semtimedop()
>> -> sem_obtain_object_check()
>> semctl(IPC_RMID)
>> -> freeary()
>> -> ipc_rcu_putref()
>> -> call_rcu()
>> -> somehow a grace period
>> -> sem_rcu_free()
>> -> security_sem_free()
>>
>> Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes if
>> the pointer is NULL?
>
> I tried to ask for vmcore and do more analysis, basically, the race condition
> still exists and open a hole to be DoS.
You said the patch was tested with 3.19-rc5. But did you reproduce
the bug on that kernel version before the patch? If not, what kernel
version were you running when you triggered the bug?
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-22 19:05 ` Stephen Smalley
@ 2015-01-22 20:48 ` Davidlohr Bueso
-1 siblings, 0 replies; 35+ messages in thread
From: Davidlohr Bueso @ 2015-01-22 20:48 UTC (permalink / raw)
To: Stephen Smalley
Cc: Ethan Zhao, Manfred Spraul, Stephen Smalley, Ethan Zhao,
James Morris, Serge E. Hallyn, Eric Paris, Paul Moore, selinux,
linux-security-module, LKML, ethan.kernel
On Thu, 2015-01-22 at 14:05 -0500, Stephen Smalley wrote:
> On Wed, Jan 21, 2015 at 9:44 PM, Ethan Zhao <ethan.kernel@gmail.com> wrote:
> > On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
> > <manfred@colorfullife.com> wrote:
> >> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
> >>>
> >>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
> >>> wrote:
> >>>>
> >>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
> >>>>>
> >>>>> sys_semget()
> >>>>> ->newary()
> >>>>> ->security_sem_alloc()
> >>>>> ->sem_alloc_security()
> >>>>> selinux_sem_alloc_security()
> >>>>> ->ipc_alloc_security() {
> >>>>> ->rc = avc_has_perm()
> >>>>> if (rc) {
> >>>>>
> >>>>> ipc_free_security(&sma->sem_perm);
> >>>>> return rc;
> >>>>
> >>>> We free the security structure here to avoid a memory leak on a
> >>>> failed/denied semaphore set creation. In this situation, we return an
> >>>> error to the caller (ultimately to newary), it does an
> >>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
> >>>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
> >>>> created. So how then can you call semtimedop() on it?
> >>>
> >>> Seems it wouldn't happen after commit
> >>> e8577d1f0329d4842e8302e289fb2c22156abef4 ?
> >>
> >> That was my first guess when I read the bug report - but it can't be the
> >> fix, because security_sem_alloc() is before the ipc_addid(), with or without
> >> the patch.
> >>
> >> thread A:
> >> thread B:
> >>
> >> semtimedop()
> >> -> sem_obtain_object_check()
> >> semctl(IPC_RMID)
> >> -> freeary()
> >> -> ipc_rcu_putref()
> >> -> call_rcu()
> >> -> somehow a grace period
> >> -> sem_rcu_free()
> >> -> security_sem_free()
> >>
> >> Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes if
> >> the pointer is NULL?
> >
> > I tried to ask for vmcore and do more analysis, basically, the race condition
> > still exists and open a hole to be DoS.
>
> You said the patch was tested with 3.19-rc5. But did you reproduce
> the bug on that kernel version before the patch? If not, what kernel
> version were you running when you triggered the bug?
Also, is this a vanilla kernel? Or from a distro?
Essentially, did the kernel with the reproducible bug have:
commit 53dad6d3a8e5ac1af8bacc6ac2134ae1a8b085f1
Author: Davidlohr Bueso <davidlohr@hp.com>
Date: Mon Sep 23 17:04:45 2013 -0700
ipc: fix race with LSMs
Currently, IPC mechanisms do security and auditing related checks under
RCU. However, since security modules can free the security structure,
for example, through selinux_[sem,msg_queue,shm]_free_security(), we can
race if the structure is freed before other tasks are done with it,
creating a use-after-free condition. Manfred illustrates this nicely,
for instance with shared mem and selinux:
-> do_shmat calls rcu_read_lock()
-> do_shmat calls shm_object_check().
Checks that the object is still valid - but doesn't acquire any locks.
Then it returns.
-> do_shmat calls security_shm_shmat (e.g. selinux_shm_shmat)
-> selinux_shm_shmat calls ipc_has_perm()
-> ipc_has_perm accesses ipc_perms->security
shm_close()
-> shm_close acquires rw_mutex & shm_lock
-> shm_close calls shm_destroy
-> shm_destroy calls security_shm_free (e.g. selinux_shm_free_security)
-> selinux_shm_free_security calls ipc_free_security(&shp->shm_perm)
-> ipc_free_security calls kfree(ipc_perms->security)
This patch delays the freeing of the security structures after all RCU
readers are done. Furthermore it aligns the security life cycle with
that of the rest of IPC - freeing them based on the reference counter.
For situations where we need not free security, the current behavior is
kept. Linus states:
"... the old behavior was suspect for another reason too: having the
security blob go away from under a user sounds like it could cause
various other problems anyway, so I think the old code was at least
_prone_ to bugs even if it didn't have catastrophic behavior."
I have tested this patch with IPC testcases from LTP on both my
quad-core laptop and on a 64 core NUMA server. In both cases selinux is
enabled, and tests pass for both voluntary and forced preemption models.
While the mentioned races are theoretical (at least no one as reported
them), I wanted to make sure that this new logic doesn't break anything
we weren't aware of.
Additionally, Manfred's concerns about the grace period are quite valid,
and it obviously assumes that the ->security nil dereference issue still
exists to some extent. Changes in RCU are something to consider as well.
This is all pretty iffy though, lets make sure we are looking at the
right kernel first.
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-22 20:48 ` Davidlohr Bueso
0 siblings, 0 replies; 35+ messages in thread
From: Davidlohr Bueso @ 2015-01-22 20:48 UTC (permalink / raw)
To: Stephen Smalley
Cc: ethan.kernel, Ethan Zhao, Manfred Spraul, Stephen Smalley, LKML,
linux-security-module, James Morris, Ethan Zhao, selinux
On Thu, 2015-01-22 at 14:05 -0500, Stephen Smalley wrote:
> On Wed, Jan 21, 2015 at 9:44 PM, Ethan Zhao <ethan.kernel@gmail.com> wrote:
> > On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
> > <manfred@colorfullife.com> wrote:
> >> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
> >>>
> >>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
> >>> wrote:
> >>>>
> >>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
> >>>>>
> >>>>> sys_semget()
> >>>>> ->newary()
> >>>>> ->security_sem_alloc()
> >>>>> ->sem_alloc_security()
> >>>>> selinux_sem_alloc_security()
> >>>>> ->ipc_alloc_security() {
> >>>>> ->rc = avc_has_perm()
> >>>>> if (rc) {
> >>>>>
> >>>>> ipc_free_security(&sma->sem_perm);
> >>>>> return rc;
> >>>>
> >>>> We free the security structure here to avoid a memory leak on a
> >>>> failed/denied semaphore set creation. In this situation, we return an
> >>>> error to the caller (ultimately to newary), it does an
> >>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
> >>>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
> >>>> created. So how then can you call semtimedop() on it?
> >>>
> >>> Seems it wouldn't happen after commit
> >>> e8577d1f0329d4842e8302e289fb2c22156abef4 ?
> >>
> >> That was my first guess when I read the bug report - but it can't be the
> >> fix, because security_sem_alloc() is before the ipc_addid(), with or without
> >> the patch.
> >>
> >> thread A:
> >> thread B:
> >>
> >> semtimedop()
> >> -> sem_obtain_object_check()
> >> semctl(IPC_RMID)
> >> -> freeary()
> >> -> ipc_rcu_putref()
> >> -> call_rcu()
> >> -> somehow a grace period
> >> -> sem_rcu_free()
> >> -> security_sem_free()
> >>
> >> Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes if
> >> the pointer is NULL?
> >
> > I tried to ask for vmcore and do more analysis, basically, the race condition
> > still exists and open a hole to be DoS.
>
> You said the patch was tested with 3.19-rc5. But did you reproduce
> the bug on that kernel version before the patch? If not, what kernel
> version were you running when you triggered the bug?
Also, is this a vanilla kernel? Or from a distro?
Essentially, did the kernel with the reproducible bug have:
commit 53dad6d3a8e5ac1af8bacc6ac2134ae1a8b085f1
Author: Davidlohr Bueso <davidlohr@hp.com>
Date: Mon Sep 23 17:04:45 2013 -0700
ipc: fix race with LSMs
Currently, IPC mechanisms do security and auditing related checks under
RCU. However, since security modules can free the security structure,
for example, through selinux_[sem,msg_queue,shm]_free_security(), we can
race if the structure is freed before other tasks are done with it,
creating a use-after-free condition. Manfred illustrates this nicely,
for instance with shared mem and selinux:
-> do_shmat calls rcu_read_lock()
-> do_shmat calls shm_object_check().
Checks that the object is still valid - but doesn't acquire any locks.
Then it returns.
-> do_shmat calls security_shm_shmat (e.g. selinux_shm_shmat)
-> selinux_shm_shmat calls ipc_has_perm()
-> ipc_has_perm accesses ipc_perms->security
shm_close()
-> shm_close acquires rw_mutex & shm_lock
-> shm_close calls shm_destroy
-> shm_destroy calls security_shm_free (e.g. selinux_shm_free_security)
-> selinux_shm_free_security calls ipc_free_security(&shp->shm_perm)
-> ipc_free_security calls kfree(ipc_perms->security)
This patch delays the freeing of the security structures after all RCU
readers are done. Furthermore it aligns the security life cycle with
that of the rest of IPC - freeing them based on the reference counter.
For situations where we need not free security, the current behavior is
kept. Linus states:
"... the old behavior was suspect for another reason too: having the
security blob go away from under a user sounds like it could cause
various other problems anyway, so I think the old code was at least
_prone_ to bugs even if it didn't have catastrophic behavior."
I have tested this patch with IPC testcases from LTP on both my
quad-core laptop and on a 64 core NUMA server. In both cases selinux is
enabled, and tests pass for both voluntary and forced preemption models.
While the mentioned races are theoretical (at least no one as reported
them), I wanted to make sure that this new logic doesn't break anything
we weren't aware of.
Additionally, Manfred's concerns about the grace period are quite valid,
and it obviously assumes that the ->security nil dereference issue still
exists to some extent. Changes in RCU are something to consider as well.
This is all pretty iffy though, lets make sure we are looking at the
right kernel first.
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-22 18:15 ` Manfred Spraul
@ 2015-01-23 2:00 ` ethan zhao
-1 siblings, 0 replies; 35+ messages in thread
From: ethan zhao @ 2015-01-23 2:00 UTC (permalink / raw)
To: Manfred Spraul
Cc: Ethan Zhao, Stephen Smalley, james.l.morris, serge, Eric Paris,
Paul Moore, selinux, linux-security-module, LKML
Manfred,
On 2015/1/23 2:15, Manfred Spraul wrote:
> On 01/22/2015 03:44 AM, Ethan Zhao wrote:
>> On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
>> <manfred@colorfullife.com> wrote:
>>> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
>>>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
>>>> wrote:
>>>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>>>>> sys_semget()
>>>>>> ->newary()
>>>>>> ->security_sem_alloc()
>>>>>> ->sem_alloc_security()
>>>>>> selinux_sem_alloc_security()
>>>>>> ->ipc_alloc_security() {
>>>>>> ->rc = avc_has_perm()
>>>>>> if (rc) {
>>>>>>
>>>>>> ipc_free_security(&sma->sem_perm);
>>>>>> return rc;
>>>>> We free the security structure here to avoid a memory leak on a
>>>>> failed/denied semaphore set creation. In this situation, we
>>>>> return an
>>>>> error to the caller (ultimately to newary), it does an
>>>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>>>>> caller. Thus, it never calls ipc_addid() and the semaphore set is
>>>>> not
>>>>> created. So how then can you call semtimedop() on it?
>>>> Seems it wouldn't happen after commit
>>>> e8577d1f0329d4842e8302e289fb2c22156abef4 ?
>>> That was my first guess when I read the bug report - but it can't be
>>> the
>>> fix, because security_sem_alloc() is before the ipc_addid(), with or
>>> without
>>> the patch.
>>>
>>> thread A:
>>> thread B:
>>>
>>> semtimedop()
>>> -> sem_obtain_object_check()
>>> semctl(IPC_RMID)
>>> -> freeary()
>>> -> ipc_rcu_putref()
>>> -> call_rcu()
>>> -> somehow a grace period
>>> -> sem_rcu_free()
>>> -> security_sem_free()
>>>
>>> Perhaps: modify ipc_free_security() to hexdump perm and a few more
>>> bytes if
>>> the pointer is NULL?
>> I tried to ask for vmcore and do more analysis, basically, the race
>> condition
>> still exists and open a hole to be DoS.
> Is the issue reproducable?
It was hit on an user's VMware while running a process named "opcmon"
maybe ported from HP_UX. I asked for the vmwcore, but not get it yet.
> If yes, can you try something like the attached patch?
Yes, will.
Thanks,
Ethan
>
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm
>> *ipc_perms,
>> u32 sid = current_sid();
>>
>> isec = ipc_perms->security;
>> + if (!isec)
>> + return -EACCES;
>>
>> ad.type = LSM_AUDIT_DATA_IPC;
>> ad.u.ipc_id = ipc_perms->key;
> ipc_has_perm() runs without any spinlocks/semaphores, only
> rcu_read_lock().
> Testing for ipc_perms->security!=NULL does solve the issue that
> ipc_perm->key could be an access to kfree'd memory: Nothing prevents
> that the kfree could happen just after the test.
>
> I.e.: The patch can't be the right solution.
>
> --
> Manfred
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-23 2:00 ` ethan zhao
0 siblings, 0 replies; 35+ messages in thread
From: ethan zhao @ 2015-01-23 2:00 UTC (permalink / raw)
To: Manfred Spraul
Cc: Ethan Zhao, LKML, linux-security-module, james.l.morris,
Stephen Smalley, selinux
Manfred,
On 2015/1/23 2:15, Manfred Spraul wrote:
> On 01/22/2015 03:44 AM, Ethan Zhao wrote:
>> On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
>> <manfred@colorfullife.com> wrote:
>>> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
>>>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
>>>> wrote:
>>>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>>>>> sys_semget()
>>>>>> ->newary()
>>>>>> ->security_sem_alloc()
>>>>>> ->sem_alloc_security()
>>>>>> selinux_sem_alloc_security()
>>>>>> ->ipc_alloc_security() {
>>>>>> ->rc = avc_has_perm()
>>>>>> if (rc) {
>>>>>>
>>>>>> ipc_free_security(&sma->sem_perm);
>>>>>> return rc;
>>>>> We free the security structure here to avoid a memory leak on a
>>>>> failed/denied semaphore set creation. In this situation, we
>>>>> return an
>>>>> error to the caller (ultimately to newary), it does an
>>>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>>>>> caller. Thus, it never calls ipc_addid() and the semaphore set is
>>>>> not
>>>>> created. So how then can you call semtimedop() on it?
>>>> Seems it wouldn't happen after commit
>>>> e8577d1f0329d4842e8302e289fb2c22156abef4 ?
>>> That was my first guess when I read the bug report - but it can't be
>>> the
>>> fix, because security_sem_alloc() is before the ipc_addid(), with or
>>> without
>>> the patch.
>>>
>>> thread A:
>>> thread B:
>>>
>>> semtimedop()
>>> -> sem_obtain_object_check()
>>> semctl(IPC_RMID)
>>> -> freeary()
>>> -> ipc_rcu_putref()
>>> -> call_rcu()
>>> -> somehow a grace period
>>> -> sem_rcu_free()
>>> -> security_sem_free()
>>>
>>> Perhaps: modify ipc_free_security() to hexdump perm and a few more
>>> bytes if
>>> the pointer is NULL?
>> I tried to ask for vmcore and do more analysis, basically, the race
>> condition
>> still exists and open a hole to be DoS.
> Is the issue reproducable?
It was hit on an user's VMware while running a process named "opcmon"
maybe ported from HP_UX. I asked for the vmwcore, but not get it yet.
> If yes, can you try something like the attached patch?
Yes, will.
Thanks,
Ethan
>
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm
>> *ipc_perms,
>> u32 sid = current_sid();
>>
>> isec = ipc_perms->security;
>> + if (!isec)
>> + return -EACCES;
>>
>> ad.type = LSM_AUDIT_DATA_IPC;
>> ad.u.ipc_id = ipc_perms->key;
> ipc_has_perm() runs without any spinlocks/semaphores, only
> rcu_read_lock().
> Testing for ipc_perms->security!=NULL does solve the issue that
> ipc_perm->key could be an access to kfree'd memory: Nothing prevents
> that the kfree could happen just after the test.
>
> I.e.: The patch can't be the right solution.
>
> --
> Manfred
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-22 19:05 ` Stephen Smalley
@ 2015-01-23 2:19 ` ethan zhao
-1 siblings, 0 replies; 35+ messages in thread
From: ethan zhao @ 2015-01-23 2:19 UTC (permalink / raw)
To: Stephen Smalley
Cc: Ethan Zhao, Manfred Spraul, Stephen Smalley, James Morris,
Serge E. Hallyn, Eric Paris, Paul Moore, selinux,
linux-security-module, LKML, ethan.kernel
Stephen,
On 2015/1/23 3:05, Stephen Smalley wrote:
> On Wed, Jan 21, 2015 at 9:44 PM, Ethan Zhao <ethan.kernel@gmail.com> wrote:
>> On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
>> <manfred@colorfullife.com> wrote:
>>> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
>>>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
>>>> wrote:
>>>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>>>>> sys_semget()
>>>>>> ->newary()
>>>>>> ->security_sem_alloc()
>>>>>> ->sem_alloc_security()
>>>>>> selinux_sem_alloc_security()
>>>>>> ->ipc_alloc_security() {
>>>>>> ->rc = avc_has_perm()
>>>>>> if (rc) {
>>>>>>
>>>>>> ipc_free_security(&sma->sem_perm);
>>>>>> return rc;
>>>>> We free the security structure here to avoid a memory leak on a
>>>>> failed/denied semaphore set creation. In this situation, we return an
>>>>> error to the caller (ultimately to newary), it does an
>>>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>>>>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>>>>> created. So how then can you call semtimedop() on it?
>>>> Seems it wouldn't happen after commit
>>>> e8577d1f0329d4842e8302e289fb2c22156abef4 ?
>>> That was my first guess when I read the bug report - but it can't be the
>>> fix, because security_sem_alloc() is before the ipc_addid(), with or without
>>> the patch.
>>>
>>> thread A:
>>> thread B:
>>>
>>> semtimedop()
>>> -> sem_obtain_object_check()
>>> semctl(IPC_RMID)
>>> -> freeary()
>>> -> ipc_rcu_putref()
>>> -> call_rcu()
>>> -> somehow a grace period
>>> -> sem_rcu_free()
>>> -> security_sem_free()
>>>
>>> Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes if
>>> the pointer is NULL?
>> I tried to ask for vmcore and do more analysis, basically, the race condition
>> still exists and open a hole to be DoS.
> You said the patch was tested with 3.19-rc5.
I just threw the 3.19-rc5 with my test patch to the 'user', he said he
doesn't hit. maybe he didn't hit or occasionally failed to reproduce it.
> But did you reproduce
> the bug on that kernel version before the patch?
Good news is not hit yet.
> If not, what kernel
> version were you running when you triggered the bug?
To be honest, a kernel from distro, but not released, but before we
get it clear, we wouldn't public more.
Thanks,
Ethan
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-23 2:19 ` ethan zhao
0 siblings, 0 replies; 35+ messages in thread
From: ethan zhao @ 2015-01-23 2:19 UTC (permalink / raw)
To: Stephen Smalley
Cc: ethan.kernel, Manfred Spraul, Stephen Smalley, LKML,
linux-security-module, James Morris, Ethan Zhao, selinux
Stephen,
On 2015/1/23 3:05, Stephen Smalley wrote:
> On Wed, Jan 21, 2015 at 9:44 PM, Ethan Zhao <ethan.kernel@gmail.com> wrote:
>> On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
>> <manfred@colorfullife.com> wrote:
>>> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
>>>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
>>>> wrote:
>>>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>>>>> sys_semget()
>>>>>> ->newary()
>>>>>> ->security_sem_alloc()
>>>>>> ->sem_alloc_security()
>>>>>> selinux_sem_alloc_security()
>>>>>> ->ipc_alloc_security() {
>>>>>> ->rc = avc_has_perm()
>>>>>> if (rc) {
>>>>>>
>>>>>> ipc_free_security(&sma->sem_perm);
>>>>>> return rc;
>>>>> We free the security structure here to avoid a memory leak on a
>>>>> failed/denied semaphore set creation. In this situation, we return an
>>>>> error to the caller (ultimately to newary), it does an
>>>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>>>>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>>>>> created. So how then can you call semtimedop() on it?
>>>> Seems it wouldn't happen after commit
>>>> e8577d1f0329d4842e8302e289fb2c22156abef4 ?
>>> That was my first guess when I read the bug report - but it can't be the
>>> fix, because security_sem_alloc() is before the ipc_addid(), with or without
>>> the patch.
>>>
>>> thread A:
>>> thread B:
>>>
>>> semtimedop()
>>> -> sem_obtain_object_check()
>>> semctl(IPC_RMID)
>>> -> freeary()
>>> -> ipc_rcu_putref()
>>> -> call_rcu()
>>> -> somehow a grace period
>>> -> sem_rcu_free()
>>> -> security_sem_free()
>>>
>>> Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes if
>>> the pointer is NULL?
>> I tried to ask for vmcore and do more analysis, basically, the race condition
>> still exists and open a hole to be DoS.
> You said the patch was tested with 3.19-rc5.
I just threw the 3.19-rc5 with my test patch to the 'user', he said he
doesn't hit. maybe he didn't hit or occasionally failed to reproduce it.
> But did you reproduce
> the bug on that kernel version before the patch?
Good news is not hit yet.
> If not, what kernel
> version were you running when you triggered the bug?
To be honest, a kernel from distro, but not released, but before we
get it clear, we wouldn't public more.
Thanks,
Ethan
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-22 20:48 ` Davidlohr Bueso
@ 2015-01-23 2:38 ` ethan zhao
-1 siblings, 0 replies; 35+ messages in thread
From: ethan zhao @ 2015-01-23 2:38 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: Stephen Smalley, Ethan Zhao, Manfred Spraul, Stephen Smalley,
James Morris, Serge E. Hallyn, Eric Paris, Paul Moore, selinux,
linux-security-module, LKML, ethan.kernel
Davidlohr,
On 2015/1/23 4:48, Davidlohr Bueso wrote:
> On Thu, 2015-01-22 at 14:05 -0500, Stephen Smalley wrote:
>> On Wed, Jan 21, 2015 at 9:44 PM, Ethan Zhao <ethan.kernel@gmail.com> wrote:
>>> On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
>>> <manfred@colorfullife.com> wrote:
>>>> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
>>>>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
>>>>> wrote:
>>>>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>>>>>> sys_semget()
>>>>>>> ->newary()
>>>>>>> ->security_sem_alloc()
>>>>>>> ->sem_alloc_security()
>>>>>>> selinux_sem_alloc_security()
>>>>>>> ->ipc_alloc_security() {
>>>>>>> ->rc = avc_has_perm()
>>>>>>> if (rc) {
>>>>>>>
>>>>>>> ipc_free_security(&sma->sem_perm);
>>>>>>> return rc;
>>>>>> We free the security structure here to avoid a memory leak on a
>>>>>> failed/denied semaphore set creation. In this situation, we return an
>>>>>> error to the caller (ultimately to newary), it does an
>>>>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>>>>>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>>>>>> created. So how then can you call semtimedop() on it?
>>>>> Seems it wouldn't happen after commit
>>>>> e8577d1f0329d4842e8302e289fb2c22156abef4 ?
>>>> That was my first guess when I read the bug report - but it can't be the
>>>> fix, because security_sem_alloc() is before the ipc_addid(), with or without
>>>> the patch.
>>>>
>>>> thread A:
>>>> thread B:
>>>>
>>>> semtimedop()
>>>> -> sem_obtain_object_check()
>>>> semctl(IPC_RMID)
>>>> -> freeary()
>>>> -> ipc_rcu_putref()
>>>> -> call_rcu()
>>>> -> somehow a grace period
>>>> -> sem_rcu_free()
>>>> -> security_sem_free()
>>>>
>>>> Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes if
>>>> the pointer is NULL?
>>> I tried to ask for vmcore and do more analysis, basically, the race condition
>>> still exists and open a hole to be DoS.
>> You said the patch was tested with 3.19-rc5. But did you reproduce
>> the bug on that kernel version before the patch? If not, what kernel
>> version were you running when you triggered the bug?
> Also, is this a vanilla kernel? Or from a distro?
The hard thing, it is hit on customer's environment, the issue kernel
doesn't
have many commits far from the last about IPC/SElinux.
> Essentially, did the kernel with the reproducible bug have:
Not easy to do reproduce it is triggered by a process "opcmon" not
public to everyone.
What I have is the panic log. the vmware not get yet.
Thanks,
Ethan
> commit 53dad6d3a8e5ac1af8bacc6ac2134ae1a8b085f1
> Author: Davidlohr Bueso <davidlohr@hp.com>
> Date: Mon Sep 23 17:04:45 2013 -0700
>
> ipc: fix race with LSMs
No, the kernel doesn't have this commit, will try it.
>
> Currently, IPC mechanisms do security and auditing related checks under
> RCU. However, since security modules can free the security structure,
> for example, through selinux_[sem,msg_queue,shm]_free_security(), we can
> race if the structure is freed before other tasks are done with it,
> creating a use-after-free condition. Manfred illustrates this nicely,
> for instance with shared mem and selinux:
>
> -> do_shmat calls rcu_read_lock()
> -> do_shmat calls shm_object_check().
> Checks that the object is still valid - but doesn't acquire any locks.
> Then it returns.
> -> do_shmat calls security_shm_shmat (e.g. selinux_shm_shmat)
> -> selinux_shm_shmat calls ipc_has_perm()
> -> ipc_has_perm accesses ipc_perms->security
>
> shm_close()
> -> shm_close acquires rw_mutex & shm_lock
> -> shm_close calls shm_destroy
> -> shm_destroy calls security_shm_free (e.g. selinux_shm_free_security)
> -> selinux_shm_free_security calls ipc_free_security(&shp->shm_perm)
> -> ipc_free_security calls kfree(ipc_perms->security)
>
> This patch delays the freeing of the security structures after all RCU
> readers are done. Furthermore it aligns the security life cycle with
> that of the rest of IPC - freeing them based on the reference counter.
> For situations where we need not free security, the current behavior is
> kept. Linus states:
>
> "... the old behavior was suspect for another reason too: having the
> security blob go away from under a user sounds like it could cause
> various other problems anyway, so I think the old code was at least
> _prone_ to bugs even if it didn't have catastrophic behavior."
>
> I have tested this patch with IPC testcases from LTP on both my
> quad-core laptop and on a 64 core NUMA server. In both cases selinux is
> enabled, and tests pass for both voluntary and forced preemption models.
> While the mentioned races are theoretical (at least no one as reported
> them), I wanted to make sure that this new logic doesn't break anything
> we weren't aware of.
>
>
> Additionally, Manfred's concerns about the grace period are quite valid,
> and it obviously assumes that the ->security nil dereference issue still
> exists to some extent. Changes in RCU are something to consider as well.
> This is all pretty iffy though, lets make sure we are looking at the
> right kernel first.
Thanks,
Ethan
>
> Thanks,
> Davidlohr
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-23 2:38 ` ethan zhao
0 siblings, 0 replies; 35+ messages in thread
From: ethan zhao @ 2015-01-23 2:38 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: ethan.kernel, Manfred Spraul, Stephen Smalley, LKML,
linux-security-module, James Morris, Ethan Zhao, selinux
Davidlohr,
On 2015/1/23 4:48, Davidlohr Bueso wrote:
> On Thu, 2015-01-22 at 14:05 -0500, Stephen Smalley wrote:
>> On Wed, Jan 21, 2015 at 9:44 PM, Ethan Zhao <ethan.kernel@gmail.com> wrote:
>>> On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
>>> <manfred@colorfullife.com> wrote:
>>>> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
>>>>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
>>>>> wrote:
>>>>>> On 01/20/2015 04:18 AM, Ethan Zhao wrote:
>>>>>>> sys_semget()
>>>>>>> ->newary()
>>>>>>> ->security_sem_alloc()
>>>>>>> ->sem_alloc_security()
>>>>>>> selinux_sem_alloc_security()
>>>>>>> ->ipc_alloc_security() {
>>>>>>> ->rc = avc_has_perm()
>>>>>>> if (rc) {
>>>>>>>
>>>>>>> ipc_free_security(&sma->sem_perm);
>>>>>>> return rc;
>>>>>> We free the security structure here to avoid a memory leak on a
>>>>>> failed/denied semaphore set creation. In this situation, we return an
>>>>>> error to the caller (ultimately to newary), it does an
>>>>>> ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
>>>>>> caller. Thus, it never calls ipc_addid() and the semaphore set is not
>>>>>> created. So how then can you call semtimedop() on it?
>>>>> Seems it wouldn't happen after commit
>>>>> e8577d1f0329d4842e8302e289fb2c22156abef4 ?
>>>> That was my first guess when I read the bug report - but it can't be the
>>>> fix, because security_sem_alloc() is before the ipc_addid(), with or without
>>>> the patch.
>>>>
>>>> thread A:
>>>> thread B:
>>>>
>>>> semtimedop()
>>>> -> sem_obtain_object_check()
>>>> semctl(IPC_RMID)
>>>> -> freeary()
>>>> -> ipc_rcu_putref()
>>>> -> call_rcu()
>>>> -> somehow a grace period
>>>> -> sem_rcu_free()
>>>> -> security_sem_free()
>>>>
>>>> Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes if
>>>> the pointer is NULL?
>>> I tried to ask for vmcore and do more analysis, basically, the race condition
>>> still exists and open a hole to be DoS.
>> You said the patch was tested with 3.19-rc5. But did you reproduce
>> the bug on that kernel version before the patch? If not, what kernel
>> version were you running when you triggered the bug?
> Also, is this a vanilla kernel? Or from a distro?
The hard thing, it is hit on customer's environment, the issue kernel
doesn't
have many commits far from the last about IPC/SElinux.
> Essentially, did the kernel with the reproducible bug have:
Not easy to do reproduce it is triggered by a process "opcmon" not
public to everyone.
What I have is the panic log. the vmware not get yet.
Thanks,
Ethan
> commit 53dad6d3a8e5ac1af8bacc6ac2134ae1a8b085f1
> Author: Davidlohr Bueso <davidlohr@hp.com>
> Date: Mon Sep 23 17:04:45 2013 -0700
>
> ipc: fix race with LSMs
No, the kernel doesn't have this commit, will try it.
>
> Currently, IPC mechanisms do security and auditing related checks under
> RCU. However, since security modules can free the security structure,
> for example, through selinux_[sem,msg_queue,shm]_free_security(), we can
> race if the structure is freed before other tasks are done with it,
> creating a use-after-free condition. Manfred illustrates this nicely,
> for instance with shared mem and selinux:
>
> -> do_shmat calls rcu_read_lock()
> -> do_shmat calls shm_object_check().
> Checks that the object is still valid - but doesn't acquire any locks.
> Then it returns.
> -> do_shmat calls security_shm_shmat (e.g. selinux_shm_shmat)
> -> selinux_shm_shmat calls ipc_has_perm()
> -> ipc_has_perm accesses ipc_perms->security
>
> shm_close()
> -> shm_close acquires rw_mutex & shm_lock
> -> shm_close calls shm_destroy
> -> shm_destroy calls security_shm_free (e.g. selinux_shm_free_security)
> -> selinux_shm_free_security calls ipc_free_security(&shp->shm_perm)
> -> ipc_free_security calls kfree(ipc_perms->security)
>
> This patch delays the freeing of the security structures after all RCU
> readers are done. Furthermore it aligns the security life cycle with
> that of the rest of IPC - freeing them based on the reference counter.
> For situations where we need not free security, the current behavior is
> kept. Linus states:
>
> "... the old behavior was suspect for another reason too: having the
> security blob go away from under a user sounds like it could cause
> various other problems anyway, so I think the old code was at least
> _prone_ to bugs even if it didn't have catastrophic behavior."
>
> I have tested this patch with IPC testcases from LTP on both my
> quad-core laptop and on a 64 core NUMA server. In both cases selinux is
> enabled, and tests pass for both voluntary and forced preemption models.
> While the mentioned races are theoretical (at least no one as reported
> them), I wanted to make sure that this new logic doesn't break anything
> we weren't aware of.
>
>
> Additionally, Manfred's concerns about the grace period are quite valid,
> and it obviously assumes that the ->security nil dereference issue still
> exists to some extent. Changes in RCU are something to consider as well.
> This is all pretty iffy though, lets make sure we are looking at the
> right kernel first.
Thanks,
Ethan
>
> Thanks,
> Davidlohr
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-23 2:19 ` ethan zhao
@ 2015-01-23 3:30 ` Davidlohr Bueso
-1 siblings, 0 replies; 35+ messages in thread
From: Davidlohr Bueso @ 2015-01-23 3:30 UTC (permalink / raw)
To: ethan zhao
Cc: Stephen Smalley, Ethan Zhao, Manfred Spraul, Stephen Smalley,
James Morris, Serge E. Hallyn, Eric Paris, Paul Moore, selinux,
linux-security-module, LKML, ethan.kernel
On Fri, 2015-01-23 at 10:19 +0800, ethan zhao wrote:
> > If not, what kernel
> > version were you running when you triggered the bug?
> To be honest, a kernel from distro, but not released, but before we
> get it clear, we wouldn't public more.
Sheesh, could Oracle be any more (ridiculously) secretive about what the
hell kernel(s) they run... it's like pulling teeth. *sigh*
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-23 3:30 ` Davidlohr Bueso
0 siblings, 0 replies; 35+ messages in thread
From: Davidlohr Bueso @ 2015-01-23 3:30 UTC (permalink / raw)
To: ethan zhao
Cc: ethan.kernel, Manfred Spraul, Stephen Smalley, LKML,
linux-security-module, James Morris, Ethan Zhao, selinux
On Fri, 2015-01-23 at 10:19 +0800, ethan zhao wrote:
> > If not, what kernel
> > version were you running when you triggered the bug?
> To be honest, a kernel from distro, but not released, but before we
> get it clear, we wouldn't public more.
Sheesh, could Oracle be any more (ridiculously) secretive about what the
hell kernel(s) they run... it's like pulling teeth. *sigh*
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
2015-01-23 3:30 ` Davidlohr Bueso
@ 2015-01-23 15:30 ` Ethan Zhao
-1 siblings, 0 replies; 35+ messages in thread
From: Ethan Zhao @ 2015-01-23 15:30 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: ethan zhao, Stephen Smalley, Manfred Spraul, Stephen Smalley,
James Morris, Serge E. Hallyn, Eric Paris, Paul Moore, selinux,
linux-security-module, LKML, ethan.kernel
Davidlohr，
I read your commit 53dad6d3a8e5ac1af8bacc6ac2134ae1a8b085f1,
ipc: fix race with LSMs
The issue we hit without the above patch, the race may happen when
process call semctl with IPC_RMID just as Manfred Spraul mentioned:
Thread A:
IPC_RMID
-> freeary()
->wake_up_sem_queue_do()
Thread B:
->security_sem_free() semtimedop()
->ipcperms()
->ipc_rcu_putref()
If this is the only race, the bug should be fixed with your patch applied
(not verified yet on my case).
Thanks,
Ethan
On Fri, Jan 23, 2015 at 11:30 AM, Davidlohr Bueso <dave@stgolabs.net> wrote:
> On Fri, 2015-01-23 at 10:19 +0800, ethan zhao wrote:
>> > If not, what kernel
>> > version were you running when you triggered the bug?
>> To be honest, a kernel from distro, but not released, but before we
>> get it clear, we wouldn't public more.
>
> Sheesh, could Oracle be any more (ridiculously) secretive about what the
> hell kernel(s) they run... it's like pulling teeth. *sigh*
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()
@ 2015-01-23 15:30 ` Ethan Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Ethan Zhao @ 2015-01-23 15:30 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: ethan.kernel, ethan zhao, Manfred Spraul, LKML,
linux-security-module, James Morris, Stephen Smalley, selinux
Davidlohr，
I read your commit 53dad6d3a8e5ac1af8bacc6ac2134ae1a8b085f1,
ipc: fix race with LSMs
The issue we hit without the above patch, the race may happen when
process call semctl with IPC_RMID just as Manfred Spraul mentioned:
Thread A:
IPC_RMID
-> freeary()
->wake_up_sem_queue_do()
Thread B:
->security_sem_free() semtimedop()
->ipcperms()
->ipc_rcu_putref()
If this is the only race, the bug should be fixed with your patch applied
(not verified yet on my case).
Thanks,
Ethan
On Fri, Jan 23, 2015 at 11:30 AM, Davidlohr Bueso <dave@stgolabs.net> wrote:
> On Fri, 2015-01-23 at 10:19 +0800, ethan zhao wrote:
>> > If not, what kernel
>> > version were you running when you triggered the bug?
>> To be honest, a kernel from distro, but not released, but before we
>> get it clear, we wouldn't public more.
>
> Sheesh, could Oracle be any more (ridiculously) secretive about what the
> hell kernel(s) they run... it's like pulling teeth. *sigh*
>
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2015-01-23 15:30 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-20 9:18 [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop() Ethan Zhao
2015-01-20 9:18 ` Ethan Zhao
2015-01-20 14:10 ` Stephen Smalley
2015-01-20 14:10 ` Stephen Smalley
2015-01-20 18:49 ` Manfred Spraul
2015-01-20 18:49 ` Manfred Spraul
2015-01-20 21:01 ` Stephen Smalley
2015-01-20 21:06 ` Eric Paris
2015-01-20 21:06 ` Eric Paris
2015-01-20 21:09 ` Stephen Smalley
2015-01-20 21:09 ` Stephen Smalley
2015-01-21 1:30 ` ethan zhao
2015-01-21 1:30 ` ethan zhao
2015-01-21 3:53 ` Ethan Zhao
2015-01-21 3:53 ` Ethan Zhao
2015-01-21 5:30 ` Manfred Spraul
2015-01-21 5:30 ` Manfred Spraul
2015-01-22 2:44 ` Ethan Zhao
2015-01-22 2:44 ` Ethan Zhao
2015-01-22 18:15 ` Manfred Spraul
2015-01-22 18:15 ` Manfred Spraul
2015-01-23 2:00 ` ethan zhao
2015-01-23 2:00 ` ethan zhao
2015-01-22 19:05 ` Stephen Smalley
2015-01-22 19:05 ` Stephen Smalley
2015-01-22 20:48 ` Davidlohr Bueso
2015-01-22 20:48 ` Davidlohr Bueso
2015-01-23 2:38 ` ethan zhao
2015-01-23 2:38 ` ethan zhao
2015-01-23 2:19 ` ethan zhao
2015-01-23 2:19 ` ethan zhao
2015-01-23 3:30 ` Davidlohr Bueso
2015-01-23 3:30 ` Davidlohr Bueso
2015-01-23 15:30 ` Ethan Zhao
2015-01-23 15:30 ` Ethan Zhao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.