Skip to contentSkip to conversation

drivers: usb: gadget: udc: remove pointer dereference after free

PATCH Superseded Gustavo A. R. Silva 4 participants 10 messages Feb 8, 2017 – Mar 10, 2017 USB

lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH 1/1 drivers: usb: gadget: udc: remove pointer dereference after free

Remove pointer dereference and write after free.

Addresses-Coverity-ID: 1091173

Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/usb/gadget/udc/pch_udc.c | 1 -
 1 file changed, 1 deletion(-)
drivers/usb/gadget/udc/pch_udc.c +0 -1
index a97da64..8a365aa 100644
@@ -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;
Signature · 1 line
2.5.0
#1 lore raw
Quoted 1 line · Gustavo A. R. Silva

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.

Quoted 13 lines

Addresses-Coverity-ID: 1091173

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/usb/gadget/udc/pch_udc.c | 1 -

 1 file changed, 1 deletion(-)
drivers/usb/gadget/udc/pch_udc.c +0 -0
index a97da64..8a365aa 100644

drivers/usb/gadget/udc/pch_udc.c static void pch_udc_free_dma_chain(struct · line 1527

Quoted 5 lines
drivers/usb/gadget/udc/pch_udc.c +0 -1
@@ -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?

Quoted 3 lines
drivers/usb/gadget/udc/pch_udc.c +0 -0
  		addr = addr2;
  	}

  req->chain_len = 1;

Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy
author
#2 lore raw

Hi Andy,

Quoting Andy Shevchenko <[email protected]>:

Quoted 16 lines · Gustavo A. R. Silva
Quoted 1 line

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.

Quoted 7 lines

Addresses-Coverity-ID: 1091173

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/usb/gadget/udc/pch_udc.c | 1 -

 1 file changed, 1 deletion(-)
Quoted 15 lines

drivers/usb/gadget/udc/pch_udc.c static void pch_udc_free_dma_chain(struct · line 1527

Quoted 11 lines
drivers/usb/gadget/udc/pch_udc.c +0 -1
index a97da64..8a365aa 100644
@@ -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.

Quoted 9 lines

Is it done under some lock / serialization?

Quoted 3 lines
drivers/usb/gadget/udc/pch_udc.c +0 -0
  		addr = addr2;
  	}

  req->chain_len = 1;

--
Andy Shevchenko <[email protected]> Intel Finland Oy

Thanks
--
Gustavo A. R. Silva

author
#3 lore raw

PATCH v2 1/1 usb: gadget: udc: remove pointer dereference after free

Remove pointer dereference after free and set pointer to NULL after free.

Addresses-Coverity-ID: 1091173

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
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(-)
drivers/usb/gadget/udc/pch_udc.c +2 -1
index a97da64..73bb58f 100644
@@ -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;
Signature · 1 line
2.5.0
Acked-by on v2 1/1
#4 lore raw

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

Quoted 4 lines · 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 <[email protected]>
Acked-by Michal Nazarewicz <[email protected]>
Quoted 14 lines

---
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(-)
drivers/usb/gadget/udc/pch_udc.c +0 -0
index a97da64..73bb58f 100644

drivers/usb/gadget/udc/pch_udc.c static void pch_udc_free_dma_chain(struct · line 1526

Quoted 6 lines
drivers/usb/gadget/udc/pch_udc.c +0 -1
@@ -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.

drivers/usb/gadget/udc/pch_udc.c static void pch_udc_free_dma_chain(struct pch_udc_dev *dev, · line 1527

Quoted 2 lines
drivers/usb/gadget/udc/pch_udc.c +2 -0
+		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.

Quoted 3 lines
drivers/usb/gadget/udc/pch_udc.c +0 -0
  		addr = addr2;
  	}

  	req->chain_len = 1;
Signature · 3 lines
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
author
#5 lore raw

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

Hi Michal,

Quoting Michal Nazarewicz <[email protected]>:

Quoted 32 lines · Gustavo A. R. Silva
Quoted 4 lines

Remove pointer dereference after free and set pointer to NULL after free.

Addresses-Coverity-ID: 1091173

Signed-off-by Gustavo A. R. Silva <[email protected]>
Acked-by Michal Nazarewicz <[email protected]>
Quoted 14 lines

---
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(-)
drivers/usb/gadget/udc/pch_udc.c +0 -0
index a97da64..73bb58f 100644

drivers/usb/gadget/udc/pch_udc.c static void pch_udc_free_dma_chain(struct · line 1526

Quoted 6 lines
drivers/usb/gadget/udc/pch_udc.c +0 -1
@@ -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.

Quoted 10 lines

drivers/usb/gadget/udc/pch_udc.c static void pch_udc_free_dma_chain(struct pch_udc_dev *dev, · line 1527

Quoted 2 lines
drivers/usb/gadget/udc/pch_udc.c +2 -0
+		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.

Quoted 3 lines
drivers/usb/gadget/udc/pch_udc.c +0 -0
  		addr = addr2;
  	}

  	req->chain_len = 1;

Thanks for your comments, I will send version 3 shortly. --
Gustavo A. R. Silva

author
#6 lore raw

PATCH v3 1/1 usb: gadget: udc: remove pointer dereference after free

Remove pointer dereference after free.

Addresses-Coverity-ID: 1091173

Acked-by Michal Nazarewicz <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>

---
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(-)
drivers/usb/gadget/udc/pch_udc.c +0 -1
index a97da64..8a365aa 100644
@@ -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;
Signature · 1 line
2.5.0
#7 lore raw

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

Quoted 23 lines · Gustavo A. R. Silva

Remove pointer dereference after free.

Addresses-Coverity-ID: 1091173

Acked-by Michal Nazarewicz <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>

---
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(-)
drivers/usb/gadget/udc/pch_udc.c +0 -0
index a97da64..8a365aa 100644

drivers/usb/gadget/udc/pch_udc.c static void pch_udc_free_dma_chain(struct · line 1523

Quoted 1 line
drivers/usb/gadget/udc/pch_udc.c +0 -0
@@ -1523,7 +1523,6 @@ static void pch_udc_free_dma_chain(struct  
 pch_udc_dev *dev,

line wrapped. Can't apply.

Signature · 1 line
balbi
author
#8 lore raw

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

Hello,

Quoting Felipe Balbi <[email protected]>:

Quoted 19 lines
Quoted 17 lines · Gustavo A. R. Silva

Remove pointer dereference after free.

Addresses-Coverity-ID: 1091173

Acked-by Michal Nazarewicz <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>

---
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(-)
Quoted 10 lines

drivers/usb/gadget/udc/pch_udc.c static void pch_udc_free_dma_chain(struct · line 1523

Quoted 7 lines
drivers/usb/gadget/udc/pch_udc.c +0 -0
index a97da64..8a365aa 100644
@@ -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

author
#9 lore raw

PATCH v4 1/1 usb: gadget: udc: remove pointer dereference after free

Remove pointer dereference after free.

Addresses-Coverity-ID: 1091173

Acked-by Michal Nazarewicz <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>

---
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(-)
drivers/usb/gadget/udc/pch_udc.c +0 -1
index a97da64..8a365aa 100644
@@ -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;
Signature · 1 line
2.5.0