Re: [PATCH v14 7/8] signal: define the field siginfo.si_faultflags

From: ebiederm@xmission•com (Eric W. Biederman)
To: Dave Martin <Dave.Martin@arm•com>
Cc: Peter Collingbourne <pcc@google•com>,
	Catalin Marinas <catalin.marinas@arm•com>,
	Helge Deller <deller@gmx•de>,
	Kevin Brodsky <kevin.brodsky@arm•com>,
	Oleg Nesterov <oleg@redhat•com>,
	linux-api@vger•kernel.org,
	"James E.J. Bottomley" <James.Bottomley@hansenpartnership•com>,
	Kostya Serebryany <kcc@google•com>,
	Linux ARM <linux-arm-kernel@lists•infradead.org>,
	Andrey Konovalov <andreyknvl@google•com>,
	David Spickett <david.spickett@linaro•org>,
	Vincenzo Frascino <vincenzo.frascino@arm•com>,
	Will Deacon <will@kernel•org>,
	Evgenii Stepanov <eugenis@google•com>,
	Richard Henderson <rth@twiddle•net>
Subject: Re: [PATCH v14 7/8] signal: define the field siginfo.si_faultflags
Date: Wed, 11 Nov 2020 14:28:38 -0600	[thread overview]
Message-ID: <87sg9fprih.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <87imabr6p8.fsf@x220.int.ebiederm.org> (Eric W. Biederman's message of "Wed, 11 Nov 2020 14:15:15 -0600")

ebiederm@xmission•com (Eric W. Biederman) writes:

> Dave Martin <Dave.Martin@arm•com> writes:
>
>> On Mon, Nov 09, 2020 at 07:57:33PM -0600, Eric W. Biederman wrote:
>>> Peter Collingbourne <pcc@google•com> writes:
>>> 
>>> > This field will contain flags that may be used by signal handlers to
>>> > determine whether other fields in the _sigfault portion of siginfo are
>>> > valid. An example use case is the following patch, which introduces
>>> > the si_addr_tag_bits{,_mask} fields.
>>> >
>>> > A new sigcontext flag, SA_FAULTFLAGS, is introduced in order to allow
>>> > a signal handler to require the kernel to set the field (but note
>>> > that the field will be set anyway if the kernel supports the flag,
>>> > regardless of its value). In combination with the previous patches,
>>> > this allows a userspace program to determine whether the kernel will
>>> > set the field.
>>> >
>>> > It is possible for an si_faultflags-unaware program to cause a signal
>>> > handler in an si_faultflags-aware program to be called with a provided
>>> > siginfo data structure by using one of the following syscalls:
>>> >
>>> > - ptrace(PTRACE_SETSIGINFO)
>>> > - pidfd_send_signal
>>> > - rt_sigqueueinfo
>>> > - rt_tgsigqueueinfo
>>> >
>>> > So we need to prevent the si_faultflags-unaware program from causing an
>>> > uninitialized read of si_faultflags in the si_faultflags-aware program when
>>> > it uses one of these syscalls.
>>> >
>>> > The last three cases can be handled by observing that each of these
>>> > syscalls fails if si_code >= 0. We also observe that kill(2) and
>>> > tgkill(2) may be used to send a signal where si_code == 0 (SI_USER),
>>> > so we define si_faultflags to only be valid if si_code > 0.
>>> >
>>> > There is no such check on si_code in ptrace(PTRACE_SETSIGINFO), so
>>> > we make ptrace(PTRACE_SETSIGINFO) clear the si_faultflags field if it
>>> > detects that the signal would use the _sigfault layout, and introduce
>>> > a new ptrace request type, PTRACE_SETSIGINFO2, that a si_faultflags-aware
>>> > program may use to opt out of this behavior.
>>> 
>>> So I think while well intentioned this is misguided.
>>> 
>>> gdb and the like may use this but I expect the primary user is CRIU
>>> which simply reads the signal out of one process saves it on disk
>>> and then restores the signal as read into the new process (possibly
>>> on a different machine).
>>> 
>>> At least for the CRIU usage PTRACE_SETSIGINFO need to remain a raw
>>> pass through kind of operation.
>>
>> This is a problem, though.
>>
>> How can we tell the difference between a siginfo that was generated by
>> the kernel and a siginfo that was generated (or altered) by a non-xflags
>> aware userspace?
>>
>> Short of revving the whole API, I don't see a simple solution to this.
>
> Unlike receiving a signal.  We do know that userspace old and new
> always sends unused fields as zero into PTRACE_SETSIGINFO.
>
> The split into kernel_siginfo verifies this and fails userspace if it
> does something different.  No problems have been reported.
>
> So in the case of xflags a non-xflags aware userspace would either pass
> the siginfo from through from somewhere else (such as
> PTRACE_GETSIGINFO), or it would simply generate a signal with all of
> the xflags bits clear.  So everything should work regardless.
>
>> Although a bit of a hack, could we include some kind of checksum in the
>> siginfo?  If the checksum matches during PTRACE_SETSIGINFO, we could
>> accept the whole thing; xflags included.  Otherwise, we could silently
>> drop non-self-describing extensions.
>>
>> If we only need to generate the checksum when PTRACE_GETSIGINFO is
>> called then it might be feasible to use a strong hash; otherwise, this
>> mechanism will be far from bulletproof.
>>
>> A hash has the advantage that we don't need any other information
>> to validate it beyond a salt: if the hash matches, it's self-
>> validating.  We could also package other data with it to describe the
>> presence of extensions, but relying on this for regular sigaction()/
>> signal delivery use feels too high-overhead.
>>
>> For debuggers, I suspect that PTRACE_SETSIGINFO2 is still useful:
>> userspace callers that want to write an extension field that they
>> knowingly generated themselves should have a way to express that.
>>
>> Thoughts?
>
> I think there are two cases:
> 1) CRIU  -- It is just a passthrough of PTRACE_GETSIGINFO
> 2) Creating a signal from nowhere -- Code that does not know about
>    xflags would leave xflags at 0 so no problem.
>
> Does anyone see any other cases I am missing?
>

Zoinks.  I forgot to read and double check the code I wrote.

copy_siginfo_from_user only verifies against 0 when we don't know the
layout.  So I don't know if we can count on userspace providing the
extra data as 0 or not.

So if we do indeed continue to need xflags we might care.

It is currently an undefined non-sense case to provide non-zero fields
there.  So I think it is reasonable to expect even debuggers generating
signals to set those fields to know values such as 0.

Further I expect it is rare for debuggers to generate pretend faults.

So I would say perform whatever testing we can, so that there are no
obvious problem users of PTRACE_SETSIGINFO and then to simply not worry
about the PTRACE_SETSIGINFO unless someone reports a regression.

But fist let's see if we really need xflags at all.

Eric

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel