Re: [PATCH 0/4] ipc: shm and msg fixes

[Manfred Spraul]

Hi all,

On 09/24/2013 02:04 AM, Davidlohr Bueso wrote:
> > (In reality, I suspect the reference count is never elevated in
> > practice, so there is only one case that calls the security freeing
> > thing, so this may all be pretty much theoretical, but at least from a
> > logic standpoint the code clearly makes a big deal about the whole
> > refcount and "last user turns off the lights").
> Right, this would/should have come up years ago if it were actually
> occurring in practice.

As far as I see, the only requirement is "last user does kfree()":
spin_lock must be possible and perm.deleted must be valid.

e.g. from msg.c:
>                 rcu_read_lock();
>                 ipc_lock_object(&msq->q_perm);
>
>                 ipc_rcu_putref(msq);
>                 if (msq->q_perm.deleted) {
>                         err = -EIDRM;
>                         goto out_unlock0;
>                 }



> 8<-----------------------------------------
> From: Davidlohr Bueso <davidlohr@xxxxxx>
> Subject: [PATCH] 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.
>
> Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Davidlohr Bueso <davidlohr@xxxxxx>
Signed-off-by: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/