Messages in this thread | | | Subject | Re: [PATCH] kernel/exit.c: make sure current's nsproxy != NULL while checking caps | From | Lukasz Pawelczyk <> | Date | Thu, 27 Nov 2014 11:55:24 +0100 |
| |
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
| |