Re: [PATCH v2] usb: storage: sddr55: Fix a possible null-pointer dereference in sddr55_transport()
Sorry, I forgot to send to Oliver, so send it again.
On 2019/7/29 19:49, Jia-Ju Bai wrote:
In sddr55_transport(), there is an if statement on line 836 to check
whether info->lba_to_pba is NULL:
if (info->lba_to_pba == NULL || ...)
When info->lba_to_pba is NULL, it is used on line 948:
pba = info->lba_to_pba[lba];
Thus, a possible null-pointer dereference may occur.
To fix this bug, info->lba_to_pba is checked before being used.
This bug is found by a static analysis tool STCheck written by us.
Signed-off-by: Jia-Ju Bai
---
v2:
* Avoid uninitialized access of pba.
Thank Oliver for helpful advice.
---
drivers/usb/storage/sddr55.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c
index b8527c55335b..d23aff16091e 100644
--- a/drivers/usb/storage/sddr55.c
+++ b/drivers/usb/storage/sddr55.c
@@ -945,7 +945,7 @@ static int sddr55_transport(struct scsi_cmnd *srb, struct
us_data *us)
return USB_STOR_TRANSPORT_FAILED;
}
- pba = info->lba_to_pba[lba];
+ pba = info->lba_to_pba ? info->lba_to_pba[lba] : 0;
if (srb->cmnd[0] == WRITE_10) {
usb_stor_dbg(us, "WRITE_10: write block %04X (LBA %04X) page
%01X pages %d\n",
[PATCH v2] usb: storage: sddr55: Fix a possible null-pointer dereference in sddr55_transport()
In sddr55_transport(), there is an if statement on line 836 to check
whether info->lba_to_pba is NULL:
if (info->lba_to_pba == NULL || ...)
When info->lba_to_pba is NULL, it is used on line 948:
pba = info->lba_to_pba[lba];
Thus, a possible null-pointer dereference may occur.
To fix this bug, info->lba_to_pba is checked before being used.
This bug is found by a static analysis tool STCheck written by us.
Signed-off-by: Jia-Ju Bai
---
v2:
* Avoid uninitialized access of pba.
Thank Oliver for helpful advice.
---
drivers/usb/storage/sddr55.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c
index b8527c55335b..d23aff16091e 100644
--- a/drivers/usb/storage/sddr55.c
+++ b/drivers/usb/storage/sddr55.c
@@ -945,7 +945,7 @@ static int sddr55_transport(struct scsi_cmnd *srb, struct
us_data *us)
return USB_STOR_TRANSPORT_FAILED;
}
- pba = info->lba_to_pba[lba];
+ pba = info->lba_to_pba ? info->lba_to_pba[lba] : 0;
if (srb->cmnd[0] == WRITE_10) {
usb_stor_dbg(us, "WRITE_10: write block %04X (LBA %04X)
page %01X pages %d\n",
--
2.17.0
Re: [PATCH] usb: storage: sddr55: Fix a possible null-pointer dereference in sddr55_transport()
On 2019/7/29 19:15, Oliver Neukum wrote: Am Montag, den 29.07.2019, 18:05 +0800 schrieb Jia-Ju Bai: Hi, In sddr55_transport(), there is an if statement on line 836 to check whether info->lba_to_pba is NULL: if (info->lba_to_pba == NULL || ...) When info->lba_to_pba is NULL, it is used on line 948: pba = info->lba_to_pba[lba]; Thus, a possible null-pointer dereference may occur. Yes, in practice READ_CAPACITY will always be called and set up the correct translation table, but you can probably exploit this. To fix this bug, info->lba_to_pba is checked before being used. This bug is found by a static analysis tool STCheck written by us. Signed-off-by: Jia-Ju Bai --- drivers/usb/storage/sddr55.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c index b8527c55335b..50afc39aa21d 100644 --- a/drivers/usb/storage/sddr55.c +++ b/drivers/usb/storage/sddr55.c @@ -945,7 +945,8 @@ static int sddr55_transport(struct scsi_cmnd *srb, struct us_data *us) return USB_STOR_TRANSPORT_FAILED; } - pba = info->lba_to_pba[lba]; + if (info->lba_to_pba) + pba = info->lba_to_pba[lba]; If you use that fix, pba will be uninitialized when used. It should be something like: pba = info->lba_to_pba ? info->lba_to_pba[lba] : 0; Thanks for the advice. I will send a v2 patch. Best wishes, Jia-Ju Bai
[PATCH] usb: storage: sddr55: Fix a possible null-pointer dereference in sddr55_transport()
In sddr55_transport(), there is an if statement on line 836 to check
whether info->lba_to_pba is NULL:
if (info->lba_to_pba == NULL || ...)
When info->lba_to_pba is NULL, it is used on line 948:
pba = info->lba_to_pba[lba];
Thus, a possible null-pointer dereference may occur.
To fix this bug, info->lba_to_pba is checked before being used.
This bug is found by a static analysis tool STCheck written by us.
Signed-off-by: Jia-Ju Bai
---
drivers/usb/storage/sddr55.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c
index b8527c55335b..50afc39aa21d 100644
--- a/drivers/usb/storage/sddr55.c
+++ b/drivers/usb/storage/sddr55.c
@@ -945,7 +945,8 @@ static int sddr55_transport(struct scsi_cmnd *srb, struct
us_data *us)
return USB_STOR_TRANSPORT_FAILED;
}
- pba = info->lba_to_pba[lba];
+ if (info->lba_to_pba)
+ pba = info->lba_to_pba[lba];
if (srb->cmnd[0] == WRITE_10) {
usb_stor_dbg(us, "WRITE_10: write block %04X (LBA %04X)
page %01X pages %d\n",
--
2.17.0
[PATCH] usb: musb: Fix a possible null-pointer dereference in musb_handle_intr_connect()
In musb_handle_intr_connect(), there is an if statement on line 783 to check whether musb->hcd is NULL: if (musb->hcd) When musb->hcd is NULL, it is used on line 797: musb_host_poke_root_hub(musb); if (musb->hcd->status_urb) Thus, a possible null-pointer dereference may occur. To fix this bug, musb->hcd is checked before calling musb_host_poke_root_hub(). This bug is found by a static analysis tool STCheck written by us. Signed-off-by: Jia-Ju Bai --- drivers/usb/musb/musb_core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c index 9f5a4819a744..329ff52f8167 100644 --- a/drivers/usb/musb/musb_core.c +++ b/drivers/usb/musb/musb_core.c @@ -794,7 +794,8 @@ static void musb_handle_intr_connect(struct musb *musb, u8 devctl, u8 int_usb) break; } - musb_host_poke_root_hub(musb); + if (musb->hcd) + musb_host_poke_root_hub(musb); musb_dbg(musb, "CONNECT (%s) devctl %02x", usb_otg_state_string(musb->xceiv->otg->state), devctl); -- 2.17.0
[PATCH] usb: xhci: Possible resource leaks when xhci_run() fails
xhci_pci_setup() is assigned to hc_driver.reset;
xhci_run() is assigned to hc_driver.start();
xhci_stop() is assigned to hc_driver.stop().
xhci_pci_setup() calls xhci_gen_setup, which calls xhci_init(). And
xhci_init() calls xhci_mem_init() to allocate resources.
xhci_stop() calls xhci_mem_cleanup(), to release the resources allocated
in xhci_mem_init() (also namely xhci_pci_setup()).
xhci_run() can fail, because xhci_try_enable_msi() or
xhci_alloc_command() in this function can fail.
In drivers/usb/core/hcd.c:
retval = hcd->driver->reset(hcd);
if (retval < 0) {
..
goto err_hcd_driver_setup;
}
..
retval = hcd->driver->start(hcd);
if (retval < 0) {
..
goto err_hcd_driver_start;
}
...
hcd->driver->stop(hcd);
hcd->state = HC_STATE_HALT;
clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
del_timer_sync(&hcd->rh_timer);
err_hcd_driver_start:
if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
free_irq(irqnum, hcd);
err_request_irq:
err_hcd_driver_setup:
err_set_rh_speed:
usb_put_invalidate_rhdev(hcd);
err_allocate_root_hub:
usb_deregister_bus(&hcd->self);
err_register_bus:
hcd_buffer_destroy(hcd);
err_create_buf:
usb_phy_roothub_power_off(hcd->phy_roothub);
err_usb_phy_roothub_power_on:
usb_phy_roothub_exit(hcd->phy_roothub);
Thus, when hcd->driver->reset() succeeds and hcd->driver->start() fails,
hcd->driver->stop() is not called.
Namely, when xhci_pci_setup() successfully allocates resources, and
xhci_run() fails, xhci_stop() is not called to release the resources.
For this reason, resource leaks occur in this case.
The ehci driver, uhci driver and ohci driver do not have such bugs:
In the ehci driver, ehci_run() (namely hcd->driver->start()) never
fails.
In the uhci driver, all the resources are allocated in uhci_start
(namely hcd->driver->start()), and no resources are allocated in
uhci_pci_init() (namely hcd->driver->reset()).
In the ohci driver, ohci_setup() (namely hcd->driver->reset()) also
allocates resources. But when ohci_start() (namely hcd->driver->start())
is going to fail, ohci_stop() is directly called to release the
resources allocated by ohci_setup().
Referring to the ohci driver, to fix these resource leaks,
xhci_mem_cleanup() is called in error handling code of xhci_run(),
to release the allocated resources.
These bugs are found by a runtime fuzzing tool named FIZZER written by
us.
Signed-off-by: Jia-Ju Bai
---
drivers/usb/host/xhci.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 7fa58c99f126..d18893cf03cb 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -651,8 +651,10 @@ int xhci_run(struct usb_hcd *hcd)
xhci_dbg_trace(xhci, trace_xhci_dbg_init, "xhci_run");
ret = xhci_try_enable_msi(hcd);
- if (ret)
+ if (ret) {
+ xhci_mem_cleanup(xhci);
return ret;
+ }
temp_64 = xhci_read_64(xhci, &xhci->ir_set->erst_dequeue);
temp_64 &= ~ERST_PTR_MASK;
@@ -683,8 +685,10 @@ int xhci_run(struct usb_hcd *hcd)
struct xhci_command *command;
command = xhci_alloc_command(xhci, false, GFP_KERNEL);
- if (!command)
+ if (!command) {
+ xhci_mem_cleanup(xhci);
return -ENOMEM;
+ }
ret = xhci_queue_vendor_command(xhci, command, 0, 0, 0,
TRB_TYPE(TRB_NEC_GET_FW));
--
2.17.0
Re: [BUG] usb: xhci: Possible resource leaks when xhci_run() fails
On 2019/5/15 0:55, Greg KH wrote:
On Tue, May 14, 2019 at 10:58:05PM +0800, Jia-Ju Bai wrote:
xhci_pci_setup() is assigned to hc_driver.reset;
xhci_run() is assigned to hc_driver.start();
xhci_stop() is assigned to hc_driver.stop().
xhci_pci_setup() calls xhci_gen_setup, which calls xhci_init(). And
xhci_init() calls xhci_mem_init() to allocate resources.
xhci_stop() calls xhci_mem_cleanup(), to release the resources allocated in
xhci_mem_init() (also namely xhci_pci_setup()).
xhci_run() can fail, because xhci_try_enable_msi() or xhci_alloc_command()
in this function can fail.
In drivers/usb/core/hcd.c:
retval = hcd->driver->reset(hcd);
if (retval < 0) {
..
goto err_hcd_driver_setup;
}
..
retval = hcd->driver->start(hcd);
if (retval < 0) {
..
goto err_hcd_driver_start;
}
...
hcd->driver->stop(hcd);
hcd->state = HC_STATE_HALT;
clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
del_timer_sync(&hcd->rh_timer);
err_hcd_driver_start:
if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
free_irq(irqnum, hcd);
err_request_irq:
err_hcd_driver_setup:
err_set_rh_speed:
usb_put_invalidate_rhdev(hcd);
err_allocate_root_hub:
usb_deregister_bus(&hcd->self);
err_register_bus:
hcd_buffer_destroy(hcd);
err_create_buf:
usb_phy_roothub_power_off(hcd->phy_roothub);
err_usb_phy_roothub_power_on:
usb_phy_roothub_exit(hcd->phy_roothub);
Thus, when hcd->driver->reset() succeeds and hcd->driver->start() fails,
hcd->driver->stop() is not called.
Namely, when xhci_pci_setup() successfully allocates resources, and
xhci_run() fails, xhci_stop() is not called to release the resources.
For this reason, resource leaks occur in this case.
I check the code of the ehci driver, uhci driver and ohci driver, and find
that they do not have such problem, because:
In the ehci driver, ehci_run() (namely hcd->driver->start()) never fails.
In the uhci driver, all the resources are allocated in uhci_start (namely
hcd->driver->start()), and no resources are allocated in uhci_pci_init()
(namely hcd->driver->reset()).
In the ohci driver, ohci_setup() (namely hcd->driver->reset()) also
allocates resources. But when ohci_start() (namely hcd->driver->start()) is
going to fail, ohci_stop() is directly called to release the resources
allocated by ohci_setup().
Thus, there are two possible ways of fixing bugs:
1) Call xhci_stop() when xhci_run() is going to fail (like the ohci driver)
2) Move all resource-allocation operations into xhci_run() (like the uhci
driver).
I am not sure whether these ways are correct, so I only report bugs.
Can you create a patch to show how you would fix this potential issue?
Given that making this type of thing fail is pretty rare, it's not a
real high priority to get to, so it might be a while for anyone here to
look at it.
Okay, I will send a patch soon.
Best wishes,
Jia-Ju Bai
[BUG] usb: xhci: Possible resource leaks when xhci_run() fails
xhci_pci_setup() is assigned to hc_driver.reset;
xhci_run() is assigned to hc_driver.start();
xhci_stop() is assigned to hc_driver.stop().
xhci_pci_setup() calls xhci_gen_setup, which calls xhci_init(). And
xhci_init() calls xhci_mem_init() to allocate resources.
xhci_stop() calls xhci_mem_cleanup(), to release the resources allocated
in xhci_mem_init() (also namely xhci_pci_setup()).
xhci_run() can fail, because xhci_try_enable_msi() or
xhci_alloc_command() in this function can fail.
In drivers/usb/core/hcd.c:
retval = hcd->driver->reset(hcd);
if (retval < 0) {
..
goto err_hcd_driver_setup;
}
..
retval = hcd->driver->start(hcd);
if (retval < 0) {
..
goto err_hcd_driver_start;
}
...
hcd->driver->stop(hcd);
hcd->state = HC_STATE_HALT;
clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
del_timer_sync(&hcd->rh_timer);
err_hcd_driver_start:
if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
free_irq(irqnum, hcd);
err_request_irq:
err_hcd_driver_setup:
err_set_rh_speed:
usb_put_invalidate_rhdev(hcd);
err_allocate_root_hub:
usb_deregister_bus(&hcd->self);
err_register_bus:
hcd_buffer_destroy(hcd);
err_create_buf:
usb_phy_roothub_power_off(hcd->phy_roothub);
err_usb_phy_roothub_power_on:
usb_phy_roothub_exit(hcd->phy_roothub);
Thus, when hcd->driver->reset() succeeds and hcd->driver->start() fails,
hcd->driver->stop() is not called.
Namely, when xhci_pci_setup() successfully allocates resources, and
xhci_run() fails, xhci_stop() is not called to release the resources.
For this reason, resource leaks occur in this case.
I check the code of the ehci driver, uhci driver and ohci driver, and
find that they do not have such problem, because:
In the ehci driver, ehci_run() (namely hcd->driver->start()) never fails.
In the uhci driver, all the resources are allocated in uhci_start
(namely hcd->driver->start()), and no resources are allocated in
uhci_pci_init() (namely hcd->driver->reset()).
In the ohci driver, ohci_setup() (namely hcd->driver->reset()) also
allocates resources. But when ohci_start() (namely hcd->driver->start())
is going to fail, ohci_stop() is directly called to release the
resources allocated by ohci_setup().
Thus, there are two possible ways of fixing bugs:
1) Call xhci_stop() when xhci_run() is going to fail (like the ohci driver)
2) Move all resource-allocation operations into xhci_run() (like the
uhci driver).
I am not sure whether these ways are correct, so I only report bugs.
These bugs are found by a runtime fuzzing tool named FIZZER written by us.
Best wishes,
Jia-Ju Bai
Re: [BUG] usb: serial: garmin_gps: A possible concurrency use-after-free bug
On 2018/12/20 21:46, Johan Hovold wrote: On Thu, Dec 20, 2018 at 09:41:16PM +0800, Jia-Ju Bai wrote: In drivers/usb/serial/garmin_gps.c, the functions garmin_read_bulk_callback() and garmin_write_bulk_callback() may be concurrently executed. In garmin_write_bulk_callback() on line 969: kfree(urb->transfer_buffer); In garmin_read_bulk_callback() on line 1165: unsigned char *data = urb->transfer_buffer; Thus, a concurrency use-after-free bug may occur. No, they operate on different struct urb. This possible bug is found by a static analysis tool written by myself. Seems you need to update your tool. Please also make sure to review its output before reporting anything. Okay, thanks for your reply. Sorry for my false positive... Best wishes, Jia-Ju Bai
[BUG] usb: serial: garmin_gps: A possible concurrency use-after-free bug
In drivers/usb/serial/garmin_gps.c, the functions garmin_read_bulk_callback() and garmin_write_bulk_callback() may be concurrently executed. In garmin_write_bulk_callback() on line 969: kfree(urb->transfer_buffer); In garmin_read_bulk_callback() on line 1165: unsigned char *data = urb->transfer_buffer; Thus, a concurrency use-after-free bug may occur. This possible bug is found by a static analysis tool written by myself. Best wishes, Jia-Ju Bai
[PATCH v2] usb: r8a66597: Fix a possible concurrency use-after-free bug in r8a66597_endpoint_disable()
The function r8a66597_endpoint_disable() and r8a66597_urb_enqueue() may
be concurrently executed.
The two functions both access a possible shared variable "hep->hcpriv".
This shared variable is freed by r8a66597_endpoint_disable() via the
call path:
r8a66597_endpoint_disable
kfree(hep->hcpriv) (line 1995 in Linux-4.19)
This variable is read by r8a66597_urb_enqueue() via the call path:
r8a66597_urb_enqueue
spin_lock_irqsave(&r8a66597->lock)
init_pipe_info
enable_r8a66597_pipe
pipe = hep->hcpriv (line 802 in Linux-4.19)
The read operation is protected by a spinlock, but the free operation
is not protected by this spinlock, thus a concurrency use-after-free bug
may occur.
To fix this bug, the spin-lock and spin-unlock function calls in
r8a66597_endpoint_disable() are moved to protect the free operation.
Signed-off-by: Jia-Ju Bai
---
v2:
* Add __acquires/__releases markings for r8a66597_endpoint_disable().
Thanks Greg for good advice.
---
drivers/usb/host/r8a66597-hcd.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
index 984892dd72f5..42668aeca57c 100644
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -1979,6 +1979,8 @@ static int r8a66597_urb_dequeue(struct usb_hcd *hcd,
struct urb *urb,
static void r8a66597_endpoint_disable(struct usb_hcd *hcd,
struct usb_host_endpoint *hep)
+__acquires(r8a66597->lock)
+__releases(r8a66597->lock)
{
struct r8a66597 *r8a66597 = hcd_to_r8a66597(hcd);
struct r8a66597_pipe *pipe = (struct r8a66597_pipe *)hep->hcpriv;
@@ -1991,13 +1993,14 @@ static void r8a66597_endpoint_disable(struct usb_hcd
*hcd,
return;
pipenum = pipe->info.pipenum;
+ spin_lock_irqsave(&r8a66597->lock, flags);
if (pipenum == 0) {
kfree(hep->hcpriv);
hep->hcpriv = NULL;
+ spin_unlock_irqrestore(&r8a66597->lock, flags);
return;
}
- spin_lock_irqsave(&r8a66597->lock, flags);
pipe_stop(r8a66597, pipe);
pipe_irq_disable(r8a66597, pipenum);
disable_irq_empty(r8a66597, pipenum);
--
2.17.0
Re: [PATCH] r8a66597: Fix a possible concurrency use-after-free bug in r8a66597_endpoint_disable()
On 2018/12/18 19:11, Greg KH wrote: On Tue, Dec 18, 2018 at 06:00:20PM +0800, Jia-Ju Bai wrote: The function r8a66597_endpoint_disable() and r8a66597_urb_enqueue() may be concurrently executed. The two functions both access a possible shared variable "hep->hcpriv". This shared variable is freed by r8a66597_endpoint_disable() via the call path: r8a66597_endpoint_disable kfree(hep->hcpriv) (line 1995 in Linux-4.19) This variable is read by r8a66597_urb_enqueue() via the call path: r8a66597_urb_enqueue spin_lock_irqsave(&r8a66597->lock); init_pipe_info enable_r8a66597_pipe pipe = hep->hcpriv (line 802 in Linux-4.19) The read operation is protected by a spinlock, but the free operation is not protected by this spinlock, thus a concurrency use-after-free bug may occur. To fix this bug, the spin-lock and spin-unlock function calls in r8a66597_endpoint_disable() are moved to protect the free operation. Signed-off-by: Jia-Ju Bai --- drivers/usb/host/r8a66597-hcd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c index 984892dd72f5..1495ce14ad22 100644 --- a/drivers/usb/host/r8a66597-hcd.c +++ b/drivers/usb/host/r8a66597-hcd.c @@ -1991,13 +1991,14 @@ static void r8a66597_endpoint_disable(struct usb_hcd *hcd, return; pipenum = pipe->info.pipenum; + spin_lock_irqsave(&r8a66597->lock, flags); Don't you also need the __aquires/__releases markings on this function in order to properly annotate it, like the rest of the driver has? Okay, thanks for this suggestion :) I will send a v2 patch. Best wishes, Jia-Ju Bai
[PATCH] r8a66597: Fix a possible concurrency use-after-free bug in r8a66597_endpoint_disable()
The function r8a66597_endpoint_disable() and r8a66597_urb_enqueue() may
be concurrently executed.
The two functions both access a possible shared variable "hep->hcpriv".
This shared variable is freed by r8a66597_endpoint_disable() via the
call path:
r8a66597_endpoint_disable
kfree(hep->hcpriv) (line 1995 in Linux-4.19)
This variable is read by r8a66597_urb_enqueue() via the call path:
r8a66597_urb_enqueue
spin_lock_irqsave(&r8a66597->lock);
init_pipe_info
enable_r8a66597_pipe
pipe = hep->hcpriv (line 802 in Linux-4.19)
The read operation is protected by a spinlock, but the free operation
is not protected by this spinlock, thus a concurrency use-after-free bug
may occur.
To fix this bug, the spin-lock and spin-unlock function calls in
r8a66597_endpoint_disable() are moved to protect the free operation.
Signed-off-by: Jia-Ju Bai
---
drivers/usb/host/r8a66597-hcd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
index 984892dd72f5..1495ce14ad22 100644
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -1991,13 +1991,14 @@ static void r8a66597_endpoint_disable(struct usb_hcd
*hcd,
return;
pipenum = pipe->info.pipenum;
+ spin_lock_irqsave(&r8a66597->lock, flags);
if (pipenum == 0) {
kfree(hep->hcpriv);
hep->hcpriv = NULL;
+ spin_unlock_irqrestore(&r8a66597->lock, flags);
return;
}
- spin_lock_irqsave(&r8a66597->lock, flags);
pipe_stop(r8a66597, pipe);
pipe_irq_disable(r8a66597, pipenum);
disable_irq_empty(r8a66597, pipenum);
--
2.17.0
[PATCH] usb: gadget: udc: fotg210-udc: Fix a sleep-in-atomic-context bug in fotg210_get_status()
The driver may sleep in an interrupt handler. The function call path (from bottom to top) in Linux-4.17 is: [FUNC] fotg210_ep_queue(GFP_KERNEL) drivers/usb/gadget/udc/fotg210-udc.c, 744: fotg210_ep_queue in fotg210_get_status drivers/usb/gadget/udc/fotg210-udc.c, 768: fotg210_get_status in fotg210_setup_packet drivers/usb/gadget/udc/fotg210-udc.c, 949: fotg210_setup_packet in fotg210_irq (interrupt handler) To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC. If possible, spin_unlock() and spin_lock() around fotg210_ep_queue() can be also removed. This bug is found by my static analysis tool DSAC. Signed-off-by: Jia-Ju Bai --- drivers/usb/gadget/udc/fotg210-udc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/udc/fotg210-udc.c b/drivers/usb/gadget/udc/fotg210-udc.c index 53a48f561458..c51510803d1f 100644 --- a/drivers/usb/gadget/udc/fotg210-udc.c +++ b/drivers/usb/gadget/udc/fotg210-udc.c @@ -741,7 +741,7 @@ static void fotg210_get_status(struct fotg210_udc *fotg210, fotg210->ep0_req->length = 2; spin_unlock(&fotg210->lock); - fotg210_ep_queue(fotg210->gadget.ep0, fotg210->ep0_req, GFP_KERNEL); + fotg210_ep_queue(fotg210->gadget.ep0, fotg210->ep0_req, GFP_ATOMIC); spin_lock(&fotg210->lock); } -- 2.17.0
Re: [PATCH v2] usb: cdc-wdm: Fix a sleep-in-atomic-context bug in service_outstanding_interrupt()
On 2018/9/11 15:49, Sebastian Andrzej Siewior wrote: On 2018-09-01 16:12:10 [+0800], Jia-Ju Bai wrote: wdm_in_callback() is a completion handler function for the USB driver. So it should not sleep. But it calls service_outstanding_interrupt(), which calls usb_submit_urb() with GFP_KERNEL. At which point does wdm_in_callback() invoke service_outstanding_interrupt()? I don't see it. I see one invocation from wdm_read() and another from service_interrupt_work(). Also, if that would be the case, then spin_unlock_irq() in an USB completion handler (which might run in IRQ context with interrupts disabled) would be wrong. Yes, you are right. I checked an old kernel version Linux-4.16 and got this report. The current code looks much different from the code that I checked. Sorry for my false report, and thanks for your correction. Best wishes, Jia-Ju Bai
[PATCH] usb: host: u132-hcd: Fix a sleep-in-atomic-context bug in u132_get_frame()
i_usX2Y_subs_startup in usbusx2yaudio.c is a completion handler function
for the USB driver. So it should not sleep, but it is can sleep
according to the function call paths (from bottom to top) in Linux-4.16.
[FUNC] msleep
drivers/usb/host/u132-hcd.c, 2558:
msleep in u132_get_frame
drivers/usb/core/hcd.c, 2231:
[FUNC_PTR]u132_get_frame in usb_hcd_get_frame_number
drivers/usb/core/usb.c, 822:
usb_hcd_get_frame_number in usb_get_current_frame_number
sound/usb/usx2y/usbusx2yaudio.c, 303:
usb_get_current_frame_number in i_usX2Y_urb_complete
sound/usb/usx2y/usbusx2yaudio.c, 366:
i_usX2Y_urb_complete in i_usX2Y_subs_startup
Note that [FUNC_PTR] means a function pointer call is used.
To fix this bug, msleep() is replaced with mdelay().
This bug is found by my static analysis tool DSAC.
Signed-off-by: Jia-Ju Bai
---
drivers/usb/host/u132-hcd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/u132-hcd.c b/drivers/usb/host/u132-hcd.c
index 032b8652910a..02f8e08b3ee8 100644
--- a/drivers/usb/host/u132-hcd.c
+++ b/drivers/usb/host/u132-hcd.c
@@ -2555,7 +2555,7 @@ static int u132_get_frame(struct usb_hcd *hcd)
} else {
int frame = 0;
dev_err(&u132->platform_dev->dev, "TODO: u132_get_frame\n");
- msleep(100);
+ mdelay(100);
return frame;
}
}
--
2.17.0
[PATCH] usb: misc: uss720: Fix two sleep-in-atomic-context bugs
async_complete() in uss720.c is a completion handler function for the
USB driver. So it should not sleep, but it is can sleep according to the
function call paths (from bottom to top) in Linux-4.16.
[FUNC] set_1284_register(GFP_KERNEL)
drivers/usb/misc/uss720.c, 372:
set_1284_register in parport_uss720_frob_control
drivers/parport/ieee1284.c, 560:
[FUNC_PTR]parport_uss720_frob_control in parport_ieee1284_ack_data_avail
drivers/parport/ieee1284.c, 577:
parport_ieee1284_ack_data_avail in parport_ieee1284_interrupt
./include/linux/parport.h, 474:
parport_ieee1284_interrupt in parport_generic_irq
drivers/usb/misc/uss720.c, 116:
parport_generic_irq in async_complete
[FUNC] get_1284_register(GFP_KERNEL)
drivers/usb/misc/uss720.c, 382:
get_1284_register in parport_uss720_read_status
drivers/parport/ieee1284.c, 555:
[FUNC_PTR]parport_uss720_read_status in parport_ieee1284_ack_data_avail
drivers/parport/ieee1284.c, 577:
parport_ieee1284_ack_data_avail in parport_ieee1284_interrupt
./include/linux/parport.h, 474:
parport_ieee1284_interrupt in parport_generic_irq
drivers/usb/misc/uss720.c, 116:
parport_generic_irq in async_complete
Note that [FUNC_PTR] means a function pointer call is used.
To fix these bugs, GFP_KERNEL is replaced with GFP_ATOMIC.
These bugs are found by my static analysis tool DSAC.
Signed-off-by: Jia-Ju Bai
---
drivers/usb/misc/uss720.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/uss720.c b/drivers/usb/misc/uss720.c
index de9a502491c2..69822852888a 100644
--- a/drivers/usb/misc/uss720.c
+++ b/drivers/usb/misc/uss720.c
@@ -369,7 +369,7 @@ static unsigned char parport_uss720_frob_control(struct
parport *pp, unsigned ch
mask &= 0x0f;
val &= 0x0f;
d = (priv->reg[1] & (~mask)) ^ val;
- if (set_1284_register(pp, 2, d, GFP_KERNEL))
+ if (set_1284_register(pp, 2, d, GFP_ATOMIC))
return 0;
priv->reg[1] = d;
return d & 0xf;
@@ -379,7 +379,7 @@ static unsigned char parport_uss720_read_status(struct
parport *pp)
{
unsigned char ret;
- if (get_1284_register(pp, 1, &ret, GFP_KERNEL))
+ if (get_1284_register(pp, 1, &ret, GFP_ATOMIC))
return 0;
return ret & 0xf8;
}
--
2.17.0
[PATCH v2] usb: cdc-wdm: Fix a sleep-in-atomic-context bug in service_outstanding_interrupt()
wdm_in_callback() is a completion handler function for the USB driver.
So it should not sleep. But it calls service_outstanding_interrupt(),
which calls usb_submit_urb() with GFP_KERNEL.
To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC.
This bug is found by my static analysis tool DSAC.
Signed-off-by: Jia-Ju Bai
---
v2:
* Add more description.
---
drivers/usb/class/cdc-wdm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index a0d284ef3f40..632a2bfabc08 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -458,7 +458,7 @@ static int service_outstanding_interrupt(struct wdm_device
*desc)
set_bit(WDM_RESPONDING, &desc->flags);
spin_unlock_irq(&desc->iuspin);
- rv = usb_submit_urb(desc->response, GFP_KERNEL);
+ rv = usb_submit_urb(desc->response, GFP_ATOMIC);
spin_lock_irq(&desc->iuspin);
if (rv) {
dev_err(&desc->intf->dev,
--
2.17.0
[PATCH] usb: cdc-wdm: Fix a sleep-in-atomic-context bug in service_outstanding_interrupt()
wdm_in_callback() is a completion handler function for the USB driver.
So it should not sleep.
To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC.
This bug is found by my static analysis tool DSAC.
Signed-off-by: Jia-Ju Bai
---
drivers/usb/class/cdc-wdm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index a0d284ef3f40..632a2bfabc08 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -458,7 +458,7 @@ static int service_outstanding_interrupt(struct wdm_device
*desc)
set_bit(WDM_RESPONDING, &desc->flags);
spin_unlock_irq(&desc->iuspin);
- rv = usb_submit_urb(desc->response, GFP_KERNEL);
+ rv = usb_submit_urb(desc->response, GFP_ATOMIC);
spin_lock_irq(&desc->iuspin);
if (rv) {
dev_err(&desc->intf->dev,
--
2.17.0
Re: [usb-storage] [PATCH] usb: storage: Fix a possible data race in uas_queuecommand_lck
On 2018/5/8 16:27, Oliver Neukum wrote: Am Dienstag, den 08.05.2018, 15:47 +0800 schrieb Jia-Ju Bai: The write operations to "cmnd->result" and "cmnd->scsi_done" are protected by the lock on line 642-643, but the write operations to these data on line 634-635 are not protected by the lock. Thus, there may exist a data race for "cmnd->result" and "cmnd->scsi_done". No, the write operations need no lock. The low level driver at this point owns the command. We cannot race with abort() for a command within queuecommand(). We take the lock where we take it to protect dev->resetting. I don't see why the scope of the lock would need to be enlarged. Okay, thanks for your reply and explanation. Best wishes, Jia-Ju Bai -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: storage: Fix a possible data race in uas_queuecommand_lck
The write operations to "cmnd->result" and "cmnd->scsi_done"
are protected by the lock on line 642-643, but the write operations
to these data on line 634-635 are not protected by the lock.
Thus, there may exist a data race for "cmnd->result"
and "cmnd->scsi_done".
To fix this data race, the write operations on line 634-635
should be also protected by the lock.
Signed-off-by: Jia-Ju Bai
---
drivers/usb/storage/uas.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 6034c39b67d1..dde7a43ad491 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -627,17 +627,18 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
if (cmnd->device->host->host_self_blocked)
return SCSI_MLQUEUE_DEVICE_BUSY;
+ spin_lock_irqsave(&devinfo->lock, flags);
+
if ((devinfo->flags & US_FL_NO_ATA_1X) &&
(cmnd->cmnd[0] == ATA_12 || cmnd->cmnd[0] == ATA_16)) {
memcpy(cmnd->sense_buffer, usb_stor_sense_invalidCDB,
sizeof(usb_stor_sense_invalidCDB));
cmnd->result = SAM_STAT_CHECK_CONDITION;
cmnd->scsi_done(cmnd);
+ spin_unlock_irqrestore(&devinfo->lock, flags);
return 0;
}
- spin_lock_irqsave(&devinfo->lock, flags);
-
if (devinfo->resetting) {
cmnd->result = DID_ERROR << 16;
cmnd->scsi_done(cmnd);
--
2.17.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] net: usb: hso: Replace GFP_ATOMIC with GFP_KERNEL in hso_create_device
hso_create_device() is never called in atomic context.
The call chains ending up at hso_create_device() are:
[1] hso_create_device() <- hso_create_bulk_serial_device() <- hso_probe()
[2] hso_create_device() <- hso_create_mux_serial_device() <- hso_probe()
[3] hso_create_device() <- hso_create_net_device() <- hso_probe()
hso_probe() is set as ".probe" in struct usb_driver,
so it is not called in atomic context.
Despite never getting called from atomic context,
hso_create_device() calls kzalloc() with GFP_ATOMIC,
which does not sleep for allocation.
GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL,
which can sleep and improve the possibility of sucessful allocation.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai
---
drivers/net/usb/hso.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index d7a3379..3d7a33f 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2332,7 +2332,7 @@ static struct hso_device *hso_create_device(struct
usb_interface *intf,
{
struct hso_device *hso_dev;
- hso_dev = kzalloc(sizeof(*hso_dev), GFP_ATOMIC);
+ hso_dev = kzalloc(sizeof(*hso_dev), GFP_KERNEL);
if (!hso_dev)
return NULL;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] usb: isp1760: Replace mdelay with msleep in isp1760_stop
isp1760_stop() is never called in atomic context. The call chain ending up at isp1760_stop() is: [1] isp1760_stop() <- isp1760_shutdown() isp1760_shutdown() is set as ".shutdown" in struct hc_driver. isp1760_stop() is also set as ".stop" in hc_driver. These functions are not called in atomic context. Despite never getting called from atomic context, isp1760_stop() calls mdelay() to busily wait. This is not necessary and can be replaced with msleep() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it Signed-off-by: Jia-Ju Bai --- drivers/usb/isp1760/isp1760-hcd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/isp1760/isp1760-hcd.c b/drivers/usb/isp1760/isp1760-hcd.c index 8e59e0c..5599310 100644 --- a/drivers/usb/isp1760/isp1760-hcd.c +++ b/drivers/usb/isp1760/isp1760-hcd.c @@ -2090,7 +2090,7 @@ static void isp1760_stop(struct usb_hcd *hcd) isp1760_hub_control(hcd, ClearPortFeature, USB_PORT_FEAT_POWER, 1, NULL, 0); - mdelay(20); + msleep(20); spin_lock_irq(&priv->lock); ehci_reset(hcd); -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] usb: isp1760: Replace mdelay with msleep in isp1760_init_core
isp1760_init_core() is never called in atomic context.
The call chains ending up at isp1760_init_core() are:
[1] isp1760_init_core() <- isp1760_register() <- isp1760_plat_probe()
[2] isp1760_init_core() <- isp1760_register() <- isp1761_pci_probe()
isp1760_plat_probe() is set as ".probe" in struct platform_driver.
isp1761_pci_probe() is set as ".probe" in struct pci_driver.
These functions are not called in atomic context.
Despite never getting called from atomic context, isp1761_pci_probe()
calls mdelay() to busily wait.
This is not necessary and can be replaced with msleep() to
avoid busy waiting.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai
---
drivers/usb/isp1760/isp1760-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/isp1760/isp1760-core.c
b/drivers/usb/isp1760/isp1760-core.c
index bfa402c..259c3eb 100644
--- a/drivers/usb/isp1760/isp1760-core.c
+++ b/drivers/usb/isp1760/isp1760-core.c
@@ -34,7 +34,7 @@ static void isp1760_init_core(struct isp1760_device *isp)
/* Low-level chip reset */
if (isp->rst_gpio) {
gpiod_set_value_cansleep(isp->rst_gpio, 1);
- mdelay(50);
+ msleep(50);
gpiod_set_value_cansleep(isp->rst_gpio, 0);
}
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: storage: Replace mdelay with msleep in init_freecom
init_freecom() is never called in atomic context. init_freecom() is set as ".initFunction" through UNUSUAL_DEV(). And ->initFunction() is only called by usb_stor_acquire_resources(), which is only called by usb_stor_probe2(). usb_stor_probe2() is called by *_probe() functions (like alauda_probe()) for each USB driver. *_probe() functions are set ".probe" in struct usb_driver. These functions are not called in atomic context. Despite never getting called from atomic context, init_freecom() calls mdelay() to busily wait. This is not necessary and can be replaced with msleep() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- drivers/usb/storage/freecom.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/storage/freecom.c b/drivers/usb/storage/freecom.c index c0a5d95..c7f886d 100644 --- a/drivers/usb/storage/freecom.c +++ b/drivers/usb/storage/freecom.c @@ -477,7 +477,7 @@ static int init_freecom(struct us_data *us) usb_stor_dbg(us, "result from activate reset is %d\n", result); /* wait 250ms */ - mdelay(250); + msleep(250); /* clear reset */ result = usb_stor_control_msg(us, us->send_ctrl_pipe, @@ -485,7 +485,7 @@ static int init_freecom(struct us_data *us) usb_stor_dbg(us, "result from clear reset is %d\n", result); /* wait 3 seconds */ - mdelay(3 * 1000); + msleep(3 * 1000); return USB_STOR_TRANSPORT_GOOD; } -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] usb/io_edgeport: a possible sleep-in-atomic bug in edge_bulk_in_callback
Okay, I had submitted a patch yesterday. You can have a look :) Thanks, Jia-Ju Bai On 2017/12/13 19:38, Johan Hovold wrote: [ +CC: linux-usb] On Wed, Dec 13, 2017 at 06:22:26PM +0800, Jia-Ju Bai wrote: According to drivers/usb/serial/io_edgeport.c, the driver may sleep under a spinlock. The function call path is: edge_bulk_in_callback (acquire the spinlock) process_rcvd_data process_rcvd_status change_port_settings send_iosp_ext_cmd write_cmd_usb usb_kill_urb --> may sleep I do not find a good way to fix it, so I only report. This possible bug is found by my static analysis tool (DSAC) and checked by my code review. Good catch! Fortunately, the fix here is just to remove that usb_kill_urb() from the error path in write_cmd_usb() after usb_submit_urb() fails. Care to submit a patch for that? Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb/io_edgeport: Fix a possible sleep-in-atomic bug in edge_bulk_in_callback
According to drivers/usb/serial/io_edgeport.c, the driver may sleep under a spinlock. The function call path is: edge_bulk_in_callback (acquire the spinlock) process_rcvd_data process_rcvd_status change_port_settings send_iosp_ext_cmd write_cmd_usb usb_kill_urb --> may sleep To fix it, usb_kill_urb() is removed from the error path after usb_submit_urb() fails. This possible bug is found by my static analysis tool (DSAC) and checked by my code review. Signed-off-by: Jia-Ju Bai --- drivers/usb/serial/io_edgeport.c |1 - 1 file changed, 1 deletion(-) diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c index 219265c..17283f4 100644 --- a/drivers/usb/serial/io_edgeport.c +++ b/drivers/usb/serial/io_edgeport.c @@ -2282,7 +2282,6 @@ static int write_cmd_usb(struct edgeport_port *edge_port, /* something went wrong */ dev_err(dev, "%s - usb_submit_urb(write command) failed, status = %d\n", __func__, status); - usb_kill_urb(urb); usb_free_urb(urb); atomic_dec(&CmdUrbs); return status; -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[BUG] kaweth: a possible sleep-in-atomic bug in kaweth_start_xmit
According to drivers/net/usb/kaweth.c, the driver may sleep under a spinlock. The function call path is: kaweth_start_xmit (acquire the spinlock) kaweth_async_set_rx_mode kaweth_control kaweth_internal_control_msg usb_start_wait_urb wait_event_timeout --> may sleep usb_kill_urb --> may sleep I do not find a good way to fix it, so I only report. This possible bug is found by my static analysis tool (DSAC) and checked by my code review. Thanks, Jia-Ju Bai -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[BUG] drivers/usb/host/isp116x-hcd: a possible sleep-in-atomic bug in isp116x_start
According to drivers/usb/host/isp116x-hcd.c, the kernel module may sleep under a spinlock. The function call path is: isp116x_start (acquire the spinlock) device_init_wakeup device_wakeup_enable wakeup_source_register wakeup_source_create kmalloc(GFP_KERNEL) --> may sleep I do not find a good way to fix it, so I only report. This possible bug is found by my static analysis tool (DSAC) and checked by my code review. Thanks, Jia-Ju Bai -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] gadget: Fix a sleep-in-atomic bug
The driver may sleep under a spin lock, and the function call path is:
ffs_epfile_io (acquire the lock by spin_lock_irq)
usb_ep_alloc_request(GFP_KERNEL) --> may sleep
To fix it, the "GFP_KERNEL" is replaced with "GFP_ATOMIC".
Signed-off-by: Jia-Ju Bai
---
drivers/usb/gadget/function/f_fs.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 47dda34..be90e25 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1015,7 +1015,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct
ffs_io_data *io_data)
else
ret = ep->status;
goto error_mutex;
- } else if (!(req = usb_ep_alloc_request(ep->ep, GFP_KERNEL))) {
+ } else if (!(req = usb_ep_alloc_request(ep->ep, GFP_ATOMIC))) {
ret = -ENOMEM;
} else {
req->buf = data;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] ehci-hcd: Disable memory-write-invalidate when the driver is removed
The driver calls pci_set_mwi to enable memory-write-invalidate when it
is initialized, but does not call pci_clear_mwi when it is removed. Many
other drivers calls pci_clear_mwi when pci_set_mwi is called, such as
r8169, 8139cp and e1000.
This patch adds a function "ehci_pci_remove" to remove the pci driver.
This function calls pci_clear_mwi and usb_hcd_pci_remove, which can
fix the problem.
Signed-off-by: Jia-Ju Bai
---
drivers/usb/host/ehci-pci.c |8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 2a5d2fd..3b3649d 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -377,6 +377,12 @@ static int ehci_pci_probe(struct pci_dev *pdev, const
struct pci_device_id *id)
return usb_hcd_pci_probe(pdev, id);
}
+static void ehci_pci_remove(struct pci_dev *pdev)
+{
+ pci_clear_mwi(pdev);
+ usb_hcd_pci_remove(pdev);
+}
+
/* PCI driver selection metadata; PCI hotplugging uses this */
static const struct pci_device_id pci_ids [] = { {
/* handle any USB 2.0 EHCI controller */
@@ -396,7 +402,7 @@ static struct pci_driver ehci_pci_driver = {
.id_table = pci_ids,
.probe =ehci_pci_probe,
- .remove = usb_hcd_pci_remove,
+ .remove = ehci_pci_remove,
.shutdown = usb_hcd_pci_shutdown,
#ifdef CONFIG_PM
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] ehci-hcd: Cleanup memory resources when ehci_halt fails
The driver calls ehci_mem_init to allocate memory resources.
But these resources are not freed when ehci_halt fails.
This patch adds "ehci_mem_cleanup" in error handling code to fix this problem.
Signed-off-by: Jia-Ju Bai
---
drivers/usb/host/ehci-hcd.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 48c92bf..015b411 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -675,8 +675,10 @@ int ehci_setup(struct usb_hcd *hcd)
return retval;
retval = ehci_halt(ehci);
- if (retval)
+ if (retval) {
+ ehci_mem_cleanup(ehci);
return retval;
+ }
ehci_reset(ehci);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ehci-hcd: Cleanup memory resources when ehci_halt fails
The driver calls ehci_mem_init to allocate memory resources.
But these resources are not freed when ehci_halt fails.
This patch adds "ehci_mem_cleanup" in error handling code to fix this problem.
Signed-off-by: Jia-Ju Bai
---
drivers/usb/host/ehci-hcd.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 48c92bf..015b411 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -675,8 +675,10 @@ int ehci_setup(struct usb_hcd *hcd)
return retval;
retval = ehci_halt(ehci);
- if (retval)
+ if (retval) {
+ ehci_mem_cleanup(ehci);
return retval;
+ }
ehci_reset(ehci);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ehci-hcd: Cleanup memory resources when ehci_halt fails
On 12/29/2015 12:04 AM, Alan Stern wrote:
On Mon, 28 Dec 2015, Jia-Ju Bai wrote:
Please add a changelog.
Signed-off-by: Jia-Ju Bai
---
drivers/usb/host/ehci-hcd.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 48c92bf..015b411 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -675,8 +675,10 @@ int ehci_setup(struct usb_hcd *hcd)
return retval;
retval = ehci_halt(ehci);
- if (retval)
+ if (retval) {
+ ehci_mem_cleanup(ehci);
return retval;
+ }
I would prefer to see the call to ehci_mem_init() moved into
ehci_setup() as well, so that we don't do mem_init in one routine and
mem_cleanup in another.
Alan Stern
Thanks ^_^
I will add the changelog.
I find that it is a little hard to move ehci_mem_init into ehci_setup.
In the code, ehci_mem_init needs data (ehci->periodic_size) in ehci_init
to allocate memory, and some data (ehci->async->hw) in ehci_init needs
the memory allocated by ehci_mem_init.
Thus, moving ehci_mem_init out of ehci_init needs much modification.
I think the easiest way is to call echi_mem_cleanup here (in ehci_setup).
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] ehci-hcd: Disable memory-write-invalidate when the driver is removed
The driver calls pci_set_mwi to enable memory-write-invalidate when it
is initialized, but does not call pci_clear_mwi when it is removed. Many
other drivers calls pci_clear_mwi when pci_set_mwi is called, such as
r8169, 8139cp and e1000.
This patch adds a function "ehci_pci_remove" to remove the pci driver.
This function calls pci_clear_mwi and usb_hcd_pci_remove, which can
fix the problem.
Signed-off-by: Jia-Ju Bai
---
drivers/usb/host/ehci-pci.c |8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 2a5d2fd..3b3649d 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -377,6 +377,12 @@ static int ehci_pci_probe(struct pci_dev *pdev, const
struct pci_device_id *id)
return usb_hcd_pci_probe(pdev, id);
}
+static void ehci_pci_remove(struct pci_dev *pdev)
+{
+ pci_clear_mwi(pdev);
+ usb_hcd_pci_remove(pdev);
+}
+
/* PCI driver selection metadata; PCI hotplugging uses this */
static const struct pci_device_id pci_ids [] = { {
/* handle any USB 2.0 EHCI controller */
@@ -396,7 +402,7 @@ static struct pci_driver ehci_pci_driver = {
.id_table = pci_ids,
.probe =ehci_pci_probe,
- .remove = usb_hcd_pci_remove,
+ .remove = ehci_pci_remove,
.shutdown = usb_hcd_pci_shutdown,
#ifdef CONFIG_PM
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ehci-hcd: Cleanup memory resources when ehci_halt fails
Signed-off-by: Jia-Ju Bai
---
drivers/usb/host/ehci-hcd.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 48c92bf..015b411 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -675,8 +675,10 @@ int ehci_setup(struct usb_hcd *hcd)
return retval;
retval = ehci_halt(ehci);
- if (retval)
+ if (retval) {
+ ehci_mem_cleanup(ehci);
return retval;
+ }
ehci_reset(ehci);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ehci-hcd: Disable memory-write-invalidate when the driver is removed
Signed-off-by: Jia-Ju Bai
---
drivers/usb/host/ehci-hcd.c |2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 48c92bf..c02ec42 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -422,6 +422,7 @@ static void ehci_work (struct ehci_hcd *ehci)
static void ehci_stop (struct usb_hcd *hcd)
{
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
+ struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
ehci_dbg (ehci, "stop\n");
@@ -444,6 +445,7 @@ static void ehci_stop (struct usb_hcd *hcd)
end_free_itds(ehci);
spin_unlock_irq (&ehci->lock);
ehci_mem_cleanup (ehci);
+ pci_clear_mwi(pdev);
if (ehci->amd_pll_fix == 1)
usb_amd_dev_put();
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Potential bugs found in usb_storage
Dear Sir, I'm very sorry to trouble you. Recently I test 15 linux device drivers in runtime and find some potential bugs both in Linux 3.8.6 and Linux 3.17.2. The target file is drivers/usb/storage/usb.c, which is used to build usb_storage.ko. I hope you can help me check my findings: [1] In the normal process of usb_storage, INIT_DELAYED_WORK in usb_stor_probe1 (in storage_probe) and cancel_delayed_work_sync in quiesce_and_remove_host (in usb_stor_disconnect) is called in pairs. However, when INIT_DELAYED_WORK has been called and associate_dev is failed in usb_stor_probe1, "BadDevice" segment is executed immediately to halt the process, but cancel_delayed_work_sync is not called. [2] The same situation with [1] will happen, when usb_alloc_coherent in associate_dev in usb_stor_probe1 is failed. [3] The same situation with [1] will happen, when kmalloc in associate_dev in usb_stor_probe1 is failed. [4] The same situation with [1] will happen, when get_device_info in usb_stor_probe1 is failed. [5] The same situation with [1] will happen, when get_pipes in usb_stor_probe2 is failed. [6] The same situation with [1] will happen, when usb_alloc_urb in usb_stor_acquire_resources is failed. [7] The same situation with [1] will happen, when scsi_add_host in usb_stor_probe2 is failed. Could you help me check these findings? Thank you very much, and I'm looking forward to your reply. -- Best wishes! Jia-Ju Bai -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
