Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()

[ethan zhao <ethan.zhao@oracle.com>]

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
>