| Message ID | 20250617050844.1848232-1-khtsai@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] Revert "usb: gadget: u_serial: Add null pointer check in gs_start_io" | expand |
On 6/17/2025 10:37 AM, Kuen-Han Tsai wrote: > This reverts commit ffd603f214237e250271162a5b325c6199a65382. > > Commit ffd603f21423 ("usb: gadget: u_serial: Add null pointer check in > gs_start_io") adds null pointer checks at the beginning of the > gs_start_io() function to prevent a null pointer dereference. However, > these checks are redundant because the function's comment already > requires callers to hold the port_lock and ensure port.tty and port_usb > are not null. All existing callers already follow these rules. > > The true cause of the null pointer dereference is a race condition. When > gs_start_io() calls either gs_start_rx() or gs_start_tx(), the port_lock > is temporarily released for usb_ep_queue(). This allows port.tty and > port_usb to be cleared. > > Fixes: ffd603f21423 ("usb: gadget: u_serial: Add null pointer check in gs_start_io") > Signed-off-by: Kuen-Han Tsai <khtsai@google.com> > --- Reviewed-by: Prashanth K <prashanth.k@oss.qualcomm.com> > v2: > - Remove Cc: stable > > --- > drivers/usb/gadget/function/u_serial.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c > index ab544f6824be..c043bdc30d8a 100644 > --- a/drivers/usb/gadget/function/u_serial.c > +++ b/drivers/usb/gadget/function/u_serial.c > @@ -544,20 +544,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) > -- > 2.50.0.rc2.692.g299adb8693-goog >
On Wed, Jun 18, 2025 at 8:26 PM Prashanth K <prashanth.k@oss.qualcomm.com> wrote: > > > > On 6/17/2025 10:37 AM, Kuen-Han Tsai wrote: > > A race condition occurs when gs_start_io() calls either gs_start_rx() or > > gs_start_tx(), as those functions briefly drop the port_lock for > > usb_ep_queue(). This allows gs_close() and gserial_disconnect() to clear > > port.tty and port_usb, respectively. > > > > Use the null-safe TTY Port helper function to wake up TTY. > > > > Example > > 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() // NPE > > > > Fixes: 35f95fd7f234 ("TTY: usb/u_serial, use tty from tty_port") > > Cc: stable@vger.kernel.org > > Signed-off-by: Kuen-Han Tsai <khtsai@google.com> > > --- > > Reviewed-by: Prashanth K <prashanth.k@oss.qualcomm.com> > > > v2: > > - Move the example up to the changelog > > > > 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 | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c > > index c043bdc30d8a..540dc5ab96fc 100644 > > --- a/drivers/usb/gadget/function/u_serial.c > > +++ b/drivers/usb/gadget/function/u_serial.c > > @@ -295,8 +295,8 @@ __acquires(&port->port_lock) > > break; > > } > > > > - if (do_tty_wake && port->port.tty) > > - tty_wakeup(port->port.tty); > > + if (do_tty_wake) > > + tty_port_tty_wakeup(&port->port); > > return status; > > } > > > > @@ -574,7 +574,7 @@ static int gs_start_io(struct gs_port *port) > > gs_start_tx(port); > > /* Unblock any pending writes into our circular buffer, in case > > * we didn't in gs_start_tx() */ > > - tty_wakeup(port->port.tty); > > Just curious, since this is already under lock, checking for > port->port.tty would have also helped, right? Anyways looks like > tty_port_tty_wakeup is better. You're right, adding a null check for port->port.tty also solves the problem. I actually submitted that exact solution last year, but it was rejected. Link: https://lore.kernel.org/linux-usb/20240116141801.396398-1-khtsai@google.com/ > > > + tty_port_tty_wakeup(&port->port); > > } else { > > /* Free reqs only if we are still connected */ > > if (port->port_usb) { > > -- > > 2.50.0.rc2.692.g299adb8693-goog > >
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c index ab544f6824be..c043bdc30d8a 100644 --- a/drivers/usb/gadget/function/u_serial.c +++ b/drivers/usb/gadget/function/u_serial.c @@ -544,20 +544,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)
This reverts commit ffd603f214237e250271162a5b325c6199a65382. Commit ffd603f21423 ("usb: gadget: u_serial: Add null pointer check in gs_start_io") adds null pointer checks at the beginning of the gs_start_io() function to prevent a null pointer dereference. However, these checks are redundant because the function's comment already requires callers to hold the port_lock and ensure port.tty and port_usb are not null. All existing callers already follow these rules. The true cause of the null pointer dereference is a race condition. When gs_start_io() calls either gs_start_rx() or gs_start_tx(), the port_lock is temporarily released for usb_ep_queue(). This allows port.tty and port_usb to be cleared. Fixes: ffd603f21423 ("usb: gadget: u_serial: Add null pointer check in gs_start_io") Signed-off-by: Kuen-Han Tsai <khtsai@google.com> --- v2: - Remove Cc: stable --- drivers/usb/gadget/function/u_serial.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) -- 2.50.0.rc2.692.g299adb8693-goog