* [PATCH] drivers: usb: gadget: udc: remove pointer dereference after free
@ 2017-02-08 19:15 Gustavo A. R. Silva
2017-02-08 19:37 ` Andy Shevchenko
0 siblings, 1 reply; 10+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-08 19:15 UTC (permalink / raw)
To: gregkh, balbi, andriy.shevchenko, bhelgaas, heikki.krogerus, mail,
mina86
Cc: linux-usb, linux-kernel
Remove pointer dereference and write after free.
Addresses-Coverity-ID: 1091173
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
drivers/usb/gadget/udc/pch_udc.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
index a97da64..8a365aa 100644
--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -1523,7 +1523,6 @@ static void pch_udc_free_dma_chain(struct pch_udc_dev *dev,
td = phys_to_virt(addr);
addr2 = (dma_addr_t)td->next;
pci_pool_free(dev->data_requests, td, addr);
- td->next = 0x00;
addr = addr2;
}
req->chain_len = 1;
--
2.5.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drivers: usb: gadget: udc: remove pointer dereference after free
2017-02-08 19:15 [PATCH] drivers: usb: gadget: udc: remove pointer dereference after free Gustavo A. R. Silva
@ 2017-02-08 19:37 ` Andy Shevchenko
2017-02-08 21:33 ` Gustavo A. R. Silva
0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2017-02-08 19:37 UTC (permalink / raw)
To: Gustavo A. R. Silva, gregkh, balbi, bhelgaas, heikki.krogerus,
mail, mina86
Cc: linux-usb, linux-kernel
On Wed, 2017-02-08 at 13:15 -0600, Gustavo A. R. Silva wrote:
> Remove pointer dereference and write after free.
It's wrong description. There is no write after free. The memory is
still in pool and one may access it. Though the access is *formally*
illegal.
Code itself looks interesting.
>
> Addresses-Coverity-ID: 1091173
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
> drivers/usb/gadget/udc/pch_udc.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/pch_udc.c
> b/drivers/usb/gadget/udc/pch_udc.c
> index a97da64..8a365aa 100644
> --- a/drivers/usb/gadget/udc/pch_udc.c
> +++ b/drivers/usb/gadget/udc/pch_udc.c
> @@ -1523,7 +1523,6 @@ static void pch_udc_free_dma_chain(struct
> pch_udc_dev *dev,
> td = phys_to_virt(addr);
> addr2 = (dma_addr_t)td->next;
> pci_pool_free(dev->data_requests, td, addr);
> - td->next = 0x00;
I think the better fix is to move this line before pci_pool_free() call.
I dunno those td->next = 0x00; make any sense there.
Is it done under some lock / serialization?
> addr = addr2;
> }
> req->chain_len = 1;
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drivers: usb: gadget: udc: remove pointer dereference after free
2017-02-08 19:37 ` Andy Shevchenko
@ 2017-02-08 21:33 ` Gustavo A. R. Silva
2017-02-11 17:07 ` [PATCH v2] " Gustavo A. R. Silva
0 siblings, 1 reply; 10+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-08 21:33 UTC (permalink / raw)
To: Andy Shevchenko
Cc: gregkh, balbi, bhelgaas, heikki.krogerus, mail, mina86, linux-usb,
linux-kernel, Peter Senna Tschudin
Hi Andy,
Quoting Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> On Wed, 2017-02-08 at 13:15 -0600, Gustavo A. R. Silva wrote:
>> Remove pointer dereference and write after free.
>
> It's wrong description. There is no write after free. The memory is
> still in pool and one may access it. Though the access is *formally*
> illegal.
>
> Code itself looks interesting.
>
>>
>> Addresses-Coverity-ID: 1091173
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> ---
>> drivers/usb/gadget/udc/pch_udc.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/udc/pch_udc.c
>> b/drivers/usb/gadget/udc/pch_udc.c
>> index a97da64..8a365aa 100644
>> --- a/drivers/usb/gadget/udc/pch_udc.c
>> +++ b/drivers/usb/gadget/udc/pch_udc.c
>> @@ -1523,7 +1523,6 @@ static void pch_udc_free_dma_chain(struct
>> pch_udc_dev *dev,
>> td = phys_to_virt(addr);
>> addr2 = (dma_addr_t)td->next;
>> pci_pool_free(dev->data_requests, td, addr);
>> - td->next = 0x00;
>
> I think the better fix is to move this line before pci_pool_free() call.
> I dunno those td->next = 0x00; make any sense there.
>
Yeah, I think it is better to do as you say.
It seems to me that the intention of that line of code is to get rid
of the address 'next' points to, after it has been backed up into
'addr2'.
I will also change the description.
> Is it done under some lock / serialization?
>
>> addr = addr2;
>> }
>> req->chain_len = 1;
>
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy
Thanks
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] usb: gadget: udc: remove pointer dereference after free
2017-02-08 21:33 ` Gustavo A. R. Silva
@ 2017-02-11 17:07 ` Gustavo A. R. Silva
2017-02-13 16:02 ` Michal Nazarewicz
0 siblings, 1 reply; 10+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-11 17:07 UTC (permalink / raw)
To: Andy Shevchenko
Cc: gregkh, balbi, heikki.krogerus, mail, mina86, linux-usb,
linux-kernel, Peter Senna Tschudin
Remove pointer dereference after free and set pointer to NULL after free.
Addresses-Coverity-ID: 1091173
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
Changes in v2:
Move pointer dereference before pci_pool_free()
Set pointer to NULL after free
drivers/usb/gadget/udc/pch_udc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/pch_udc.c
b/drivers/usb/gadget/udc/pch_udc.c
index a97da64..73bb58f 100644
--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -1522,8 +1522,9 @@ static void pch_udc_free_dma_chain(struct
pch_udc_dev *dev,
/* do not free first desc., will be done by free for request */
td = phys_to_virt(addr);
addr2 = (dma_addr_t)td->next;
- pci_pool_free(dev->data_requests, td, addr);
td->next = 0x00;
+ pci_pool_free(dev->data_requests, td, addr);
+ td = NULL;
addr = addr2;
}
req->chain_len = 1;
--
2.5.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] usb: gadget: udc: remove pointer dereference after free
2017-02-11 17:07 ` [PATCH v2] " Gustavo A. R. Silva
@ 2017-02-13 16:02 ` Michal Nazarewicz
2017-02-14 4:50 ` Gustavo A. R. Silva
0 siblings, 1 reply; 10+ messages in thread
From: Michal Nazarewicz @ 2017-02-13 16:02 UTC (permalink / raw)
To: Gustavo A. R. Silva, Andy Shevchenko
Cc: gregkh, balbi, heikki.krogerus, mail, linux-usb, linux-kernel,
Peter Senna Tschudin
On Sat, Feb 11 2017, Gustavo A. R. Silva wrote:
> Remove pointer dereference after free and set pointer to NULL after free.
>
> Addresses-Coverity-ID: 1091173
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
Acked-by: Michal Nazarewicz <mina86@mina86.com>
> ---
> Changes in v2:
> Move pointer dereference before pci_pool_free()
> Set pointer to NULL after free
>
> drivers/usb/gadget/udc/pch_udc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/pch_udc.c
> b/drivers/usb/gadget/udc/pch_udc.c
> index a97da64..73bb58f 100644
> --- a/drivers/usb/gadget/udc/pch_udc.c
> +++ b/drivers/usb/gadget/udc/pch_udc.c
> @@ -1522,8 +1522,9 @@ static void pch_udc_free_dma_chain(struct
> pch_udc_dev *dev,
> /* do not free first desc., will be done by free for request */
> td = phys_to_virt(addr);
> addr2 = (dma_addr_t)td->next;
> - pci_pool_free(dev->data_requests, td, addr);
> td->next = 0x00;
Or just drop this. pci_pool_free doesn’t care about contents of td.
It’s just a void* for it.
> + pci_pool_free(dev->data_requests, td, addr);
> + td = NULL;
This isn’t necessary either. td will get overwritten on next iteration
and once we’re done it’s not used again.
> addr = addr2;
> }
> req->chain_len = 1;
--
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] usb: gadget: udc: remove pointer dereference after free
2017-02-13 16:02 ` Michal Nazarewicz
@ 2017-02-14 4:50 ` Gustavo A. R. Silva
2017-02-14 4:53 ` [PATCH v3] " Gustavo A. R. Silva
0 siblings, 1 reply; 10+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-14 4:50 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Andy Shevchenko, gregkh, balbi, heikki.krogerus, mail, linux-usb,
linux-kernel, Peter Senna Tschudin
Hi Michal,
Quoting Michal Nazarewicz <mina86@mina86.com>:
> On Sat, Feb 11 2017, Gustavo A. R. Silva wrote:
>> Remove pointer dereference after free and set pointer to NULL after free.
>>
>> Addresses-Coverity-ID: 1091173
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
>
>> ---
>> Changes in v2:
>> Move pointer dereference before pci_pool_free()
>> Set pointer to NULL after free
>>
>> drivers/usb/gadget/udc/pch_udc.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/udc/pch_udc.c
>> b/drivers/usb/gadget/udc/pch_udc.c
>> index a97da64..73bb58f 100644
>> --- a/drivers/usb/gadget/udc/pch_udc.c
>> +++ b/drivers/usb/gadget/udc/pch_udc.c
>> @@ -1522,8 +1522,9 @@ static void pch_udc_free_dma_chain(struct
>> pch_udc_dev *dev,
>> /* do not free first desc., will be done by free for request */
>> td = phys_to_virt(addr);
>> addr2 = (dma_addr_t)td->next;
>> - pci_pool_free(dev->data_requests, td, addr);
>> td->next = 0x00;
>
> Or just drop this. pci_pool_free doesn’t care about contents of td.
> It’s just a void* for it.
>
>> + pci_pool_free(dev->data_requests, td, addr);
>> + td = NULL;
>
> This isn’t necessary either. td will get overwritten on next iteration
> and once we’re done it’s not used again.
>
>> addr = addr2;
>> }
>> req->chain_len = 1;
>
Thanks for your comments, I will send version 3 shortly.
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] usb: gadget: udc: remove pointer dereference after free
2017-02-14 4:50 ` Gustavo A. R. Silva
@ 2017-02-14 4:53 ` Gustavo A. R. Silva
2017-03-10 11:35 ` Felipe Balbi
0 siblings, 1 reply; 10+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-14 4:53 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Andy Shevchenko, gregkh, balbi, heikki.krogerus, mail, linux-usb,
linux-kernel, Peter Senna Tschudin
Remove pointer dereference after free.
Addresses-Coverity-ID: 1091173
Acked-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
Changes in v2:
Move pointer dereference before pci_pool_free()
Set pointer to NULL after free
Changes in v3:
Remove 'td->next = 0x00' inside for loop.
Remove unnecessary pointer nullification after free.
drivers/usb/gadget/udc/pch_udc.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/pch_udc.c
b/drivers/usb/gadget/udc/pch_udc.c
index a97da64..8a365aa 100644
--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -1523,7 +1523,6 @@ static void pch_udc_free_dma_chain(struct
pch_udc_dev *dev,
td = phys_to_virt(addr);
addr2 = (dma_addr_t)td->next;
pci_pool_free(dev->data_requests, td, addr);
- td->next = 0x00;
addr = addr2;
}
req->chain_len = 1;
--
2.5.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v3] usb: gadget: udc: remove pointer dereference after free
2017-02-14 4:53 ` [PATCH v3] " Gustavo A. R. Silva
@ 2017-03-10 11:35 ` Felipe Balbi
2017-03-10 21:20 ` Gustavo A. R. Silva
0 siblings, 1 reply; 10+ messages in thread
From: Felipe Balbi @ 2017-03-10 11:35 UTC (permalink / raw)
To: Gustavo A. R. Silva, Michal Nazarewicz
Cc: Andy Shevchenko, gregkh, heikki.krogerus, mail, linux-usb,
linux-kernel, Peter Senna Tschudin
[-- Attachment #1: Type: text/plain, Size: 918 bytes --]
"Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
> Remove pointer dereference after free.
>
> Addresses-Coverity-ID: 1091173
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
> Changes in v2:
> Move pointer dereference before pci_pool_free()
> Set pointer to NULL after free
>
> Changes in v3:
> Remove 'td->next = 0x00' inside for loop.
> Remove unnecessary pointer nullification after free.
>
> drivers/usb/gadget/udc/pch_udc.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/pch_udc.c
> b/drivers/usb/gadget/udc/pch_udc.c
> index a97da64..8a365aa 100644
> --- a/drivers/usb/gadget/udc/pch_udc.c
> +++ b/drivers/usb/gadget/udc/pch_udc.c
> @@ -1523,7 +1523,6 @@ static void pch_udc_free_dma_chain(struct
> pch_udc_dev *dev,
line wrapped. Can't apply.
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] usb: gadget: udc: remove pointer dereference after free
2017-03-10 11:35 ` Felipe Balbi
@ 2017-03-10 21:20 ` Gustavo A. R. Silva
2017-03-10 21:39 ` [PATCH v4] " Gustavo A. R. Silva
0 siblings, 1 reply; 10+ messages in thread
From: Gustavo A. R. Silva @ 2017-03-10 21:20 UTC (permalink / raw)
To: Felipe Balbi
Cc: Michal Nazarewicz, Andy Shevchenko, gregkh, heikki.krogerus, mail,
linux-usb, linux-kernel, Peter Senna Tschudin
Hello,
Quoting Felipe Balbi <balbi@kernel.org>:
> "Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
>
>> Remove pointer dereference after free.
>>
>> Addresses-Coverity-ID: 1091173
>> Acked-by: Michal Nazarewicz <mina86@mina86.com>
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> ---
>> Changes in v2:
>> Move pointer dereference before pci_pool_free()
>> Set pointer to NULL after free
>>
>> Changes in v3:
>> Remove 'td->next = 0x00' inside for loop.
>> Remove unnecessary pointer nullification after free.
>>
>> drivers/usb/gadget/udc/pch_udc.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/udc/pch_udc.c
>> b/drivers/usb/gadget/udc/pch_udc.c
>> index a97da64..8a365aa 100644
>> --- a/drivers/usb/gadget/udc/pch_udc.c
>> +++ b/drivers/usb/gadget/udc/pch_udc.c
>> @@ -1523,7 +1523,6 @@ static void pch_udc_free_dma_chain(struct
>> pch_udc_dev *dev,
>
> line wrapped. Can't apply.
>
I'll fix it right away.
Thanks
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4] usb: gadget: udc: remove pointer dereference after free
2017-03-10 21:20 ` Gustavo A. R. Silva
@ 2017-03-10 21:39 ` Gustavo A. R. Silva
0 siblings, 0 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2017-03-10 21:39 UTC (permalink / raw)
To: Felipe Balbi
Cc: Michal Nazarewicz, Andy Shevchenko, gregkh, heikki.krogerus, mail,
linux-usb, linux-kernel, Peter Senna Tschudin,
Gustavo A. R. Silva
Remove pointer dereference after free.
Addresses-Coverity-ID: 1091173
Acked-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
Changes in v2:
Move pointer dereference before pci_pool_free()
Set pointer to NULL after free
Changes in v3:
Remove 'td->next = 0x00' inside for loop.
Remove unnecessary pointer nullification after free.
Changes in v4:
Fix line-wrapping in previous patch.
drivers/usb/gadget/udc/pch_udc.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
index a97da64..8a365aa 100644
--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -1523,7 +1523,6 @@ static void pch_udc_free_dma_chain(struct pch_udc_dev *dev,
td = phys_to_virt(addr);
addr2 = (dma_addr_t)td->next;
pci_pool_free(dev->data_requests, td, addr);
- td->next = 0x00;
addr = addr2;
}
req->chain_len = 1;
--
2.5.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-10 21:39 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-08 19:15 [PATCH] drivers: usb: gadget: udc: remove pointer dereference after free Gustavo A. R. Silva
2017-02-08 19:37 ` Andy Shevchenko
2017-02-08 21:33 ` Gustavo A. R. Silva
2017-02-11 17:07 ` [PATCH v2] " Gustavo A. R. Silva
2017-02-13 16:02 ` Michal Nazarewicz
2017-02-14 4:50 ` Gustavo A. R. Silva
2017-02-14 4:53 ` [PATCH v3] " Gustavo A. R. Silva
2017-03-10 11:35 ` Felipe Balbi
2017-03-10 21:20 ` Gustavo A. R. Silva
2017-03-10 21:39 ` [PATCH v4] " Gustavo A. R. Silva
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.