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

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

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

    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

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

[胡连勤]

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

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

[Prashanth K]

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