Re: [bug report] usb: gadget: u_serial: Fix the issue that gs_start_io crashed due to accessing null pointer

[Prashanth K <prashanth.k@oss.qualcomm.com>]

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