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 <[email protected]>
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 <[email protected]>
Acked-by: Kumar Gala <[email protected]>
Signed-off-by: Paul Mackerras <[email protected]>
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)) {
/*
On Tue, 27 Jul 2010 22:40:19 +1000
Paul Mackerras <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> On Wed, 28 Jul 2010 14:47:31 +1000
> Paul Mackerras <[email protected]> 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