linux-kernel - Re: [PATCH] kernel/exit.c: make sure current's nsproxy != NULL while checking caps

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]

Message-id: <1417085724.1805.7.camel@samsung.com>
Date:	Thu, 27 Nov 2014 11:55:24 +0100
From:	Lukasz Pawelczyk <l.pawelczyk@...sung.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Lukasz Pawelczyk <havner@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Michal Hocko <mhocko@...e.cz>,
	David Rientjes <rientjes@...gle.com>,
	Sameer Nanda <snanda@...omium.org>,
	Guillaume Morin <guillaume@...infr.org>,
	Li Zefan <lizefan@...wei.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kernel/exit.c: make sure current's nsproxy != NULL while
 checking caps

On śro, 2014-11-26 at 21:52 +0100, Oleg Nesterov wrote:
> On 11/26, Lukasz Pawelczyk wrote:
> >
> > My understanding is that while we have to use task_nsproxy()
> 
> task_nsproxy() has already gone... probably this doesn't matter but which
> kernel version ?

Ah, yes, 728dba3a39c66b3d8ac889ddbe38b5b1c264aec3. But you're right, it
doesn't matter here. I've seen this change, just forgot about it.

> > task's nsproxy and check whether it's NULL, for the 'current' we don't
> > have to and it's expected not to be NULL.
> 
> Well, unless exit_task_namespaces() was called ;)

That's the thing, should we ever get to a point that current's nsproxy
is NULL during an LSM check? That's why I mentioned code like:
current->nsproxy->some_ns

being in a kernel without a check. I think that current's nsproxy should
always be guaranteed to exist.

> > There seem to be no crash currently because of this, but with other LSM
> > modules or in future there might be. This is the backtrace:
> 
> Confused... backtrace of what? did kernel crash or what?

Sorry, probably should have explained this a little better. Yes, this is
backtrace of crash, but one that will not happen in the exact form on
master. This is of a modified smack that makes use of nsproxy during LSM
checks.

But this really doesn't matter. I posted this backtrace to show that
there is an LSM check that happens after exit_task_namespaces() has been
called.

> 
> > 0  smk_tskacc (task=0xffff88003b0b92e0, obj_known=0x2 <irq_stack_union+2>, mode=2, a=0xffff88003be53dd8) at security/smack/smack_access.c:261
> > 1  0xffffffff8130e2aa in smk_curacc (obj_known=<optimized out>, mode=<optimized out>, a=<optimized out>) at security/smack/smack_access.c:318
> > 2  0xffffffff8130a50d in smack_task_kill (p=0xffff88003b0b92e0, info=<optimized out>, sig=<optimized out>, secid=<optimized out>) at security/smack/smack_lsm.c:2071
> 
> I do not know this code, so could you please tell more? How/wher smk_tskacc()
> uses ->nsproxy?  smack_access.c:261 leads to the comment header above smk_curacc()
> in my tree, so this tells me nothing.

This is a code from my working tree. I'm working on Smack/LSM namespaces
that will make Smack use nsproxy during LSM checks. The code above is
not important at the moment. As I said previously this backtrace is just
a proof that an LSM check happens after exit_task_namespaces(), during
exit_notify().

> Well, we can probably move exit_task_namespaces() down (perhaps we even
> want to move it after exit_task_work).
> 
> But I am not sure about exit_notify(), in this case free_nsproxy() can
> be called when the caller is already reaped.
> 
> In any case, please more details?

The capability checks sometimes use nsproxy, e.g. user namespaces. Same
thing might be a case for Smack (or any other LSM module) when working
inside a namespace. I'm WIP on those changes and I will be trying to
upstream them to. This issue is a stopper for me so I though I'd try to
push it earlier.

I'm not expert on this code hence any suggestion would be welcome.
My understanding is that when we do an LSM hook there might be a
capability check inside. And this capability check might be using
ns_capable() instead of capable() if namespaced. And in the case
depicted above the nsproxy of the current process is already destroyed.
I think that this is a bug, even though it has no repercussions in the
kernel yet.

-- 
Lukasz Pawelczyk
Samsung R&D Institute Poland
Samsung Electronics

--
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/