| Message ID | 20100727124019.GB14947@brick.ozlabs.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, 27 Jul 2010 22:40:19 +1000 Paul Mackerras <paulus@samba.org> wrote: > Please do a pull from > > git://git.kernel.org/pub/scm/linux/kernel/git/paulus/perf.git urgent > > to get one commit that fixes a problem where, on some Freescale > embedded PowerPC machines, unprivileged userspace could oops the > kernel using the perf_event subsystem. I know it's late, but it is a > potential security hole (but only on Freescale embedded systems), the > fix is small (3 lines) and only affects Freescale embedded processors, > and I was on vacation for the past two weeks. :) [snip] > diff --git a/arch/powerpc/kernel/perf_event_fsl_emb.c b/arch/powerpc/kernel/perf_event_fsl_emb.c > index 369872f..babccee 100644 > --- a/arch/powerpc/kernel/perf_event_fsl_emb.c > +++ b/arch/powerpc/kernel/perf_event_fsl_emb.c > @@ -566,9 +566,9 @@ static void record_and_restart(struct perf_event *event, unsigned long val, > * Finally record data if requested. > */ > if (record) { > - struct perf_sample_data data = { > - .period = event->hw.last_period, > - }; > + struct perf_sample_data data; > + > + perf_sample_data_init(&data, 0); > > if (perf_event_overflow(event, nmi, &data, regs)) { > /* Doesn't the setting of .period need to be maintained (it is in the other powerpc perf_event implementation that this is derived from)? I don't see how this is a security fix -- the existing initializer above should zero-fill the fields that are not explicitly initialized. In fact, it's taking other fields that were previously initialized to zero and is making them uninitialized, since perf_sample_data_init only sets addr and raw. CCing linuxppc-dev on the original patch would have been nice... -Scott
On Tue, Jul 27, 2010 at 11:28:54AM -0500, Scott Wood wrote: > Doesn't the setting of .period need to be maintained (it is in the other > powerpc perf_event implementation that this is derived from)? Gah, yes it does. > I don't see how this is a security fix -- the existing initializer above > should zero-fill the fields that are not explicitly initialized. In fact, > it's taking other fields that were previously initialized to zero and is > making them uninitialized, since perf_sample_data_init only sets addr and > raw. So I misunderstood how an initializer for an automatic struct works. Brown paper bag time for me... :( Regarding the other fields, I assume Peter et al. have checked that they don't need to be cleared, so it's a microoptimization to not clear them. > CCing linuxppc-dev on the original patch would have been nice... True, but at least I can blame Peter Z. for that. :) Kumar and Ben, how do you want to proceed on this one? Paul.
On Jul 27, 2010, at 11:47 PM, Paul Mackerras wrote: > On Tue, Jul 27, 2010 at 11:28:54AM -0500, Scott Wood wrote: > >> Doesn't the setting of .period need to be maintained (it is in the other >> powerpc perf_event implementation that this is derived from)? > > Gah, yes it does. > >> I don't see how this is a security fix -- the existing initializer above >> should zero-fill the fields that are not explicitly initialized. In fact, >> it's taking other fields that were previously initialized to zero and is >> making them uninitialized, since perf_sample_data_init only sets addr and >> raw. > > So I misunderstood how an initializer for an automatic struct works. > Brown paper bag time for me... :( > > Regarding the other fields, I assume Peter et al. have checked that > they don't need to be cleared, so it's a microoptimization to not > clear them. > >> CCing linuxppc-dev on the original patch would have been nice... > > True, but at least I can blame Peter Z. for that. :) > > Kumar and Ben, how do you want to proceed on this one? If we aren't concerned about an oops being generated lets just submit a patch for 2.6.36. - k
On Wed, 28 Jul 2010 14:47:31 +1000 Paul Mackerras <paulus@samba.org> wrote: > On Tue, Jul 27, 2010 at 11:28:54AM -0500, Scott Wood wrote: > > > Doesn't the setting of .period need to be maintained (it is in the other > > powerpc perf_event implementation that this is derived from)? > > Gah, yes it does. Well, looks like Linus pulled anyway... I'll send a patch to add .period. -Scott
* Scott Wood <scottwood@freescale.com> wrote: > On Wed, 28 Jul 2010 14:47:31 +1000 > Paul Mackerras <paulus@samba.org> wrote: > > > On Tue, Jul 27, 2010 at 11:28:54AM -0500, Scott Wood wrote: > > > > > Doesn't the setting of .period need to be maintained (it is in the other > > > powerpc perf_event implementation that this is derived from)? > > > > Gah, yes it does. > > Well, looks like Linus pulled anyway... I'll send a patch to > add .period. Yes, the original commit was already upstream when you reported this bug. Paul added a -stable tag to the fix so it will get into the .35.1 pipeline this week. Thanks, Ingo
diff --git a/arch/powerpc/kernel/perf_event_fsl_emb.c b/arch/powerpc/kernel/perf_event_fsl_emb.c index 369872f..babccee 100644 --- a/arch/powerpc/kernel/perf_event_fsl_emb.c +++ b/arch/powerpc/kernel/perf_event_fsl_emb.c @@ -566,9 +566,9 @@ static void record_and_restart(struct perf_event *event, unsigned long val, * Finally record data if requested. */ if (record) { - struct perf_sample_data data = { - .period = event->hw.last_period, - }; + struct perf_sample_data data; + + perf_sample_data_init(&data, 0); if (perf_event_overflow(event, nmi, &data, regs)) { /*
Linus, Please do a pull from git://git.kernel.org/pub/scm/linux/kernel/git/paulus/perf.git urgent to get one commit that fixes a problem where, on some Freescale embedded PowerPC machines, unprivileged userspace could oops the kernel using the perf_event subsystem. I know it's late, but it is a potential security hole (but only on Freescale embedded systems), the fix is small (3 lines) and only affects Freescale embedded processors, and I was on vacation for the past two weeks. :) Thanks, Paul. Peter Zijlstra (1): perf, powerpc: Use perf_sample_data_init() for the FSL code arch/powerpc/kernel/perf_event_fsl_emb.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) commit 6b95ed345b9faa4ab3598a82991968f2e9f851bb Author: Peter Zijlstra <a.p.zijlstra@chello.nl> Date: Fri Jul 9 10:21:21 2010 +0200 perf, powerpc: Use perf_sample_data_init() for the FSL code We should use perf_sample_data_init() to initialize struct perf_sample_data. As explained in the description of commit dc1d628a ("perf: Provide generic perf_sample_data initialization"), it is possible for userspace to get the kernel to dereference data.raw, so if it is not initialized, that means that unprivileged userspace can possibly oops the kernel. Using perf_sample_data_init makes sure it gets initialized to NULL. This conversion should have been included in commit dc1d628a, but it got missed. Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> Acked-by: Kumar Gala <kumar.gala@freescale.com> Signed-off-by: Paul Mackerras <paulus@samba.org>