[PATCH] drivers: usb: gadget: udc: remove pointer dereference after free

[PATCH] drivers: usb: gadget: udc: remove pointer dereference after free

[Gustavo A. R. Silva]

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

Re: [PATCH] drivers: usb: gadget: udc: remove pointer dereference after free

[Andy Shevchenko]

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

Re: [PATCH] drivers: usb: gadget: udc: remove pointer dereference after free

[Gustavo A. R. Silva]

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

[PATCH v2] usb: gadget: udc: remove pointer dereference after free

[Gustavo A. R. Silva]

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

Re: [PATCH v2] usb: gadget: udc: remove pointer dereference after free

[Michal Nazarewicz]

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»

Re: [PATCH v2] usb: gadget: udc: remove pointer dereference after free

[Gustavo A. R. Silva]

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

[PATCH v3] usb: gadget: udc: remove pointer dereference after free

[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.

  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

Re: [PATCH v3] usb: gadget: udc: remove pointer dereference after free

[Felipe Balbi]

[-- 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 --]

Re: [PATCH v3] usb: gadget: udc: remove pointer dereference after free

[Gustavo A. R. Silva]

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

[PATCH v4] usb: gadget: udc: remove pointer dereference after free

[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