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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 24 Sep 2013 11:05:40 +0200
From:	Manfred Spraul <manfred@...orfullife.com>
To:	Davidlohr Bueso <davidlohr@...com>
CC:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Eric Paris <eparis@...isplace.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Rik van Riel <riel@...hat.com>,
	Mike Galbraith <efault@....de>,
	Sedat Dilek <sedat.dilek@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Stephen Smalley <sds@...ho.nsa.gov>,
	James Morris <james.l.morris@...cle.com>,
	LSM List <linux-security-module@...r.kernel.org>,
	Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH 0/4] ipc: shm and msg fixes

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@...com>
> 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@...ux-foundation.org>
> Signed-off-by: Davidlohr Bueso <davidlohr@...com>
Signed-off-by: Manfred Spraul <manfred@...orfullife.com>

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