usb: Fix null-ptr-deref in qt2_process_read_urb()

PATCH Superseded qasdev 2 participants 3 messages Jan 11, 2025 – Jan 12, 2025

Subsystems (2)

USB USB serial

lore

Share & tools

lore

qasdev

author

2025-01-11 20:09 UTC #0 lore raw

PATCH 1/1 usb: Fix null-ptr-deref in qt2_process_read_urb()

This patch addresses a null-ptr-deref in qt2_process_read_urb() due to an incorrect bounds check in the following:

       if (newport > serial->num_ports) {
	       dev_err(&port->dev,
		       "%s - port change to invalid port: %i\n",
		       __func__, newport);
	       break;
       }

The condition doesn't account for the valid range of the serial->port buffer, which is from 0 to serial->num_ports - 1. When newport is equal to serial->num_ports, the assignment of "port" in the following code is out-of-bounds and NULL:

       serial_priv->current_port = newport;
       port = serial->port[serial_priv->current_port];

The fix checks if newport is greater than or equal to serial->num_ports indicating it is out-of-bounds.

Reported-by syzbot <[email protected]>

Closes https://syzkaller.appspot.com/bug?extid=506479ebf12fe435d01a

Tested-by syzbot <[email protected]>

Tested-by Qasim Ijaz <[email protected]>

Signed-off-by Qasim Ijaz <[email protected]>

---
 drivers/usb/serial/quatech2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/usb/serial/quatech2.c +1 -1

index a317bdbd00ad..72fe83a6c978 100644
@@ -503,7 +503,7 @@ static void qt2_process_read_urb(struct urb *urb)
 
 				newport = *(ch + 3);
 
-				if (newport > serial->num_ports) {
+				if (newport >= serial->num_ports) {
 					dev_err(&port->dev,
 						"%s - port change to invalid port: %i\n",
 						__func__, newport);

Signature · 1 line

2.39.5

Greg Kroah-Hartman

maintainer

2025-01-12 08:42 UTC #1 lore raw

Quoted 23 lines · qasdev

This patch addresses a null-ptr-deref in qt2_process_read_urb() due to an incorrect bounds check in the following:
""
       if (newport > serial->num_ports) {
	       dev_err(&port->dev,
		       "%s - port change to invalid port: %i\n",
		       __func__, newport);
	       break;
       }
""
The condition doesn't account for the valid range of the serial->port buffer, which is from 0 to serial->num_ports - 1. When newport is equal to serial->num_ports, the assignment of "port" in the following code is out-of-bounds and NULL:
""
       serial_priv->current_port = newport;
       port = serial->port[serial_priv->current_port];
""

Quoted 8 lines

The fix checks if newport is greater than or equal to serial->num_ports indicating it is out-of-bounds.
Reported-by syzbot <[email protected]>
Closes https://syzkaller.appspot.com/bug?extid=506479ebf12fe435d01a
Tested-by syzbot <[email protected]>
Tested-by Qasim Ijaz <[email protected]>
Signed-off-by Qasim Ijaz <[email protected]>

Your signed-off-by does not match your "From:" line :(

Also, no need to add a tested-by when you sign off on your own patch, that is usually implied.

thanks,

greg k-h

Qasim Ijaz

author

2025-01-12 10:14 UTC #2 lore raw

Quoted 33 lines · Greg Kroah-Hartman

Quoted 23 lines · qasdev
This patch addresses a null-ptr-deref in qt2_process_read_urb() due to an incorrect bounds check in the following:
""
       if (newport > serial->num_ports) {
	       dev_err(&port->dev,
		       "%s - port change to invalid port: %i\n",
		       __func__, newport);
	       break;
       }
""
The condition doesn't account for the valid range of the serial->port buffer, which is from 0 to serial->num_ports - 1. When newport is equal to serial->num_ports, the assignment of "port" in the following code is out-of-bounds and NULL:
""
       serial_priv->current_port = newport;
       port = serial->port[serial_priv->current_port];
""
Quoted 8 lines
The fix checks if newport is greater than or equal to serial->num_ports indicating it is out-of-bounds.
Reported-by syzbot <[email protected]>
Closes https://syzkaller.appspot.com/bug?extid=506479ebf12fe435d01a
Tested-by syzbot <[email protected]>
Tested-by Qasim Ijaz <[email protected]>
Signed-off-by Qasim Ijaz <[email protected]>

Quoted 8 lines

Your signed-off-by does not match your "From:" line :(
Also, no need to add a tested-by when you sign off on your own patch, that is usually implied.
thanks,
greg k-h

Hi Greg,

Thank you for your feedback on my patch. I’ve sent a v2 patch in a new thread, addressing the issues with the From: line and removing the redundant Tested-by tag.

Best regards,
Qasim