ebiederm@xxxxxxxxxxxx (Eric W. Biederman) writes:
> Dave Martin <Dave.Martin@xxxxxxx> writes:
>
>> On Mon, Nov 09, 2020 at 07:57:33PM -0600, Eric W. Biederman wrote:
>>> Peter Collingbourne <pcc@xxxxxxxxxx> 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
