Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1335955 > unrolled thread
| Started by | Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> |
|---|---|
| First post | 2016-02-17 01:30 +0100 |
| Last post | 2016-02-20 09:10 +0100 |
| Articles | 8 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH] tpm_crb: fix bad name pointer usage with struct resource Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-02-17 01:30 +0100
Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-02-17 06:00 +0100
Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-02-17 10:40 +0100
Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-02-17 15:30 +0100
Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-02-18 18:40 +0100
Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-02-19 16:10 +0100
Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-02-19 18:50 +0100
Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-02-20 09:10 +0100
| From | Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> |
|---|---|
| Date | 2016-02-17 01:30 +0100 |
| Subject | [PATCH] tpm_crb: fix bad name pointer usage with struct resource |
| Message-ID | <r2Yae-6Sl-13@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Jason Gunthorpe <jgunthorpe@obsidianresearch.com> |
|---|---|
| Date | 2016-02-17 06:00 +0100 |
| Subject | Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource |
| Message-ID | <r32nx-1k0-15@gated-at.bofh.it> |
| In reply to | #1335955 |
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
[toc] | [prev] | [next] | [standalone]
| From | Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> |
|---|---|
| Date | 2016-02-17 10:40 +0100 |
| Subject | Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource |
| Message-ID | <r36Ku-4qr-37@gated-at.bofh.it> |
| In reply to | #1336054 |
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
[toc] | [prev] | [next] | [standalone]
| From | Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> |
|---|---|
| Date | 2016-02-17 15:30 +0100 |
| Subject | Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource |
| Message-ID | <r3bh9-7Ch-29@gated-at.bofh.it> |
| In reply to | #1336172 |
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
[toc] | [prev] | [next] | [standalone]
| From | Jason Gunthorpe <jgunthorpe@obsidianresearch.com> |
|---|---|
| Date | 2016-02-18 18:40 +0100 |
| Subject | Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource |
| Message-ID | <r3AIy-yZ-29@gated-at.bofh.it> |
| In reply to | #1336482 |
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
[toc] | [prev] | [next] | [standalone]
| From | Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> |
|---|---|
| Date | 2016-02-19 16:10 +0100 |
| Subject | Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource |
| Message-ID | <r3UQV-6RV-5@gated-at.bofh.it> |
| In reply to | #1337571 |
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
[toc] | [prev] | [next] | [standalone]
| From | Jason Gunthorpe <jgunthorpe@obsidianresearch.com> |
|---|---|
| Date | 2016-02-19 18:50 +0100 |
| Subject | Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource |
| Message-ID | <r3XlL-hr-1@gated-at.bofh.it> |
| In reply to | #1338268 |
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
[toc] | [prev] | [next] | [standalone]
| From | Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> |
|---|---|
| Date | 2016-02-20 09:10 +0100 |
| Subject | Re: [tpmdd-devel] [PATCH] tpm_crb: fix bad name pointer usage with struct resource |
| Message-ID | <r4aM2-28b-21@gated-at.bofh.it> |
| In reply to | #1338383 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web