Please pull my perf.git urgent branch

Please pull my perf.git urgent branch

[Paul Mackerras]

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>

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)) {
 			/*

Re: Please pull my perf.git urgent branch

[Scott Wood]

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

Re: Please pull my perf.git urgent branch

[Paul Mackerras]

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.

Re: Please pull my perf.git urgent branch

[Kumar Gala]

On Jul 27, 2010, at 11:47 PM, Paul Mackerras wrote:

> On Tue, Jul 27, 2010 at 11:28:54AM -0500, Scott Wood wrote:
>=20
>> Doesn't the setting of .period need to be maintained (it is in the =
other
>> powerpc perf_event implementation that this is derived from)?
>=20
> Gah, yes it does.
>=20
>> 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.
>=20
> So I misunderstood how an initializer for an automatic struct works.
> Brown paper bag time for me... :(
>=20
> 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.
>=20
>> CCing linuxppc-dev on the original patch would have been nice...
>=20
> True, but at least I can blame Peter Z. for that. :)
>=20
> 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=

Re: Please pull my perf.git urgent branch

[Scott Wood]

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

Re: Please pull my perf.git urgent branch

[Ingo Molnar]

* 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