[PATCH] tpm_crb: fix bad name pointer usage with struct resource

[PATCH] tpm_crb: fix bad name pointer usage with struct resource

[Jarkko Sakkinen]

The memory was not zeroed for new_res, which caused
devm_ioremap_resource() not to use dev_name() but instead whatever
garbage was pointed by new_res->name.

The problem crb_check_resource is different. There not zeroing the
name pointer causes use-after-free.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Fixes: 1bd047be37d9 ("tpm_crb: Use devm_ioremap_resource")
---
 drivers/char/tpm/tpm_crb.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 916332c..151689d 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -227,8 +227,10 @@ static int crb_check_resource(struct acpi_resource *ares, void *data)
 	struct crb_priv *priv = data;
 	struct resource res;
 
-	if (acpi_dev_resource_memory(ares, &res))
+	if (acpi_dev_resource_memory(ares, &res)) {
+		res.name = NULL;
 		priv->res = res;
+	}
 
 	return 1;
 }
@@ -236,11 +238,13 @@ static int crb_check_resource(struct acpi_resource *ares, void *data)
 static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
 				 u64 start, u32 size)
 {
-	struct resource new_res = {
-		.start	= start,
-		.end	= start + size - 1,
-		.flags	= IORESOURCE_MEM,
-	};
+	struct resource new_res;
+
+	memset(&new_res, 0, sizeof(new_res));
+
+	new_res.start	= start;
+	new_res.end	= start + size - 1;
+	new_res.flags	= IORESOURCE_MEM;
 
 	/* Detect a 64 bit address on a 32 bit system */
 	if (start != new_res.start)
-- 
2.7.0


------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

[PATCH] tpm_crb: fix bad name pointer usage with struct resource

[Jarkko Sakkinen]

The memory was not zeroed for new_res, which caused
devm_ioremap_resource() not to use dev_name() but instead whatever
garbage was pointed by new_res->name.

The problem crb_check_resource is different. There not zeroing the
name pointer causes use-after-free.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Fixes: 1bd047be37d9 ("tpm_crb: Use devm_ioremap_resource")
---
 drivers/char/tpm/tpm_crb.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 916332c..151689d 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -227,8 +227,10 @@ static int crb_check_resource(struct acpi_resource *ares, void *data)
 	struct crb_priv *priv = data;
 	struct resource res;
 
-	if (acpi_dev_resource_memory(ares, &res))
+	if (acpi_dev_resource_memory(ares, &res)) {
+		res.name = NULL;
 		priv->res = res;
+	}
 
 	return 1;
 }
