From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Martin Subject: Re: [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized Date: Thu, 19 Apr 2018 09:26:57 +0100 Message-ID: <20180419082656.GK16308@e103592.cambridge.arm.com> References: <20180413184522.GD16308@e103592.cambridge.arm.com> <20180415131206.GR16141@n2100.armlinux.org.uk> <87604sa2fu.fsf_-_@xmission.com> <87zi248nte.fsf_-_@xmission.com> <20180417132328.GF16308@e103592.cambridge.arm.com> <87d0yxzksd.fsf@xmission.com> <20180418124717.GI16308@e103592.cambridge.arm.com> <87h8o8y4q6.fsf@xmission.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <87h8o8y4q6.fsf@xmission.com> Sender: linux-kernel-owner@vger.kernel.org To: "Eric W. Biederman" Cc: linux-arch@vger.kernel.org, Linus Torvalds , Linux Kernel Mailing List , "Dmitry V. Levin" , sparclinux , Russell King - ARM Linux , ppc-dev , linux-arm-kernel List-Id: linux-arch.vger.kernel.org On Wed, Apr 18, 2018 at 09:22:09AM -0500, Eric W. Biederman wrote: > Dave Martin writes: > > > On Tue, Apr 17, 2018 at 02:37:38PM -0500, Eric W. Biederman wrote: [...] > >> 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 Good point. I hadn't considered that only one class of signal comes from the arch code, but now that you point it out, it sounds right. > > 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 Makes sense. I wonder if all the relevant siginfo data could be passed to force_sig_fault() (or whatever) as arguments. Then the problem of uninitialised fields goes away. Perhaps that's what you had in mind. [...] > >> 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. I may follow it up later if I find myself at a loose end... Cheers ---Dave From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Martin Date: Thu, 19 Apr 2018 08:26:57 +0000 Subject: Re: [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized Message-Id: <20180419082656.GK16308@e103592.cambridge.arm.com> List-Id: References: <20180413184522.GD16308@e103592.cambridge.arm.com> <20180415131206.GR16141@n2100.armlinux.org.uk> <87604sa2fu.fsf_-_@xmission.com> <87zi248nte.fsf_-_@xmission.com> <20180417132328.GF16308@e103592.cambridge.arm.com> <87d0yxzksd.fsf@xmission.com> <20180418124717.GI16308@e103592.cambridge.arm.com> <87h8o8y4q6.fsf@xmission.com> In-Reply-To: <87h8o8y4q6.fsf@xmission.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: "Eric W. Biederman" Cc: linux-arch@vger.kernel.org, Linus Torvalds , Linux Kernel Mailing List , "Dmitry V. Levin" , sparclinux , Russell King - ARM Linux , ppc-dev , linux-arm-kernel On Wed, Apr 18, 2018 at 09:22:09AM -0500, Eric W. Biederman wrote: > Dave Martin writes: > > > On Tue, Apr 17, 2018 at 02:37:38PM -0500, Eric W. Biederman wrote: [...] > >> 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 Good point. I hadn't considered that only one class of signal comes from the arch code, but now that you point it out, it sounds right. > > 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 Makes sense. I wonder if all the relevant siginfo data could be passed to force_sig_fault() (or whatever) as arguments. Then the problem of uninitialised fields goes away. Perhaps that's what you had in mind. [...] > >> 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. I may follow it up later if I find myself at a loose end... Cheers ---Dave From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave.Martin@arm.com (Dave Martin) Date: Thu, 19 Apr 2018 09:26:57 +0100 Subject: [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized In-Reply-To: <87h8o8y4q6.fsf@xmission.com> References: <20180413184522.GD16308@e103592.cambridge.arm.com> <20180415131206.GR16141@n2100.armlinux.org.uk> <87604sa2fu.fsf_-_@xmission.com> <87zi248nte.fsf_-_@xmission.com> <20180417132328.GF16308@e103592.cambridge.arm.com> <87d0yxzksd.fsf@xmission.com> <20180418124717.GI16308@e103592.cambridge.arm.com> <87h8o8y4q6.fsf@xmission.com> Message-ID: <20180419082656.GK16308@e103592.cambridge.arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Apr 18, 2018 at 09:22:09AM -0500, Eric W. Biederman wrote: > Dave Martin writes: > > > On Tue, Apr 17, 2018 at 02:37:38PM -0500, Eric W. Biederman wrote: [...] > >> 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 Good point. I hadn't considered that only one class of signal comes from the arch code, but now that you point it out, it sounds right. > > 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 Makes sense. I wonder if all the relevant siginfo data could be passed to force_sig_fault() (or whatever) as arguments. Then the problem of uninitialised fields goes away. Perhaps that's what you had in mind. [...] > >> 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. I may follow it up later if I find myself at a loose end... Cheers ---Dave