| Message ID | 1455668874-13261-1-git-send-email-jarkko.sakkinen@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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 ------------------------------------------------------------------------------ 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
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
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 ------------------------------------------------------------------------------ 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
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)
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(-)