linux-kernel - Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user

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: <87lfy96sta.fsf@xmission.com>
Date:   Mon, 10 Jun 2019 16:20:33 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Deepa Dinamani <deepa.kernel@...il.com>,
        linux-kernel@...r.kernel.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,
        Al Viro <viro@...IV.linux.org.uk>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        David Laight <David.Laight@...LAB.COM>,
        linux-arch@...r.kernel.org
Subject: Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

Oleg Nesterov <oleg@...hat.com> writes:

> On 06/07, Eric W. Biederman wrote:
>>
>> +static int set_sigmask(sigset_t *kmask)
>> +{
>> +	set_restore_sigmask();
>> +	current->saved_sigmask = current->blocked;
>> +	set_current_blocked(kmask);
>> +
>> +	return 0;
>> +}
>
> I was going to do the same change except my version returns void ;)
>
> So ACK.
>
>
> As for 2-5, sorry I can't read them today, will do tomorrow.
>
> But at first glance... yes, we can remove TIF_RESTORE_SIGMASK.
>
> As for "remove saved_sigmask" I have some concerns... At least this
> means a user-visible change iiuc. Say, pselect unblocks a fatal signal.
> Say, SIGINT without a handler. Suppose SIGINT comes after set_sigmask().
>
> Before this change the process will be killed.
>
> After this change it will be killed or not. It won't be killed if
> do_select() finds an already ready fd without blocking, or it finds a
> ready fd right after SIGINT interrupts poll_schedule_timeout().

Yes.  Because having the signal set in real_blocked disables the
immediate kill optimization, and the signal has to be delivered before
we decide to kill the process.  Which matters because as you say if
nothing checks signal_pending() when the signals are unblocked we might
not attempt to deliver the signal.

So it is a matter of timing.

If we have both a signal and a file descriptor become ready
at the same time I would call that a race.  Either could
wake up the process and depending on the exact time we could
return either one.

So it is possible that today if the signal came just after the file
descriptor ,the code might have made it to restore_saved_sigmask_unless,
before __send_signal runs.

I see the concern.  I think in a matter like this we try it.  Make
the patches clean so people can bisect the problem.  Then if someone
runs into this problem we revert the offending patches.

If it looks like bisection won't cleanly reveal the potential problem
please let me know.

Personally I don't think anyone sane would intentionally depend on this
and I don't think there is a sufficiently reliable way to depend on this
by accident that people would actually be depending on it.

> And _to me_ the new behaviour makes more sense. But when it comes to
> user-visible changes you can never know if it breaks something or not.

True.

The set of applications is larger than any developer can reasonably test.

Eric