[PATCH] drivers/perf: xgene_pmu: Fix uninitialized resource struct

[PATCH] drivers/perf: xgene_pmu: Fix uninitialized resource struct

[Mark Salter]

This splat was reported on newer Fedora kernels booting on certain
X-gene based machines:

 xgene-pmu APMC0D83:00: X-Gene PMU version 3
 Unable to handle kernel read from unreadable memory at virtual \
 address 0000000000004006
 ...
 Call trace:
  string+0x50/0x100
  vsnprintf+0x160/0x750
  devm_kvasprintf+0x5c/0xb4
  devm_kasprintf+0x54/0x60
  __devm_ioremap_resource+0xdc/0x1a0
  devm_ioremap_resource+0x14/0x20
  acpi_get_pmu_hw_inf.isra.0+0x84/0x15c
  acpi_pmu_dev_add+0xbc/0x21c
  acpi_ns_walk_namespace+0x16c/0x1e4
  acpi_walk_namespace+0xb4/0xfc
  xgene_pmu_probe_pmu_dev+0x7c/0xe0
  xgene_pmu_probe.part.0+0x2c0/0x310
  xgene_pmu_probe+0x54/0x64
  platform_drv_probe+0x60/0xb4
  really_probe+0xe8/0x4a0
  driver_probe_device+0xe4/0x100
  device_driver_attach+0xcc/0xd4
  __driver_attach+0xb0/0x17c
  bus_for_each_dev+0x6c/0xb0
  driver_attach+0x30/0x40
  bus_add_driver+0x154/0x250
  driver_register+0x84/0x140
  __platform_driver_register+0x54/0x60
  xgene_pmu_driver_init+0x28/0x34
  do_one_initcall+0x40/0x204
  do_initcalls+0x104/0x144
  kernel_init_freeable+0x198/0x210
  kernel_init+0x20/0x12c
  ret_from_fork+0x10/0x18
 Code: 91000400 110004e1 eb08009f 540000c0 (38646846)
 ---[ end trace f08c10566496a703 ]---

This was due to use of an uninitialized local resource struct in the xgene
pmu driver. A pointer to that struct gets to __devm_ioremap_resource()
where the name field is passed to devm_kasprintf() and dereferenced. The
struct was never initialized, so the name pointer is whatever happened to
be underlying it on the stack. This has been the case since the original
checkin of xgene_pmu.c, but there was a recent change to use the name field
in __devm_ioremap_resource() which revealed the problem.

Fixes: 832c927d119b ("perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver")
Signed-off-by: Mark Salter <msalter@redhat.com>
---
 drivers/perf/xgene_pmu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
index edac28cd25dd..fdbbd0804b92 100644
--- a/drivers/perf/xgene_pmu.c
+++ b/drivers/perf/xgene_pmu.c
@@ -1483,6 +1483,7 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
 		return NULL;
 
 	INIT_LIST_HEAD(&resource_list);
+	memset(&res, 0, sizeof(res));
 	rc = acpi_dev_get_resources(adev, &resource_list,
 				    acpi_pmu_dev_add_resource, &res);
 	acpi_dev_free_resource_list(&resource_list);
-- 
2.26.2

[PATCH] drivers/perf: xgene_pmu: Fix uninitialized resource struct

[Mark Salter]

