diff mbox series

usb: gadget: u_serial: Add null pointer checks after RX/TX submission

Message ID 20240116141801.396398-1-khtsai@google.com
State New
Headers
Series usb: gadget: u_serial: Add null pointer checks after RX/TX submission |

Commit Message

Kuen-Han Tsai Jan. 16, 2024, 2:16 p.m. UTC 
  Commit ffd603f21423 ("usb: gadget: u_serial: Add null pointer check in
gs_start_io") adds null pointer checks to gs_start_io(), but it doesn't
fully fix the potential null pointer dereference issue. While
gserial_connect() calls gs_start_io() with port_lock held, gs_start_rx()
and gs_start_tx() release the lock during endpoint request submission.
This creates a window where gs_close() could set port->port_tty to NULL,
leading to a dereference when the lock is reacquired.

This patch adds a null pointer check for port->port_tty after RX/TX
submission, and removes the initial null pointer check in gs_start_io()
since the caller must hold port_lock and guarantee non-null values for
port_usb and port_tty.

Fixes: ffd603f21423 ("usb: gadget: u_serial: Add null pointer check in gs_start_io")
Cc: stable@vger.kernel.org
Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
---
Explanation:
    CPU1:                            CPU2:
    gserial_connect() // lock
                                     gs_close() // await lock
    gs_start_rx()     // unlock
    usb_ep_queue()
                                     gs_close() // lock, reset port_tty and unlock
    gs_start_rx()     // lock
    tty_wakeup()      // dereference

Stack traces:
[   51.494375][  T278] ttyGS1: shutdown
[   51.494817][  T269] android_work: sent uevent USB_STATE=DISCONNECTED
[   52.115792][ T1508] usb: [dm_bind] generic ttyGS1: super speed IN/ep1in OUT/ep1out
[   52.516288][ T1026] android_work: sent uevent USB_STATE=CONNECTED
[   52.551667][ T1533] gserial_connect: start ttyGS1
[   52.565634][ T1533] [khtsai] enter gs_start_io, ttyGS1, port->port.tty=0000000046bd4060
[   52.565671][ T1533] [khtsai] gs_start_rx, unlock port ttyGS1
[   52.591552][ T1533] [khtsai] gs_start_rx, lock port ttyGS1
[   52.619901][ T1533] [khtsai] gs_start_rx, unlock port ttyGS1
[   52.638659][ T1325] [khtsai] gs_close, lock port ttyGS1
[   52.656842][ T1325] gs_close: ttyGS1 (0000000046bd4060,00000000be9750a5) ...
[   52.683005][ T1325] [khtsai] gs_close, clear ttyGS1
[   52.683007][ T1325] gs_close: ttyGS1 (0000000046bd4060,00000000be9750a5) done!
[   52.708643][ T1325] [khtsai] gs_close, unlock port ttyGS1
[   52.747592][ T1533] [khtsai] gs_start_rx, lock port ttyGS1
[   52.747616][ T1533] [khtsai] gs_start_io, ttyGS1, going to call tty_wakeup(), port->port.tty=0000000000000000
[   52.747629][ T1533] Unable to handle kernel NULL pointer dereference at virtual address 00000000000001f8
---
 drivers/usb/gadget/function/u_serial.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

--
2.43.0.275.g3460e3d667-goog

Comments

Jiri Slaby Jan. 18, 2024, 9:27 a.m. UTC | #1 
On 16. 01. 24, 15:16, Kuen-Han Tsai wrote:
> Commit ffd603f21423 ("usb: gadget: u_serial: Add null pointer check in
> gs_start_io") adds null pointer checks to gs_start_io(), but it doesn't
> fully fix the potential null pointer dereference issue. While
> gserial_connect() calls gs_start_io() with port_lock held, gs_start_rx()
> and gs_start_tx() release the lock during endpoint request submission.
> This creates a window where gs_close() could set port->port_tty to NULL,
> leading to a dereference when the lock is reacquired.
> 
> This patch adds a null pointer check for port->port_tty after RX/TX
> submission, and removes the initial null pointer check in gs_start_io()
> since the caller must hold port_lock and guarantee non-null values for
> port_usb and port_tty.

Or you switch to tty_port refcounting and need not fiddling with this at 
all ;).

thanks,
Greg KH Jan. 28, 2024, 1:29 a.m. UTC | #2 
On Thu, Jan 18, 2024 at 10:27:54AM +0100, Jiri Slaby wrote:
> On 16. 01. 24, 15:16, Kuen-Han Tsai wrote:
> > Commit ffd603f21423 ("usb: gadget: u_serial: Add null pointer check in
> > gs_start_io") adds null pointer checks to gs_start_io(), but it doesn't
> > fully fix the potential null pointer dereference issue. While
> > gserial_connect() calls gs_start_io() with port_lock held, gs_start_rx()
> > and gs_start_tx() release the lock during endpoint request submission.
> > This creates a window where gs_close() could set port->port_tty to NULL,
> > leading to a dereference when the lock is reacquired.
> > 
> > This patch adds a null pointer check for port->port_tty after RX/TX
> > submission, and removes the initial null pointer check in gs_start_io()
> > since the caller must hold port_lock and guarantee non-null values for
> > port_usb and port_tty.
> 
> Or you switch to tty_port refcounting and need not fiddling with this at all
> ;).

I agree, Kuen-Han, why not do that instead?

thanks,

greg k-h
Kuen-Han Tsai Jan. 28, 2024, 2 p.m. UTC | #3 
Hi Greg & Jiri,

>> Or you switch to tty_port refcounting and need not fiddling with this at all
>> ;).
> I agree, Kuen-Han, why not do that instead?

