linux-kernel - 答复: [PATCH v6] usb: gadget: u_serial: Add null pointer check in gs_read_complete & gs_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <TYUPR06MB62176043F3E6D6B6675301D3D2882@TYUPR06MB6217.apcprd06.prod.outlook.com>
Date: Fri, 23 Aug 2024 06:40:18 +0000
From: 胡连勤 <hulianqin@...o.com>
To: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC: Prashanth K <quic_prashk@...cinc.com>, "quic_jjohnson@...cinc.com"
	<quic_jjohnson@...cinc.com>, "linux-usb@...r.kernel.org"
	<linux-usb@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, opensource.kernel
	<opensource.kernel@...o.com>, "akpm@...ux-foundation.org"
	<akpm@...ux-foundation.org>, Michael Nazzareno Trimarchi
	<michael@...rulasolutions.com>
Subject:
 答复: [PATCH v6] usb: gadget: u_serial: Add null pointer check in gs_read_complete & gs_write_complete

Hello linux community expert:

>> Fixes: c1dca562be8a ("usb gadget: split out serial core")
>> Cc: stable@...r.kernel.org
>> Signed-off-by: Lianqin Hu <hulianqin@...o.com>
>> ---
>> v6:
>>   - Update the commit text
>>   - Add the Fixes tag
>>   - CC stable kernel
>>   - Add serial_port_lock protection when checking port pointer
>>   - Optimize code comments
>>   - Delete log printing

>You need to list ALL of the versions here, I seem to have missed v4 and
>v5 somewhere so I don't know what changed there.

 V4: Add cc stable kernel     >> Cc: stable@...r.kernel.org
 V5: Add the Fixes tag       >> Fixes: c1dca562be8a ("usb gadget: split out serial core")
>You can also add the Fixes tag and CC stable kernel, so that it can be
>backported to older kernels (such as 5.15) also.
   ---------  The above two lines are from Prashanth K's comment

>>  static void gs_read_complete(struct usb_ep *ep, struct usb_request 
>> *req)  {
>> -	struct gs_port	*port = ep->driver_data;
>> +	struct gs_port	*port;
>> +	unsigned long  flags;
>> +
>> +	spin_lock_irqsave(&serial_port_lock, flags);
>> +	port = ep->driver_data;
>> +
>> +	/* When port is NULL, return to avoid panic. */

>This comment is not needed, it's obvious that you check before dereference.
 OK, I will delete this comment in the new patch.

>BUT you can mention that you are trying to check with the race somewhere else, right?  Please do that, and document here where that race is at that you are doing this extra locking for.
 I don't fully understand what you mean. Are you asking which logic is in competition with this one, causing this port to be null?
  

>> +	if (!port) {
>> +		spin_unlock_irqrestore(&serial_port_lock, flags);
>> +		return;
>> +	}
>>  
>> -	/* Queue all received data until the tty layer is ready for it. */
>>  	spin_lock(&port->port_lock);
>> +	spin_unlock(&serial_port_lock);

>nested spinlocks, why?  Did you run this with lockdep enabled to verify you aren't hitting a different bug now?
 Because there is a competition relationship between this function and the gserial_disconnect function, 
 the gserial_disconnect function first obtains serial_port_lock and then obtains port->port_lock. 
 The purpose of nesting is to ensure that when gs_read_complete is executed, it can be successfully executed after obtaining serial_port_lock.
 gserial_disconnect(..)
 {
	struct gs_port	*port = gser->ioport;
	...
	spin_lock_irqsave(&serial_port_lock, flags);
	spin_lock(&port->port_lock);
	...
	gser->ioport = NULL;   ---> port = NULL;
	...
	spin_unlock(&port->port_lock);
	spin_unlock_irqrestore(&serial_port_lock, flags);
 }

After enabling the lockdep function (CONFIG_DEBUG_LOCK_ALLOC=y), there is no lockdep-related warning information.

>And why is one irqsave and one not?  That feels odd, it might be right, but you need to document here why the difference.
 After the gs_read_complete function is executed, spin_unlock_irqrestore is used to restore the previous state，
-	/* Queue all received data until the tty layer is ready for it. */
 	spin_lock(&port->port_lock);
+	spin_unlock(&serial_port_lock);
+
+	/* Queue all received data until the tty layer is ready for it. */
 	list_add_tail(&req->list, &port->read_queue);
 	schedule_delayed_work(&port->push, 0);
-	spin_unlock(&port->port_lock);
+	spin_unlock_irqrestore(&port->port_lock, flags);   ---> Here we use spin_unlock_irqrestore to restore the state
 }

Thanks