linux-kernel - Re: [PATCH] move tty_kref_put() outside of __cleanup

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: <20100318231632.GA13591@redhat.com>
Date:	Fri, 19 Mar 2010 00:16:32 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Alan Cox <alan@...ux.intel.com>,
	Roland McGrath <roland@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] move tty_kref_put() outside of __cleanup_signal()

On 03/18, Alan Cox wrote:
>
> > -			tty_kref_put(p->signal->tty);
> >  			p->signal->tty = tty_kref_get(current->signal->tty);
>
> That bit needs commenting clearly or a WARN_ON() that p->signal->tty is
> NULL before the get otherwise when the assumption is broken the flaw will
> be subtle and hard to find.

Well. It must be NULL. This tty_kref_get() was added by
9c9f4ded90a59eee84e15f5fd38c03d60184e112, and the same commit also added
sig->tty = NULL into copy_signal(). With the recent changes in copy_signal()
it must be NULL due to zalloc().

But this doesn't matter. We should not do tty_kref_put() even if it is not
NULL. If we do "put", we need the balancing "get" which we do not have.
That is why this "get" is not only unneeded, but wrong imho.

Probably the little comment makes sense, but I don't really understand what
it should explain. To me, this is like list_add_tail(&init_task.tasks) few
lines below. We do not have the comment to explain the "missing" list_del().
This task is new, nobody can see/use it before we drop the locks. NULL or not,
its signal->tty is just uninitialized yet.

> > --- 34-rc1/kernel/exit.c~7_TTY_PUT	2010-03-17 20:05:38.000000000 +0100
> > +++ 34-rc1/kernel/exit.c	2010-03-18 22:46:41.000000000 +0100
> > @@ -150,6 +150,7 @@ static void __exit_signal(struct task_st
> >  		 * see account_group_exec_runtime().
> >  		 */
> >  		task_rq_unlock_wait(tsk);
> > +		tty_kref_put(sig->tty);
>
> and a sig->tty = NULL assignment to trap races might not go amiss here
> perhaps ?

Indeed ;)

The subsequent patches will do this, we need more changes anyway. Currently
this doesn't matter because we are going to kfree() this memory unconditionally.
But when we pin ->signal to task_struct, we should clear ->signal->tty before
we drop ->siglock, then tty_kref_put().

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/