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

[Dave Martin <Dave.Martin@arm.com>]

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.

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?

Cheers
---Dave