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
| ||
|
Date: Tue, 8 Oct 2013 19:56:25 +0200 From: Oleg Nesterov <oleg@...hat.com> To: Chen Gang <gang.chen@...anux.com> Cc: "Eric W. Biederman" <ebiederm@...ssion.com>, Serge Hallyn <serge.hallyn@...onical.com>, "Serge E. Hallyn" <serge@...lyn.com>, Andrew Morton <akpm@...ux-foundation.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org> Subject: Re: [PATCH] kernel/pid.c: check pid whether be NULL in __change_pid() On 10/08, Chen Gang wrote: > > On 10/07/2013 08:43 PM, Oleg Nesterov wrote: > > > >> but still recommend to check it > >> in __change_pid() to let itself consistency. > > > > Why? > > > > Contrary, I think we should not hide the problem. If __change_pid() is > > called when task->pids[type].pid is already NULL there is something > > seriously wrong. > > > > Hmm... In my opinion, it means need BUG_ON() for original 'link->pid'. > > --------------------------------patch begin----------------------------- > > [PATCH] kernel/pid.c: add BUG_ON() for "!pid" in __change_pid() > > Within __change_pid(), 'new' may be NULL if it comes from detach_pid(), Yes, this is fine, > and 'link->pid' also may be NULL ("link->pid = new"), > ... > the original 'link->pid' may be NULL, too. Too? You mean, it becomes NULL after detach_pid(). > But in real world, all related extern functions always assume "if > 'link->pid' is already NULL, there must be something seriously wrong", > although __change_pid() can accept parameter 'new' as NULL. I simply can't understand why you mix "new == NULL" and "link->pid == NULL". > So in __change_pid(), need add BUG_ON() for it: "it should not happen, > when it really happen, OS must be continuing blindly, OS will crash a couple of lines below trying to dereference this pointer. > --- a/kernel/pid.c > +++ b/kernel/pid.c > @@ -396,6 +396,12 @@ static void __change_pid(struct task_struct *task, enum pid_type type, > link = &task->pids[type]; > pid = link->pid; > > + /* > + * If task->pids[type].pid is already NULL, there must be something > + * seriously wrong > + */ > + BUG_ON(!pid); Following this logic you should also add BUG_ON(!task); BUG_ON(!link->node.next); BUG_ON(!link->node.prev || link->node.prev == LIST_POISON2); ... Seriously, I do not understand the point. Yes, detach_pid() should not be called twice. And it has a single caller. And this caller will crash too if it is called twice. So you can also add BUG_ON() into __unhash_process(). And so on. Oleg. -- 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/
Powered by blists - more mailing lists