* Re: [PATCH v2] usb: gadget: u_serial: Add null pointer check in gserial_resume
2023-02-11 12:17 [PATCH v2] usb: gadget: u_serial: Add null pointer check in gserial_resume Prashanth K
@ 2023-02-11 16:26 ` Alan Stern
0 siblings, 0 replies; 2+ messages in thread
From: Alan Stern @ 2023-02-11 16:26 UTC (permalink / raw)
To: Prashanth K
Cc: Greg Kroah-Hartman, Christophe JAILLET, Xiu Jianfeng,
Pratham Pratap, Jack Pham, linux-usb, linux-kernel
On Sat, Feb 11, 2023 at 05:47:38PM +0530, Prashanth K wrote:
> 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
> gser->ioport and thus causing null pointer dereference.Add
> a null pointer check to prevent this.
>
> Added a static spinlock to prevent gser->ioport from becoming
> null after the newly added check.
>
> Fixes: aba3a8d01d62 ("usb: gadget: u_serial: add suspend resume callbacks")
> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
> ---
> v2: Added static spinlock and fixed Fixes tag.
>
> drivers/usb/gadget/function/u_serial.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index 840626e..9ced0fa 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -82,6 +82,8 @@
> #define WRITE_BUF_SIZE 8192 /* TX only */
> #define GS_CONSOLE_BUF_SIZE 8192
>
> +static DEFINE_SPINLOCK(serial_port_lock);
> +
> /* console info */
> struct gs_console {
> struct console console;
> @@ -1370,11 +1372,13 @@ EXPORT_SYMBOL_GPL(gserial_connect);
> void gserial_disconnect(struct gserial *gser)
> {
> struct gs_port *port = gser->ioport;
> - unsigned long flags;
> + unsigned long flags, serial_flag;
You don't need two separate flags here. The fact that you wrote this
indicates you don't understand how spin_lock_irqsave() and
spin_lock_irqrestore() work.
When spin_lock_irqsave(&lock, flag) is called, it saves the current INT
(interrupt-enable) setting in flag, disables interrupts, and acquires
the lock. When spin_unlock_irqrestore(&lock, flag) is called, it
releases the lock and writes the value in flag back to the INT setting.
The end result is that if interrupts were enabled before
spin_lock_irqsave() then they will be enabled after
spin_unlock_irqrestore(). If interrupts were disabled beforehand, they
will remain disabled afterward. And either way, interrupts will be
disabled between the two calls.
>
> if (!port)
> return;
>
> + spin_lock_irqsave(&serial_port_lock, serial_flag);
So now interrupts are disabled.
> +
> /* tell the TTY glue not to do I/O here any more */
> spin_lock_irqsave(&port->port_lock, flags);
Hence there's no need for flag here. You don't need to save the current
INT setting because you already know what it is: interrupts are
disabled. You can simply call spin_lock(), which will acquire the lock
without doing anything to the INT setting.
>
> @@ -1392,6 +1396,7 @@ void gserial_disconnect(struct gserial *gser)
> }
> port->suspended = false;
> spin_unlock_irqrestore(&port->port_lock, flags);
Likewise, here you can call spin_unlock().
> + spin_unlock_irqrestore(&serial_port_lock, serial_flag);
>
> /* disable endpoints, aborting down any active I/O */
> usb_ep_disable(gser->out);
> @@ -1426,9 +1431,16 @@ EXPORT_SYMBOL_GPL(gserial_suspend);
> void gserial_resume(struct gserial *gser)
> {
> struct gs_port *port = gser->ioport;
> - unsigned long flags;
> + unsigned long flags, serial_flag;
> +
> + spin_lock_irqsave(&serial_port_lock, serial_flag);
> + if (!port) {
> + spin_unlock_irqrestore(&serial_port_lock, serial_flag);
> + return;
> + }
This is a little trickier, but the same principles apply. Since
spin_lock_irqsave() was called above, interrupts are now disabled.
>
> spin_lock_irqsave(&port->port_lock, flags);
So there's no need for _irqsave here.
> + spin_unlock_irqrestore(&serial_port_lock, serial_flag);
And here you must not use spin_unlock_irqrestore(). This will do the
wrong thing, because it will enable interrupts if they were enabled at
the start of the function. Then you would be running with
port->port_lock held and interrupts enabled, a bad combination.
> port->suspended = false;
> if (!port->start_delayed) {
> spin_unlock_irqrestore(&port->port_lock, flags);
Here, at the final unlock, is where you should restore the INT setting
to the value it had at the start of the function.
Alan Stern
^ permalink raw reply [flat|nested] 2+ messages in thread