This splat was reported on newer Fedora kernels booting on certain
X-gene based machines:

 xgene-pmu APMC0D83:00: X-Gene PMU version 3
 Unable to handle kernel read from unreadable memory at virtual \
 address 0000000000004006
 ...
 Call trace:
  string+0x50/0x100
  vsnprintf+0x160/0x750
  devm_kvasprintf+0x5c/0xb4
  devm_kasprintf+0x54/0x60
  __devm_ioremap_resource+0xdc/0x1a0
  devm_ioremap_resource+0x14/0x20
  acpi_get_pmu_hw_inf.isra.0+0x84/0x15c
  acpi_pmu_dev_add+0xbc/0x21c
  acpi_ns_walk_namespace+0x16c/0x1e4
  acpi_walk_namespace+0xb4/0xfc
  xgene_pmu_probe_pmu_dev+0x7c/0xe0
  xgene_pmu_probe.part.0+0x2c0/0x310
  xgene_pmu_probe+0x54/0x64
  platform_drv_probe+0x60/0xb4
  really_probe+0xe8/0x4a0
  driver_probe_device+0xe4/0x100
  device_driver_attach+0xcc/0xd4
  __driver_attach+0xb0/0x17c
  bus_for_each_dev+0x6c/0xb0
  driver_attach+0x30/0x40
  bus_add_driver+0x154/0x250
  driver_register+0x84/0x140
  __platform_driver_register+0x54/0x60
  xgene_pmu_driver_init+0x28/0x34
  do_one_initcall+0x40/0x204
  do_initcalls+0x104/0x144
  kernel_init_freeable+0x198/0x210
  kernel_init+0x20/0x12c
  ret_from_fork+0x10/0x18
 Code: 91000400 110004e1 eb08009f 540000c0 (38646846)
 ---[ end trace f08c10566496a703 ]---

This was due to use of an uninitialized local resource struct in the xgene
pmu driver. A pointer to that struct gets to __devm_ioremap_resource()
where the name field is passed to devm_kasprintf() and dereferenced. The
struct was never initialized, so the name pointer is whatever happened to
be underlying it on the stack. This has been the case since the original
checkin of xgene_pmu.c, but there was a recent change to use the name field
in __devm_ioremap_resource() which revealed the problem.

Fixes: 832c927d119b ("perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver")
Signed-off-by: Mark Salter <msalter@redhat.com>
---
 drivers/perf/xgene_pmu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
index edac28cd25dd..fdbbd0804b92 100644
--- a/drivers/perf/xgene_pmu.c
+++ b/drivers/perf/xgene_pmu.c
@@ -1483,6 +1483,7 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
 		return NULL;
 
 	INIT_LIST_HEAD(&resource_list);
+	memset(&res, 0, sizeof(res));
 	rc = acpi_dev_get_resources(adev, &resource_list,
 				    acpi_pmu_dev_add_resource, &res);
 	acpi_dev_free_resource_list(&resource_list);
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drivers/perf: xgene_pmu: Fix uninitialized resource struct

[Will Deacon]

On Wed, Sep 02, 2020 at 02:27:29PM -0400, Mark Salter wrote:
> diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
> index edac28cd25dd..fdbbd0804b92 100644
> --- a/drivers/perf/xgene_pmu.c
> +++ b/drivers/perf/xgene_pmu.c
> @@ -1483,6 +1483,7 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
>  		return NULL;
>  
>  	INIT_LIST_HEAD(&resource_list);
> +	memset(&res, 0, sizeof(res));
>  	rc = acpi_dev_get_resources(adev, &resource_list,
>  				    acpi_pmu_dev_add_resource, &res);
>  	acpi_dev_free_resource_list(&resource_list);

Hmm, to be honest, I'm not sure we should be calling devm_ioremap_resource()
at all here. The resource is clearly bogus, even with this change: the name
and the resource hierarchy pointers will all be NULL. I think it would be
better to follow the TX2 PMU driver (drivers/perf/thunderx2_pmu.c) which
appears to assign the resource directly in tx2_uncore_pmu_init_dev().

Is there a reason we can't do that?

Will

Re: [PATCH] drivers/perf: xgene_pmu: Fix uninitialized resource struct

[Will Deacon]

On Wed, Sep 02, 2020 at 02:27:29PM -0400, Mark Salter wrote:
> diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
> index edac28cd25dd..fdbbd0804b92 100644
> --- a/drivers/perf/xgene_pmu.c
> +++ b/drivers/perf/xgene_pmu.c
> @@ -1483,6 +1483,7 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
>  		return NULL;
>  
>  	INIT_LIST_HEAD(&resource_list);
> +	memset(&res, 0, sizeof(res));
>  	rc = acpi_dev_get_resources(adev, &resource_list,
>  				    acpi_pmu_dev_add_resource, &res);
>  	acpi_dev_free_resource_list(&resource_list);

