* 答复: [bug report] usb: gadget: u_serial: Fix the issue that gs_start_io crashed due to accessing null pointer
2025-06-25 15:22 [bug report] usb: gadget: u_serial: Fix the issue that gs_start_io crashed due to accessing null pointer Dan Carpenter
@ 2025-06-26 2:16 ` 胡连勤
2025-06-26 6:22 ` Prashanth K
1 sibling, 0 replies; 3+ messages in thread
From: 胡连勤 @ 2025-06-26 2:16 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-usb@vger.kernel.org
Hello Dan Carpenter:
>
> Hello Lianqin Hu,
>
> The patch 4cfbca86f6a8: "usb: gadget: u_serial: Fix the issue that gs_start_io
> crashed due to accessing null pointer" from Dec 3, 2024, leads to the
> following static checker warning:
>
> drivers/usb/gadget/function/u_serial.c:580 gs_start_io()
> warn: variable dereferenced before check 'port->port_usb' (see line
> 547)
>
> drivers/usb/gadget/function/u_serial.c
> 544 static int gs_start_io(struct gs_port *port)
> 545 {
> 546 struct list_head *head = &port->read_pool;
> 547 struct usb_ep *ep = port->port_usb->out;
> ^^^^^^^^^^^^^^^^^^^
> port->port_usb dereferenced here
>
> 548 int status;
> 549 unsigned started;
> 550
> 551 /* Allocate RX and TX I/O buffers. We can't easily do this much
> 552 * earlier (with GFP_KERNEL) because the requests are coupled to
> 553 * endpoints, as are the packet sizes we'll be using. Different
> 554 * configurations may use different endpoints with a given port;
> 555 * and high speed vs full speed changes packet sizes too.
> 556 */
> 557 status = gs_alloc_requests(ep, head, gs_read_complete,
> 558 &port->read_allocated);
> 559 if (status)
> 560 return status;
> 561
> 562 status = gs_alloc_requests(port->port_usb->in, &port->write_pool,
> ^^^^^^^^^^^^^^^^^^ and here
>
> 563 gs_write_complete, &port->write_allocated);
> 564 if (status) {
> 565 gs_free_requests(ep, head, &port->read_allocated);
> 566 return status;
> 567 }
> 568
> 569 /* queue read requests */
> 570 port->n_read = 0;
> 571 started = gs_start_rx(port);
The gs_start_rx function will briefly release prot->lock, and then hold port->lock again, as follows:
static unsigned gs_start_rx(struct gs_port *port)
{
...
spin_unlock(&port->port_lock);
status = usb_ep_queue(out, req, GFP_ATOMIC);
spin_lock(&port->port_lock);
...
}
> 572
> 573 if (started) {
> 574 gs_start_tx(port);
> 575 /* Unblock any pending writes into our circular buffer, in case
> 576 * we didn't in gs_start_tx() */
> 577 tty_port_tty_wakeup(&port->port);
> 578 } else {
> 579 /* Free reqs only if we are still connected */
> --> 580 if (port->port_usb) {
> ^^^^^^^^^^^^^^ Checked here. The commit message says
> that this is to fix a race condition where a different thread could set port-
> >port_usb to NULL after we call gs_start_rx(). However, the code is still racy
> because we're not holding the spin_lock(&port->port_lock) so port->usb
> could still be freed between lines 580 and 582. The window for the race is
> much smaller but it's still a potential issue.
so, from line 580 to line 582, there is port->lock protection.
After the gs_start_io function call is completed, the main call to gs_open goes back to release port->lock
static int gs_open(struct tty_struct *tty, struct file *file)
{
...
pr_debug("gs_open: start ttyGS%d\n", port->port_num);
gs_start_io(port);
if (gser->connect)
gser->connect(gser);
} else {
pr_debug("delay start of ttyGS%d\n", port->port_num);
port->start_delayed = true;
}
}
pr_debug("gs_open: ttyGS%d (%p,%p)\n", port->port_num, tty, file);
exit_unlock_port:
spin_unlock_irq(&port->port_lock); ----------> here
>
> 581 gs_free_requests(ep, head, &port->read_allocated);
> 582 gs_free_requests(port->port_usb->in, &port-
> >write_pool,
> 583 &port->write_allocated);
> 584 }
> 585 status = -EIO;
> 586 }
> 587
> 588 return status;
> 589 }
>
> regards,
> dan carpenter
Thanks
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] usb: gadget: u_serial: Fix the issue that gs_start_io crashed due to accessing null pointer
2025-06-25 15:22 [bug report] usb: gadget: u_serial: Fix the issue that gs_start_io crashed due to accessing null pointer Dan Carpenter
2025-06-26 2:16 ` 答复: " 胡连勤
@ 2025-06-26 6:22 ` Prashanth K
1 sibling, 0 replies; 3+ messages in thread
From: Prashanth K @ 2025-06-26 6:22 UTC (permalink / raw)
To: Dan Carpenter, hulianqin; +Cc: linux-usb
On 6/25/2025 8:52 PM, Dan Carpenter wrote:
> Hello Lianqin Hu,
>
> The patch 4cfbca86f6a8: "usb: gadget: u_serial: Fix the issue that
> gs_start_io crashed due to accessing null pointer" from Dec 3, 2024,
> leads to the following static checker warning:
>
> drivers/usb/gadget/function/u_serial.c:580 gs_start_io()
> warn: variable dereferenced before check 'port->port_usb' (see line 547)
>
Hi Dan,
Here's the timeline of commits,
commit ffd603f21423 (usb: gadget: u_serial: Add null pointer check in
gs_start_io)
- Removes line 547 *ep = port->port_usb->out
commit 4cfbca86f6a8 (usb: gadget: u_serial: Fix the issue that
gs_start_io crashed due to accessing null pointer)
- Adds Null pointer check for port_usb before freeing requests (Line 580)
commit f6c7bc4a6823 (Revert "usb: gadget: u_serial: Add null pointer
check in gs_start_io")
- Got accepted last week, adds back line 547 *ep = port->port_usb->out
> drivers/usb/gadget/function/u_serial.c
> 544 static int gs_start_io(struct gs_port *port)
> 545 {
> 546 struct list_head *head = &port->read_pool;
> 547 struct usb_ep *ep = port->port_usb->out;
> ^^^^^^^^^^^^^^^^^^^
> port->port_usb dereferenced here
>
> 548 int status;
> 549 unsigned started;
> 550
> 551 /* Allocate RX and TX I/O buffers. We can't easily do this much
> 552 * earlier (with GFP_KERNEL) because the requests are coupled to
> 553 * endpoints, as are the packet sizes we'll be using. Different
> 554 * configurations may use different endpoints with a given port;
> 555 * and high speed vs full speed changes packet sizes too.
> 556 */
> 557 status = gs_alloc_requests(ep, head, gs_read_complete,
> 558 &port->read_allocated);
> 559 if (status)
> 560 return status;
> 561
> 562 status = gs_alloc_requests(port->port_usb->in, &port->write_pool,
> ^^^^^^^^^^^^^^^^^^
> and here
>
> 563 gs_write_complete, &port->write_allocated);
> 564 if (status) {
> 565 gs_free_requests(ep, head, &port->read_allocated);
> 566 return status;
> 567 }
> 568
> 569 /* queue read requests */
> 570 port->n_read = 0;
> 571 started = gs_start_rx(port);
> 572
> 573 if (started) {
> 574 gs_start_tx(port);
> 575 /* Unblock any pending writes into our circular buffer, in case
> 576 * we didn't in gs_start_tx() */
> 577 tty_port_tty_wakeup(&port->port);
> 578 } else {
> 579 /* Free reqs only if we are still connected */
> --> 580 if (port->port_usb) {
> ^^^^^^^^^^^^^^
> Checked here. The commit message says that this is to fix a race
> condition where a different thread could set port->port_usb to NULL
> after we call gs_start_rx(). However, the code is still racy
> because we're not holding the spin_lock(&port->port_lock) so port->usb
> could still be freed between lines 580 and 582. The window for the
> race is much smaller but it's still a potential issue.
gs_start_io() is called with port_lock, hence port_usb is not null @line547
But gs_start_rx() and gs_start_tx() releases and acquires back the lock
during ep_queue, if port gets disconnect then port_usb can become NULL
during this timeframe.
Hence a null check was added at line 580. This prevents null pointer
dereference. It cant become NULL after the check since the lock is held.
>
> 581 gs_free_requests(ep, head, &port->read_allocated);
> 582 gs_free_requests(port->port_usb->in, &port->write_pool,
> 583 &port->write_allocated);
> 584 }
> 585 status = -EIO;
> 586 }
> 587
> 588 return status;
> 589 }
>
> regards,
> dan carpenter
>
Looks like a false positive to me.
Regards,
Prashanth K
^ permalink raw reply [flat|nested] 3+ messages in thread