Dave Martin [off-list ref] writes:

On Tue, Apr 17, 2018 at 02:37:38PM -0500, Eric W. Biederman wrote:

quoted

Dave Martin [off-list ref] writes:

quoted

Hmmm

memset()/clear_siginfo() may ensure that there are no uninitialised
explicit fields except for those in inactive union members, but I'm not
sure that this approach is guaranteed to sanitise the padding seen by
userspace.

Rationale below, though it's a bit theoretical...

With this in mind, I tend agree with Linus that hiding memset() calls
from the maintainer may be a bad idea unless they are also hidden from
the compiler.  If the compiler sees the memset() it may be able to
optimise it in ways that wouldn't be possible for some other random
external function call, including optimising all or part of the call
out.

As a result, the breakdown into individual put_user()s etc. in
copy_siginfo_to_user() may still be valuable even if all paths have the
memset().

The breakdown into individual put_user()s is known to be problematically
slow, and is actually wrong.

Slowness certainly looked like a potential problem.

quoted

Even exclusing the SI_USER duplication in a small number of cases the
fields filled out in siginfo by architecture code are not the fields
that copy_siginfo_to_user is copying.  Which is much worse.  The code
looks safe but is not.

My intention is to leave 0 instances of clear_siginfo in the
architecture specific code.  Ideally struct siginfo will be limited to
kernel/signal.c but I am not certain I can quite get that far.
The function do_coredump appears to have a legit need for siginfo.

So, you mean we can't detect that the caller didn't initialise all the
members, or initialised the wrong union member?

Correct.  Even when we smuggled the the union member in the upper bits
of si_code we got it wrong.  So an interface that helps out and does
more and is harder to misues looks desirable.

What would be the alternative?  Have a separate interface for each SIL_
type, with only kernel/signal.c translating that into the siginfo_t that
userspace sees?

Yes.  It really isn't bad as architecture specific code only generates
faults.  In general faults only take a pointer.  I have already merged
the needed helpers into kernel/signal.c

Either way, I don't see how we force the caller to initilise the whole
structure.

In general the plan is to convert the callers to call force_sig_fault,
and then there is no need to have siginfo in the architecture specific
code.  I have all of the necessary helpers are already merged into
kernel/signal.c

quoted

quoted

(Rationale for an arch/arm example:)

quoted

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 4c375e11ae95..adda3fc2dde8 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c

@@ -218,8 +218,7 @@ static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs)
 {
 	siginfo_t info;
 
-	memset(&info, 0, sizeof(info));
-
+	clear_siginfo(&info);
 	info.si_signo = SIGFPE;

/* by c11 (n1570) 6.2.6.1 para 6 [1], all padding bytes in info now take
   unspecified values */

quoted

 	info.si_code = sicode;
 	info.si_addr = (void __user *)(instruction_pointer(regs) - 4);

/* by c11 (n1570) 6.2.6.1 para 7 [2], all bytes of the union info._sifields
   other than than those corresponding to _sigfault take unspecified
   values */

So I don't see why the compiler needs to ensure that any of the affected
bytes are zero: it could potentially skip a lot of the memset() as a
result, in theory.

I've not seen a compiler actually take advantage of that, but I'm now
not sure what forbids it.

I took a quick look at gcc-4.9 which I have handy.

The passes -f-no-strict-aliasing which helps, and gcc actually
documents that if you access things through the union it will
not take advantage of c11.

gcc-4.9 Documents it this way:

quoted

-fstrict-aliasing'
     Allow the compiler to assume the strictest aliasing rules
     applicable to the language being compiled.  For C (and C++), this
     activates optimizations based on the type of expressions.  In
     particular, an object of one type is assumed never to reside at the
     same address as an object of a different type, unless the types are
     almost the same.  For example, an 'unsigned int' can alias an
     'int', but not a 'void*' or a 'double'.  A character type may alias
     any other type.

     Pay special attention to code like this:
          union a_union {
            int i;
            double d;
          };

          int f() {
            union a_union t;
            t.d = 3.0;
            return t.i;
          }
     The practice of reading from a different union member than the one
     most recently written to (called "type-punning") is common.  Even
     with '-fstrict-aliasing', type-punning is allowed, provided the
     memory is accessed through the union type.  So, the code above
     works as expected.

This makes the C standard look precise (I love the "works as expected"),
and says nothing about the cumulative effect of assigning to multiple
members of a union, or about the effects on padding bytes.

I'm not convinced that all of this falls under strict-aliasing, but
I'd have to do more digging to confirm it.

quoted

quoted

If this can happen, I only see two watertight workarounds:

1) Ensure that there is no implicit padding in any UAPI structure, e.g.
aeb1f39d814b: ("arm64/ptrace: Avoid uninitialised struct padding in
fpr_set()").  This would include tail-padding of any union member that
is smaller than the containing union.

It would be significantly more effort to ensure this for siginfo though.

2) Poke all values directly into allocated or user memory directly
via pointers to paddingless types; never assign to objects on the kernel
stack if you care what ends up in the padding, e.g., what your
copy_siginfo_to_user() does prior to this series.


If I'm not barking up the wrong tree, memset() cannot generally be
used to determine the value of padding bytes, but it may still be
useful for forcing otherwise uninitialised members to sane initial
values.

This likely affects many more things than just siginfo.

Unless gcc has changed it's stance on type-punning through unions
or it's semantics with -fno-strict_aliasing we should be good.

In practice you're probably right.

Today, gcc is pretty conservative in this area, and I haven't been able
to convince clang to optimise away memset in this way either.

My concern is that is this assumption turns out to be wrong it may be
some time before anybody notices, because the leakage of kernel stack may
be the only symptom.

I'll try to nail down a compiler guy to see if we can get a promise on
this at least with -fno-strict-aliasing.


I wonder whether it's worth protecting ourselves with something like:


static void clear_siginfo(siginfo_t *si)
{
	asm ("" : "=m" (*si));
	memset(si, 0, sizeof(*si));
	asm ("" : "+m" (*si));
}

Probably needs to be thought about more widely though.  I guess it's out
of scope for this series.

It is definitely a question worth asking.

Eric