| Message ID | 1675864487-18620-1-git-send-email-quic_prashk@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: gadget: u_serial: Add null pointer check in gserial_resume | expand |
On Thu, Feb 09, 2023 at 09:13:37PM +0530, Prashanth K wrote: > > > On 09-02-23 08:39 pm, Alan Stern wrote: > > You should consider having _two_ spinlocks: One in the gs_port structure > > (the way it is now) and a separate global lock. The first would be used > > in situations where you know you have a valid pointer. The second would > > be used in situations where you don't know if the pointer is non-NULL > > or where you are changing the pointer's value. > Lets say we replaced the existing spinlock in gserial_resume and > gserial_disconnect with a new static spinlock, and kept the spinlocks in > other functions unchanged. In that case, wouldn't it cause additional race > conditions as we are using 2 different locks. Not race conditions, but possibilities for deadlock. Indeed, you would have to be very careful about avoiding deadlock scenarios. In particular, you would have to ensure that the code never tries to acquire the global spinlock while already holding one of the per-port spinlocks. Alan Stern
On 09-02-23 09:33 pm, Alan Stern wrote: > On Thu, Feb 09, 2023 at 09:13:37PM +0530, Prashanth K wrote: >> >> >> On 09-02-23 08:39 pm, Alan Stern wrote: >>> You should consider having _two_ spinlocks: One in the gs_port structure >>> (the way it is now) and a separate global lock. The first would be used >>> in situations where you know you have a valid pointer. The second would >>> be used in situations where you don't know if the pointer is non-NULL >>> or where you are changing the pointer's value. >> Lets say we replaced the existing spinlock in gserial_resume and >> gserial_disconnect with a new static spinlock, and kept the spinlocks in >> other functions unchanged. In that case, wouldn't it cause additional race >> conditions as we are using 2 different locks. > > Not race conditions, but possibilities for deadlock. > > Indeed, you would have to be very careful about avoiding deadlock > scenarios. In particular, you would have to ensure that the code never > tries to acquire the global spinlock while already holding one of the > per-port spinlocks. > > Alan Stern Hi Alan, instead of doing these and causing potential regressions, can we just have the null pointer check which i suggested in the beginning? The major concern was that port might become null after the null pointer check. We mark gser->ioport as null pointer in gserial_disconnect, and in gserial_resume we copy the gser->ioport to *port in the beginning. struct gs_port *port = gser->ioport; And hence it wont cause null pointer deref after the check as we don't de-reference anything from gser->ioport afterwards. We only use the local pointer *port afterwards. Thanks, Prashanth K
> And this seems like a viable option to me, what do you suggest? > > gserial_disconnect { > spin_lock(static) > spin_lock(port) > ... > gser->ioport = NULL; > ... > spin_lock(port) > spin_unlock(static) > > } > > gserial_resume { > struct gs_port *port = gser->ioport; > > spin_lock(static) > if (!port) spin_unlock(static) > return > spin_lock(port) > > ... > spin_unlock(port) > spin_unlock(static) > } > > Thanks, > Prashanth K Small correction inlined.
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c index 840626e..98be2b8 100644 --- a/drivers/usb/gadget/function/u_serial.c +++ b/drivers/usb/gadget/function/u_serial.c @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser) struct gs_port *port = gser->ioport; unsigned long flags; + if (!port) + return; + spin_lock_irqsave(&port->port_lock, flags); port->suspended = false; if (!port->start_delayed) {
Consider a case where gserial_disconnect has already cleared gser->ioport. And if a wakeup interrupt triggers afterwards, gserial_resume gets called, which will lead to accessing of gserial->port and thus causing null pointer dereference.Add a null pointer check to prevent this. Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks") Signed-off-by: Prashanth K <quic_prashk@quicinc.com> --- drivers/usb/gadget/function/u_serial.c | 3 +++ 1 file changed, 3 insertions(+)