linux-kernel - Re: [PATCH 2/2] signal: Better detection of synchronous signals

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:   Mon, 11 Feb 2019 18:01:39 -0600
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Dmitry Vyukov <dvyukov@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        jolsa@...hat.com, Namhyung Kim <namhyung@...nel.org>,
        luca abeni <luca.abeni@...tannapisa.it>,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: [PATCH 2/2] signal: Better detection of synchronous signals

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

> Eric, I'll try to finally read the whole thread later, probably I missed
> something, but...
>
> On 02/07, Eric W. Biederman wrote:
>>
>> Recently syzkaller was able to create unkillablle processes by
>> creating a timer that is delivered as a thread local signal on SIGHUP,
>> and receiving SIGHUP SA_NODEFERER.  Ultimately causing a loop failing
>> to deliver SIGHUP but always trying.
>>
>> When the stack overflows delivery of SIGHUP fails and force_sigsegv is
>> called.  Unfortunately because SIGSEGV is numerically higher than
>> SIGHUP next_signal tries again to deliver a SIGHUP.
>
> Confused... In this particular case next_signal() should return SIGSEGV
> because it must be pending too and SYNCHRONOUS_MASK doesn't include SIGHUP.
>
> Not that it really matters, the timer can deliver another SYNCHRONOUS_MASK
> signal < SIGSEGV, just I am trying to understand what have I missed...

Bah. It was SIGBUS.  My brain kept thinking SIGHUP. My apologies this
mental error slipped into the patch description.

>> +	/*
>> +	 * Check if there is another siginfo for the same signal.
>> +	 */
>> +	list_for_each_entry_continue(q, &pending->list, list) {
>> +		if (q->info.si_signo == sync->info.si_signo)
>> +			goto still_pending;
>> +	}
>
> But this must not be possible? SYNCHRONOUS_MASK doesn't include real-time
> signals, we can't have 2 siginfo's for the same signal < SIGRTMIN.

Yes for that reason it should be safe to strip that logic out at the
moment.  I overlooked that when writing the code.

However.  I am not certain that is a limit we actually want to honor
with synchronous signals.  As it results in a louzy quality of
implementation.

We start with an instruction in the program being debugged.  In
principle before that instruction starts we know that no signals
are pending because they were not delivered to that process.

If we for some reason send signal A to the process and at the same time
hit a fault that is reported as signal A.  It is currently a race which
one wins.  I think we could legitimately say that the fault happened
before signal A was enqueued, and deliver both. It is a bit murkier if
signal A was blocked.

If we let the enqueued signal A win (as we do today) we have SA_SIGNFO
that is not useful for describing the fault the instruction generated.
Which is a really lousy quality of implementation.

Which is a long way of saying I think that hunk of code is useful as it
allows us the possibility of fixing a lousy quality of implementation in
our code today.

Eric