ebiederm@xmission.com (Eric W. Biederman) writes:
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?
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
On Wed, Nov 11, 2020 at 02:28:38PM -0600, Eric W. Biederman wrote:
ebiederm@xmission.com (Eric W. Biederman) writes:
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?
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.
You may well be right that the only time extra fields coming from
userspace should be deliberately nonzero is in the passthrough case.
I'm still concerned about padding or type-punning issues causing
unallocated fields to be initialised with junk, even in the presence of
a memset(0). I haven't actually seen this happen, but the language
standards seem to allow it and compilers may get more aggressive in the
future.
This is why I think we still want an explicit way for userspace to
indicate what extension fields initialised.
Am I missing something that eliminates the danger today?
Further I expect it is rare for debuggers to generate pretend faults.
That seems a reasonable assumption. Hand-rolling a fault siginfo seems
a pretty fragile thing to do, if it works reliably at all.
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
Ack
Cheers
---Dave