Re: [PATCH v3] usb: gadget: u_serial: Add null pointer check in gserial_resume

[Prashanth K <quic_prashk@quicinc.com>]

On 12-02-23 02:02 am, Alan Stern wrote:
> On Sun, Feb 12, 2023 at 01:37:13AM +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>
>> ---
> 
> This looks pretty good, except for a couple of small things...
> 
>> v3: Fixed the spin_lock_irqsave flags.
>>
>>   drivers/usb/gadget/function/u_serial.c | 22 +++++++++++++++++-----
>>   1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
>> index 840626e..471087f 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);
> 
> You might put a short comment before this line, explaining what the
> purpose of serial_port_lock is.  Otherwise people will wonder what it is
> for.
That's right, will do it.
> 
>> +
>>   /* console info */
>>   struct gs_console {
>>   	struct console		console;
>> @@ -1370,13 +1372,15 @@ EXPORT_SYMBOL_GPL(gserial_connect);
>>   void gserial_disconnect(struct gserial *gser)
>>   {
>>   	struct gs_port	*port = gser->ioport;
>> -	unsigned long	flags;
>> +	unsigned long flags;
> 
> Unnecessary whitespace change.  Leave the original code as it is.
> 
>>   
>>   	if (!port)
>>   		return;
> 
> Is it really possible for port to be NULL here?  If it is possible,
> where would gser->ioport be set to NULL?
> 
> And if it's not possible, this test should be removed.
Seems like this is present since the inception of this file, hence I'm 
not exactly sure why it was added. In my opinion lets keep it, so as to 
not cause regressions.
> 
>>   
>> +	spin_lock_irqsave(&serial_port_lock, flags);
>> +
>>   	/* tell the TTY glue not to do I/O here any more */
>> -	spin_lock_irqsave(&port->port_lock, flags);
>> +	spin_lock(&port->port_lock);
>>   
>>   	gs_console_disconnect(port);
>>   
>> @@ -1391,7 +1395,8 @@ void gserial_disconnect(struct gserial *gser)
>>   			tty_hangup(port->port.tty);
>>   	}
>>   	port->suspended = false;
>> -	spin_unlock_irqrestore(&port->port_lock, flags);
>> +	spin_unlock(&port->port_lock);
>> +	spin_unlock_irqrestore(&serial_port_lock, flags);
>>   
>>   	/* 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;
> 
> You shouldn't read gser->ioport here; do it under the protection of the
> static spinlock.  If you do the read here then there will still be a
> data race, because gserial_disconnect() might change the value just as
> you are reading it.
> 
>> -	unsigned long	flags;
>> +	unsigned long flags;
> 
> Again, unnecessary whitespace change.
> 
>>   
>> -	spin_lock_irqsave(&port->port_lock, flags);
>> +	spin_lock_irqsave(&serial_port_lock, flags);
> 
> Here is where you should read gser->ioport.
Yes, understood
> 
>> +	if (!port) {
>> +		spin_unlock_irqrestore(&serial_port_lock, flags);
>> +		return;
>> +	}
>> +
>> +	spin_lock(&port->port_lock);
>> +	spin_unlock(&serial_port_lock);
>>   	port->suspended = false;
>>   	if (!port->start_delayed) {
>>   		spin_unlock_irqrestore(&port->port_lock, flags);
> 
> Alan Stern