Hmm, to be honest, I'm not sure we should be calling devm_ioremap_resource()
at all here. The resource is clearly bogus, even with this change: the name
and the resource hierarchy pointers will all be NULL. I think it would be
better to follow the TX2 PMU driver (drivers/perf/thunderx2_pmu.c) which
appears to assign the resource directly in tx2_uncore_pmu_init_dev().

Is there a reason we can't do that?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drivers/perf: xgene_pmu: Fix uninitialized resource struct

[Mark Salter]

On Mon, 2020-09-07 at 14:50 +0100, Will Deacon wrote:
> On Wed, Sep 02, 2020 at 02:27:29PM -0400, Mark Salter wrote:
> > diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
> > index edac28cd25dd..fdbbd0804b92 100644
> > --- a/drivers/perf/xgene_pmu.c
> > +++ b/drivers/perf/xgene_pmu.c
> > @@ -1483,6 +1483,7 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
> >  		return NULL;
> >  
> >  	INIT_LIST_HEAD(&resource_list);
> > +	memset(&res, 0, sizeof(res));
> >  	rc = acpi_dev_get_resources(adev, &resource_list,
> >  				    acpi_pmu_dev_add_resource, &res);
> >  	acpi_dev_free_resource_list(&resource_list);
> 
> Hmm, to be honest, I'm not sure we should be calling devm_ioremap_resource()
> at all here. The resource is clearly bogus, even with this change: the name
> and the resource hierarchy pointers will all be NULL. I think it would be
> better to follow the TX2 PMU driver (drivers/perf/thunderx2_pmu.c) which
> appears to assign the resource directly in tx2_uncore_pmu_init_dev().
> 
> Is there a reason we can't do that?
> 
> Will
> 

There's no difference between xgene and tx2 wrt resouce name/hierarchy.
They both call devm_ioresource_remap() which ends up setting the name
and hierarchy. The difference is that xgene calls acpi_dev_resource_memory()
directly from the acpi_dev_get_resources() callback. TX2 doesn't use such a
callback and in that case, acpi_dev_resource_memory() gets called with a
zeroed struct.

That said, changing xgene to avoid the callback like TX2 does would fix the
problem as well. If you'd rather go that route, I can send a patch for it.

Mark

Re: [PATCH] drivers/perf: xgene_pmu: Fix uninitialized resource struct

[Mark Salter]

On Mon, 2020-09-07 at 14:50 +0100, Will Deacon wrote:
> On Wed, Sep 02, 2020 at 02:27:29PM -0400, Mark Salter wrote:
> > diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
> > index edac28cd25dd..fdbbd0804b92 100644
> > --- a/drivers/perf/xgene_pmu.c
> > +++ b/drivers/perf/xgene_pmu.c
> > @@ -1483,6 +1483,7 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
> >  		return NULL;
> >  
> >  	INIT_LIST_HEAD(&resource_list);
> > +	memset(&res, 0, sizeof(res));
> >  	rc = acpi_dev_get_resources(adev, &resource_list,
> >  				    acpi_pmu_dev_add_resource, &res);
> >  	acpi_dev_free_resource_list(&resource_list);
> 
> Hmm, to be honest, I'm not sure we should be calling devm_ioremap_resource()
> at all here. The resource is clearly bogus, even with this change: the name
> and the resource hierarchy pointers will all be NULL. I think it would be
> better to follow the TX2 PMU driver (drivers/perf/thunderx2_pmu.c) which
> appears to assign the resource directly in tx2_uncore_pmu_init_dev().
> 
> Is there a reason we can't do that?
> 
> Will
> 

There's no difference between xgene and tx2 wrt resouce name/hierarchy.
They both call devm_ioresource_remap() which ends up setting the name
and hierarchy. The difference is that xgene calls acpi_dev_resource_memory()
directly from the acpi_dev_get_resources() callback. TX2 doesn't use such a
callback and in that case, acpi_dev_resource_memory() gets called with a
zeroed struct.

That said, changing xgene to avoid the callback like TX2 does would fix the
problem as well. If you'd rather go that route, I can send a patch for it.