@@ -236,11 +238,13 @@ static int crb_check_resource(struct acpi_resource *ares, void *data)
 static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
 				 u64 start, u32 size)
 {
-	struct resource new_res = {
-		.start	= start,
-		.end	= start + size - 1,
-		.flags	= IORESOURCE_MEM,
-	};
+	struct resource new_res;
+
+	memset(&new_res, 0, sizeof(new_res));
+
+	new_res.start	= start;
+	new_res.end	= start + size - 1;
+	new_res.flags	= IORESOURCE_MEM;
 
 	/* Detect a 64 bit address on a 32 bit system */
 	if (start != new_res.start)
-- 
2.7.0

Re: [PATCH] tpm_crb: fix bad name pointer usage with struct resource

[Jason Gunthorpe]

On Wed, Feb 17, 2016 at 02:27:54AM +0200, Jarkko Sakkinen wrote:
> -	if (acpi_dev_resource_memory(ares, &res))
> +	if (acpi_dev_resource_memory(ares, &res)) {
> +		res.name = NULL;

What? How is this not a bug in acpi_dev_resource_memory? Maybe it
needs to memcpy into devm allocated memory instead, but I'm confused
how/why/when acpi could free name.

The same code exists in tpm_tis as well.

>  {
> -	struct resource new_res = {
> -		.start	= start,
> -		.end	= start + size - 1,
> -		.flags	= IORESOURCE_MEM,
> -	};
> +	struct resource new_res;
> +
> +	memset(&new_res, 0, sizeof(new_res));
> +
> +	new_res.start	= start;
> +	new_res.end	= start + size - 1;
> +	new_res.flags	= IORESOURCE_MEM;

These two things are equivalent (C requires non-initialized members of
an initalized struct to be 0), why this change?

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource

[Jason Gunthorpe]

On Wed, Feb 17, 2016 at 02:27:54AM +0200, Jarkko Sakkinen wrote:
> -	if (acpi_dev_resource_memory(ares, &res))
> +	if (acpi_dev_resource_memory(ares, &res)) {
> +		res.name = NULL;

What? How is this not a bug in acpi_dev_resource_memory? Maybe it
needs to memcpy into devm allocated memory instead, but I'm confused
how/why/when acpi could free name.

The same code exists in tpm_tis as well.

>  {
> -	struct resource new_res = {
> -		.start	= start,
> -		.end	= start + size - 1,
> -		.flags	= IORESOURCE_MEM,
> -	};
> +	struct resource new_res;
> +
> +	memset(&new_res, 0, sizeof(new_res));
> +
> +	new_res.start	= start;
> +	new_res.end	= start + size - 1;
> +	new_res.flags	= IORESOURCE_MEM;

These two things are equivalent (C requires non-initialized members of
an initalized struct to be 0), why this change?

Jason

Re: [PATCH] tpm_crb: fix bad name pointer usage with struct resource

[Jarkko Sakkinen]

On Tue, Feb 16, 2016 at 09:52:19PM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 17, 2016 at 02:27:54AM +0200, Jarkko Sakkinen wrote:
> > -	if (acpi_dev_resource_memory(ares, &res))
> > +	if (acpi_dev_resource_memory(ares, &res)) {
> > +		res.name = NULL;
> 
> What? How is this not a bug in acpi_dev_resource_memory? Maybe it
> needs to memcpy into devm allocated memory instead, but I'm confused
> how/why/when acpi could free name.
> 
> The same code exists in tpm_tis as well.

That was the only way to fix the garbage issue. I would keep things
this way for Linux 4.5.

/Jarkko

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource

[Jarkko Sakkinen]

On Tue, Feb 16, 2016 at 09:52:19PM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 17, 2016 at 02:27:54AM +0200, Jarkko Sakkinen wrote:
> > -	if (acpi_dev_resource_memory(ares, &res))
> > +	if (acpi_dev_resource_memory(ares, &res)) {
> > +		res.name = NULL;
> 
> What? How is this not a bug in acpi_dev_resource_memory? Maybe it
> needs to memcpy into devm allocated memory instead, but I'm confused
> how/why/when acpi could free name.
> 
> The same code exists in tpm_tis as well.

That was the only way to fix the garbage issue. I would keep things
this way for Linux 4.5.

/Jarkko

Re: [PATCH] tpm_crb: fix bad name pointer usage with struct resource

[Jarkko Sakkinen]

On Wed, Feb 17, 2016 at 11:36:23AM +0200, Jarkko Sakkinen wrote:
> On Tue, Feb 16, 2016 at 09:52:19PM -0700, Jason Gunthorpe wrote:
> > On Wed, Feb 17, 2016 at 02:27:54AM +0200, Jarkko Sakkinen wrote:
> > > -	if (acpi_dev_resource_memory(ares, &res))
> > > +	if (acpi_dev_resource_memory(ares, &res)) {
> > > +		res.name = NULL;
> > 
> > What? How is this not a bug in acpi_dev_resource_memory? Maybe it
> > needs to memcpy into devm allocated memory instead, but I'm confused
> > how/why/when acpi could free name.
> > 
> > The same code exists in tpm_tis as well.
> 
> That was the only way to fix the garbage issue. I would keep things
> this way for Linux 4.5.

Hmm... Interesting with the machine where I have dTPM:

$ cat /proc/iomem|grep -A2 MSFT
fed40000-fed44fff : MSFT0101:00
  fed40000-fed44fff : 

Just an empty string.

Maybe for the release the safest bet would be anyway explicitly not
use the name field? That's the safest bet given the release time
frame.

/Jarkko

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource

[Jarkko Sakkinen]

On Wed, Feb 17, 2016 at 11:36:23AM +0200, Jarkko Sakkinen wrote:
> On Tue, Feb 16, 2016 at 09:52:19PM -0700, Jason Gunthorpe wrote:
> > On Wed, Feb 17, 2016 at 02:27:54AM +0200, Jarkko Sakkinen wrote:
> > > -	if (acpi_dev_resource_memory(ares, &res))
> > > +	if (acpi_dev_resource_memory(ares, &res)) {
> > > +		res.name = NULL;
> > 
> > What? How is this not a bug in acpi_dev_resource_memory? Maybe it
> > needs to memcpy into devm allocated memory instead, but I'm confused
> > how/why/when acpi could free name.
> > 
> > The same code exists in tpm_tis as well.
> 
> That was the only way to fix the garbage issue. I would keep things
> this way for Linux 4.5.

Hmm... Interesting with the machine where I have dTPM:

$ cat /proc/iomem|grep -A2 MSFT
fed40000-fed44fff : MSFT0101:00
  fed40000-fed44fff : 

Just an empty string.

Maybe for the release the safest bet would be anyway explicitly not
use the name field? That's the safest bet given the release time
frame.

/Jarkko

Re: [PATCH] tpm_crb: fix bad name pointer usage with struct resource

[Jason Gunthorpe]

On Wed, Feb 17, 2016 at 04:20:16PM +0200, Jarkko Sakkinen wrote:
> Maybe for the release the safest bet would be anyway explicitly not
> use the name field? That's the safest bet given the release time
> frame.

nulling it in the acpi paths of tis and crb, if you know those are broken
seems good for a rc.

I haven't looked at what other options there are, I suspect it needs
strdup due to how the drivers are working with acpi..

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource

[Jason Gunthorpe]

On Wed, Feb 17, 2016 at 04:20:16PM +0200, Jarkko Sakkinen wrote:
> Maybe for the release the safest bet would be anyway explicitly not
> use the name field? That's the safest bet given the release time
> frame.

nulling it in the acpi paths of tis and crb, if you know those are broken
seems good for a rc.

I haven't looked at what other options there are, I suspect it needs
strdup due to how the drivers are working with acpi..

Jason

Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource

[Jarkko Sakkinen]

On Thu, Feb 18, 2016 at 10:31:40AM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 17, 2016 at 04:20:16PM +0200, Jarkko Sakkinen wrote:
> > Maybe for the release the safest bet would be anyway explicitly not
> > use the name field? That's the safest bet given the release time
> > frame.
> 
> nulling it in the acpi paths of tis and crb, if you know those are broken
> seems good for a rc.
> 
> I haven't looked at what other options there are, I suspect it needs
> strdup due to how the drivers are working with acpi..

Can you quickly check the two top-most patches:

https://github.com/jsakkine/linux-tpmdd/commits/master

This is for 4.5 release.

> Jason

/Jarkko

Re: [PATCH] tpm_crb: fix bad name pointer usage with struct resource

[Jason Gunthorpe]

On Fri, Feb 19, 2016 at 05:06:06PM +0200, Jarkko Sakkinen wrote:

> Can you quickly check the two top-most patches:
> 
> https://github.com/jsakkine/linux-tpmdd/commits/master

Drop the change to crb_map_res, the memset is not needed.

The shutdown change is probably OK for a rc fix, but it is still wrong.
The shutdown has to be done inside the code after the core has removed
all access to the tpm but before it has torn down so much that the
driver doesn't work anymore.

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource

[Jason Gunthorpe]

On Fri, Feb 19, 2016 at 05:06:06PM +0200, Jarkko Sakkinen wrote:

> Can you quickly check the two top-most patches:
> 
> https://github.com/jsakkine/linux-tpmdd/commits/master

Drop the change to crb_map_res, the memset is not needed.

The shutdown change is probably OK for a rc fix, but it is still wrong.
The shutdown has to be done inside the code after the core has removed
all access to the tpm but before it has torn down so much that the
driver doesn't work anymore.

Jason

Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource

[Jarkko Sakkinen]

On Fri, Feb 19, 2016 at 10:44:34AM -0700, Jason Gunthorpe wrote:
> On Fri, Feb 19, 2016 at 05:06:06PM +0200, Jarkko Sakkinen wrote:
> 
> > Can you quickly check the two top-most patches:
> > 
> > https://github.com/jsakkine/linux-tpmdd/commits/master
> 
> Drop the change to crb_map_res, the memset is not needed.

Left-over from when I tried to catch the issue. Removed.

> The shutdown change is probably OK for a rc fix, but it is still wrong.
> The shutdown has to be done inside the code after the core has removed
> all access to the tpm but before it has torn down so much that the
> driver doesn't work anymore.

I made the modifications you asked and run my smoke tests. Can I apply
your Reviewed-by's?

> Jason

/Jarkko