From: ethan zhao <ethan.zhao@oracle.com> To: Davidlohr Bueso <dave@stgolabs.net> Cc: Stephen Smalley <stephen.smalley@gmail.com>, Ethan Zhao <ethan.kernel@gmail.com>, Manfred Spraul <manfred@colorfullife.com>, Stephen Smalley <sds@tycho.nsa.gov>, James Morris <james.l.morris@oracle.com>, "Serge E. Hallyn" <serge@hallyn.com>, Eric Paris <eparis@parisplace.org>, Paul Moore <paul@paul-moore.com>, selinux <selinux@tycho.nsa.gov>, linux-security-module@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>, ethan.kernel@gmail.conm Subject: Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop() Date: Fri, 23 Jan 2015 10:38:27 +0800 [thread overview] Message-ID: <54C1B423.6080103@oracle.com> (raw) In-Reply-To: <1421959704.4903.29.camel@stgolabs.net> 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 >
WARNING: multiple messages have this Message-ID (diff)
From: ethan zhao <ethan.zhao@oracle.com> To: Davidlohr Bueso <dave@stgolabs.net> Cc: ethan.kernel@gmail.conm, Manfred Spraul <manfred@colorfullife.com>, Stephen Smalley <sds@tycho.nsa.gov>, LKML <linux-kernel@vger.kernel.org>, linux-security-module@vger.kernel.org, James Morris <james.l.morris@oracle.com>, Ethan Zhao <ethan.kernel@gmail.com>, selinux <selinux@tycho.nsa.gov> Subject: Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop() Date: Fri, 23 Jan 2015 10:38:27 +0800 [thread overview] Message-ID: <54C1B423.6080103@oracle.com> (raw) In-Reply-To: <1421959704.4903.29.camel@stgolabs.net> 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 >
next prev parent reply other threads:[~2015-01-23 2:38 UTC|newest] Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top 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 [this message] 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
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=54C1B423.6080103@oracle.com \ --to=ethan.zhao@oracle.com \ --cc=dave@stgolabs.net \ --cc=eparis@parisplace.org \ --cc=ethan.kernel@gmail.com \ --cc=ethan.kernel@gmail.conm \ --cc=james.l.morris@oracle.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-security-module@vger.kernel.org \ --cc=manfred@colorfullife.com \ --cc=paul@paul-moore.com \ --cc=sds@tycho.nsa.gov \ --cc=selinux@tycho.nsa.gov \ --cc=serge@hallyn.com \ --cc=stephen.smalley@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.