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

[Eric W. Biederman]

Dave Martin <Dave.Martin at arm.com> writes:

> On Mon, Nov 09, 2020 at 07:57:33PM -0600, Eric W. Biederman wrote:
>> Peter Collingbourne <pcc at 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?

Eric