linux-kernel - Re: [PATCH v2 0/2] signalfd/epoll 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:	Sat, 25 Feb 2012 11:00:10 -0800
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Davide Libenzi <davidel@...ilserver.org>,
	Eric Dumazet <eric.dumazet@...il.com>,
	Greg KH <greg@...ah.com>, Jason Baron <jbaron@...hat.com>,
	Roland McGrath <roland@...k.frob.com>,
	Eugene Teo <eugeneteo@...nel.sg>,
	Maxime Bizon <mbizon@...ebox.fr>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/2] signalfd/epoll fixes

On Sat, Feb 25, 2012 at 8:08 AM, Oleg Nesterov <oleg@...hat.com> wrote:
>
> Yes, this fixes use-after-free. But suppose the the application does:
>
>        sigfd = signalfd(...);
>        epoll_ctl(epollfd, EPOLL_CTL_ADD, sigfd);
>        execve();

Well, that can't work anyway. We're adding to the wrong sighand.

Sure, we can force a wakeup for it at execve(), but that's not the old
behavior either, so why would we care about the above case? It's not a
sane thing to do, and it has never worked before either, so why
bother? My patch should make it "work" as well as it ever did.

Quite frankly, if you wanted to make signalfd work sanely with
eventpoll, you'd need to change the semantics entirely, and just say
"signalfd works in the context it was creared in, no other". Those
aren't the semantics we have, though, and there's no point in trying
to make up some *new* semantics for "if you execve() we'll still
follow it"

> And in any case. If the task exits, to me it looks simply pointless
> to keep its ->sighand until __fput(). This only makes poll_rm() or
> whatever safe, it can't report a signal. OK, OK, I am not arguing,
> POLLFREE is equally ugly or even worse.

That's my point. POLLFREE really does look worse, and doesn't really
give any saner behaviors.

> I _think_ that the right fix should move wait_queue_head_t from
> sighand_struct to signalfd_ctx (file->private_data) somehow...

I looked at it - and it requires the same "poll_rm()" callback, and
then instead you have to make allocations in poll() (for the list) and
de-allocate in "poll_rm()".

I do agree that in many ways it would be the "right" thing to do, but
it does require the same infrastructure, and the advantage is dubious
for this case. The main advantage is that we can extend the signalfd +
epoll interaction in new directions (like the "across execve" or other
cases), but I'd argue that we don't *want* to do that.

So your POLLFREE patches are merged, and I think they work. The reason
I like the poll_rm() thing is that I think it's conceptually the right
solution, and while our "poll_wait()" interface has really worked very
well, the fact that you cannot do anything stateful in it (because
there is never any way to undo anything except for the "remove from
wait queue" action) is very rigid and inflexible.

Of course, aside from signalfd, that inflexibility has never really
mattered. So..

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