Thanks for the feedback! I agree that switching to tty_port
refcounting is the right approach.

I'm currently digging into tty_port.c to understand the best way to
implement this change. Could you confirm if I'm on the right track by
using tty_kref_get() and tty_kref_put() to address race conditions?
Additionally, do I need to refactor other functions in u_serial.c that
interact with the TTY without refcounting?

Regards,
Kuen-Han
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index a92eb6d90976..2f1890c8f473 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -539,20 +539,16 @@  static int gs_alloc_requests(struct usb_ep *ep, struct list_head *head,
 static int gs_start_io(struct gs_port *port)
 {
 	struct list_head	*head = &port->read_pool;
-	struct usb_ep		*ep;
+	struct usb_ep		*ep = port->port_usb->out;
 	int			status;
 	unsigned		started;

-	if (!port->port_usb || !port->port.tty)
-		return -EIO;
-
 	/* Allocate RX and TX I/O buffers.  We can't easily do this much
 	 * earlier (with GFP_KERNEL) because the requests are coupled to
 	 * endpoints, as are the packet sizes we'll be using.  Different
 	 * configurations may use different endpoints with a given port;
 	 * and high speed vs full speed changes packet sizes too.
 	 */
-	ep = port->port_usb->out;
 	status = gs_alloc_requests(ep, head, gs_read_complete,
 		&port->read_allocated);
 	if (status)
@@ -569,12 +565,22 @@  static int gs_start_io(struct gs_port *port)
 	port->n_read = 0;
 	started = gs_start_rx(port);

+	/*
+	 * The TTY may be set to NULL by gs_close() after gs_start_rx() or
+	 * gs_start_tx() release locks for endpoint request submission.
+	 */
+	if (!port->port.tty)
+		goto out;
+
 	if (started) {
 		gs_start_tx(port);
 		/* Unblock any pending writes into our circular buffer, in case
 		 * we didn't in gs_start_tx() */
+		if (!port->port.tty)
+			goto out;
 		tty_wakeup(port->port.tty);
 	} else {
+out:
 		gs_free_requests(ep, head, &port->read_allocated);
 		gs_free_requests(port->port_usb->in, &port->write_pool,
 			&port->write_allocated);