Received-SPF: pass (google.com: domain of linux-kernel+bounces-96957-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240116141801.396398-1-khtsai@google.com> <02bec7b8-7754-4b9d-84ae-51621d6aa7ec@kernel.org>
 <2024012724-chirpy-google-51bb@gregkh> <CAKzKK0oEO5_-CBKvYSw4DKY4Wp5UPrrt1ehBFRd79idy7FsUuQ@mail.gmail.com>
In-Reply-To: <CAKzKK0oEO5_-CBKvYSw4DKY4Wp5UPrrt1ehBFRd79idy7FsUuQ@mail.gmail.com>
From: Kuen-Han Tsai <khtsai@google.com>
Date: Fri, 8 Mar 2024 19:47:04 +0800
Message-ID: <CAKzKK0pmswLnGa8zabp_wo=6BcvCd9DR368FCJ5mcpZ38i4Jdw@mail.gmail.com>
Subject: Re: [PATCH] usb: gadget: u_serial: Add null pointer checks after
 RX/TX submission
To: Greg KH <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>, quic_prashk@quicinc.com, 
	stern@rowland.harvard.edu, linux-usb@vger.kernel.org, 
	linux-kernel@vger.kernel.org, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Greg & Jiri,

On Sun, Jan 28, 2024 at 9:29=E2=80=AFAM Greg KH <gregkh@linuxfoundation.org=
> wrote:
>
> 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 i=
n
> > > 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_r=
x()
> > > and gs_start_tx() release the lock during endpoint request submission=
.
> > > This creates a window where gs_close() could set port->port_tty to NU=
LL,
> > > 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 fo=
r
> > > port_usb and port_tty.
> >
> > Or you switch to tty_port refcounting and need not fiddling with this a=
t all
> > ;).
>
> I agree, Kuen-Han, why not do that instead?

The u_serial driver has already maintained the usage count of a TTY
structure for open and close. While the driver tracks the usage count
via open/close, it doesn't fully eliminate race conditions. Below are
two potential scenarios:

Case 1 (Observed):
1. gs_open() sets usage count to 1.
2. gserial_connect(), gs_start_io(), and gs_start_rx() execute in
sequence (lock held).
3. Lock released, usb_ep_queue() called.
4. In parallel, gs_close() executes, sees count of 1, clears TTY, releases =
lock.
5. Original thread resumes in gs_start_rx(), potentially leading to
kernel panic on an invalid TTY.

---

Case 2: Hypothesis. Similar to Case 1, but the race occurs between
gs_open() and gs_close(), also potentially causing a kernel panic.
1. gserial_connect() enables usb endpoints.
2. gs_open(), gs_start_io(), and gs_start_rx() execute in sequence (lock he=
ld).
3. Lock released, usb_ep_queue() called.
4. In parallel, gs_close() executes, sees count of 1, clears TTY, releases =
lock.
5. Original thread resumes in gs_start_rx(), potentially leading to
kernel panic on an invalid TTY.

---

Since both gserial_connect() and gs_open() initiate gs_start_io(),
there's a brief window where gs_start_rx() releases a spinlock for USB
submission. If gs_close() executes during this window, it could
acquire the lock and clear the TTY structure prematurely. This happens
because the lock is released and the usage count remains 1, making it
appear like a valid final reference, even though gs_start_io() is
still in progress.

My only solution so far is to recheck the TTY structure after
gs_start_rx() or gs_start_tx(). I would greatly appreciate your
insights on how to address this race condition effectively.

Regards,
Kuen-Han