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

Dave Martin [off-list ref] writes:

On Wed, Nov 11, 2020 at 02:15:15PM -0600, Eric W. Biederman wrote:

quoted

Dave Martin [off-list ref] writes:

quoted

On Mon, Nov 09, 2020 at 07:57:33PM -0600, Eric W. Biederman wrote:

quoted

Peter Collingbourne [off-list ref] writes:

quoted

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.

quoted

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?

Eric, did you have any view on the hash idea here?

I am not quite certain what you meant by salt.  A per kernel instance
secret I suspect.

Such a secret would break creating siginfo by hand and checkpointing
and restoring on a different machine.

If you don't go with full crypto security it sounds like it would work.

If we really need to deploy xflags I think it bears looking at, but
right now it feels like one thing too many.



Eric