Hi Peter,
On Mon, Nov 02, 2020 at 08:09:43PM -0800, Peter Collingbourne wrote:
> 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.
As per patch 5, a user is supposed to call sigaction() twice to figure
out whether _faultflags is meaningful. That's the part I'm not
particularly fond of. Are the unused parts of siginfo always zeroed when
the kernel delivers a signal? If yes, we could simply check the new
field for non-zero bits.
> 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.
I find this pretty fragile but maybe I have to read it a few more times
to fully understand the implications ;).
Could we instead copy all the fields, potentially uninitialised, and
instead filter them when delivering the signal based on the
SA_FAULTFLAGS? That means that the kernel only writes si_faultflags if
the user requested it.
> v12:
> - Change type of si_xflags to u32 to avoid increasing alignment
[...]
> diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
> index 7aacf9389010..f43778355b77 100644
> --- a/include/uapi/asm-generic/siginfo.h
> +++ b/include/uapi/asm-generic/siginfo.h
> @@ -91,7 +91,9 @@ union __sifields {
> char _dummy_pkey[__ADDR_BND_PKEY_PAD];
> __u32 _pkey;
> } _addr_pkey;
> + void *_pad[6];
> };
> + __u32 _faultflags;
> } _sigfault;
Sorry, I haven't checked the previous discussion on alignment here but
don't we already require 64-bit alignment because of other members in
the _sigfault union? We already have void * throughout this and with the
next patch we just have a gap (unless I miscalculated the offsets).
--
Catalin
