Messages in this thread | | | Date | Fri, 23 Jan 2015 10:38:27 +0800 | From | ethan zhao <> | Subject | Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop() |
| |
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 >
| |