linux-kernel - Re: pselect/etc semantics

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: <20190530155715.GH22536@redhat.com>
Date:   Thu, 30 May 2019 17:57:16 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Deepa Dinamani <deepa.kernel@...il.com>,
        Al Viro <viro@...IV.linux.org.uk>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
        arnd@...db.de, dbueso@...e.de, axboe@...nel.dk, dave@...olabs.net,
        e@...24.org, jbaron@...mai.com, linux-fsdevel@...r.kernel.org,
        linux-aio@...ck.org, omar.kilani@...il.com, tglx@...utronix.de,
        stable@...r.kernel.org
Subject: Re: pselect/etc semantics

On 05/30, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@...hat.com> writes:
>
> > Al, Linus, Eric, please help.
> >
> > The previous discussion was very confusing, we simply can not understand each
> > other.
> >
> > To me everything looks very simple and clear, but perhaps I missed something
> > obvious? Please correct me.
>
> If I have read this thread correctly the core issue is that ther is a
> program that used to work and that fails now.  The question is why.

I didn't even try to investigate, I wasn't cc'ed initially and I then I had
enough confusion when I started to look at the patch.

However, 854a6ed56839a40f6 obviously introduced the user-visible change so
I am not surprised it breaks something. And yes, personally I think this
change is not right.

> Which means I believe we have a semantically valid change in behavior
> that is causing a regression.

See below,

> void restore_user_sigmask(const void __user *usigmask, sigset_t *sigsaved)
> {
>
> 	if (!usigmask)
> 		return;
> 	/*
> 	 * When signals are pending, do not restore them here.
> 	 * Restoring sigmask here can lead to delivering signals that the above
> 	 * syscalls are intended to block because of the sigmask passed in.
> 	 */
> 	if (signal_pending(current)) {
> 		current->saved_sigmask = *sigsaved;
> 		set_restore_sigmask();
> 		return;
> 	}
>
> 	/*
> 	 * This is needed because the fast syscall return path does not restore
> 	 * saved_sigmask when signals are not pending.
> 	 */
> 	set_current_blocked(sigsaved);
> }
>
> Which has been reported results in a return value of 0,

0 or success

> and a signal
> delivered, where previously that did not happen.

Yes.

And to me this doesn't look right. OK, OK, probably this is because I never
read the docs, only the source code in fs/select.c. But to me pselect() should
either return success/timeout or deliver a signal. Not both. Even if the signal
was already pending before pselect() was called.

To clarify, "a signal" means a signal which was blocked before pselect(sigmask)
and temporary unblocked in this syscall.

And even if this doesn't violate posix, I see no reason to change the historic
behaviour. And this regression probably means we can't ;)

Oleg.