On Tue, Apr 17, 2018 at 02:37:38PM -0500, Eric W. Biederman wrote:
> Dave Martin <Dave.Martin@xxxxxxx> writes:
>
> > 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.
> 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?
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?
Either way, I don't see how we force the caller to initilise the whole
structure.
> > (Rationale for an arch/arm example:)
> >
> >> 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 */
> >
> >> 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:
>
> > -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.
> > 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.
Cheers
---Dave