Mark



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drivers/perf: xgene_pmu: Fix uninitialized resource struct

[Will Deacon]

On Tue, Sep 08, 2020 at 02:37:22PM -0400, Mark Salter wrote:
> On Mon, 2020-09-07 at 14:50 +0100, Will Deacon wrote:
> > On Wed, Sep 02, 2020 at 02:27:29PM -0400, Mark Salter wrote:
> > > diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
> > > index edac28cd25dd..fdbbd0804b92 100644
> > > --- a/drivers/perf/xgene_pmu.c
> > > +++ b/drivers/perf/xgene_pmu.c
> > > @@ -1483,6 +1483,7 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
> > >  		return NULL;
> > >  
> > >  	INIT_LIST_HEAD(&resource_list);
> > > +	memset(&res, 0, sizeof(res));
> > >  	rc = acpi_dev_get_resources(adev, &resource_list,
> > >  				    acpi_pmu_dev_add_resource, &res);
> > >  	acpi_dev_free_resource_list(&resource_list);
> > 
> > Hmm, to be honest, I'm not sure we should be calling devm_ioremap_resource()
> > at all here. The resource is clearly bogus, even with this change: the name
> > and the resource hierarchy pointers will all be NULL. I think it would be
> > better to follow the TX2 PMU driver (drivers/perf/thunderx2_pmu.c) which
> > appears to assign the resource directly in tx2_uncore_pmu_init_dev().
> > 
> > Is there a reason we can't do that?
> > 
> > Will
> > 
> 
> There's no difference between xgene and tx2 wrt resouce name/hierarchy.
> They both call devm_ioresource_remap() which ends up setting the name
> and hierarchy. The difference is that xgene calls acpi_dev_resource_memory()
> directly from the acpi_dev_get_resources() callback. TX2 doesn't use such a
> callback and in that case, acpi_dev_resource_memory() gets called with a
> zeroed struct.
> 
> That said, changing xgene to avoid the callback like TX2 does would fix the
> problem as well. If you'd rather go that route, I can send a patch for it.

Yes please

Will

Re: [PATCH] drivers/perf: xgene_pmu: Fix uninitialized resource struct

[Will Deacon]

On Tue, Sep 08, 2020 at 02:37:22PM -0400, Mark Salter wrote:
> On Mon, 2020-09-07 at 14:50 +0100, Will Deacon wrote:
> > On Wed, Sep 02, 2020 at 02:27:29PM -0400, Mark Salter wrote:
> > > diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
> > > index edac28cd25dd..fdbbd0804b92 100644
> > > --- a/drivers/perf/xgene_pmu.c
> > > +++ b/drivers/perf/xgene_pmu.c
> > > @@ -1483,6 +1483,7 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
> > >  		return NULL;
> > >  
> > >  	INIT_LIST_HEAD(&resource_list);
> > > +	memset(&res, 0, sizeof(res));
> > >  	rc = acpi_dev_get_resources(adev, &resource_list,
> > >  				    acpi_pmu_dev_add_resource, &res);
> > >  	acpi_dev_free_resource_list(&resource_list);
> > 
> > Hmm, to be honest, I'm not sure we should be calling devm_ioremap_resource()
> > at all here. The resource is clearly bogus, even with this change: the name
> > and the resource hierarchy pointers will all be NULL. I think it would be
> > better to follow the TX2 PMU driver (drivers/perf/thunderx2_pmu.c) which
> > appears to assign the resource directly in tx2_uncore_pmu_init_dev().
> > 
> > Is there a reason we can't do that?
> > 
> > Will
> > 
> 
> There's no difference between xgene and tx2 wrt resouce name/hierarchy.
> They both call devm_ioresource_remap() which ends up setting the name
> and hierarchy. The difference is that xgene calls acpi_dev_resource_memory()
> directly from the acpi_dev_get_resources() callback. TX2 doesn't use such a
> callback and in that case, acpi_dev_resource_memory() gets called with a
> zeroed struct.
> 
> That said, changing xgene to avoid the callback like TX2 does would fix the
> problem as well. If you'd rather go that route, I can send a patch for it.

Yes please

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel