| Message ID | Z4ORrl4sZBhPiu2p@qasdev.system (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] usb: Fix null-ptr-deref in qt2_process_read_urb() | expand |
On Sun, Jan 12, 2025 at 09:55:58AM +0000, Qasim Ijaz wrote: > 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 <syzbot+506479ebf12fe435d01a@syzkaller.appspotmail.com> > Closes: https://syzkaller.appspot.com/bug?extid=506479ebf12fe435d01a Oh wow, syzbot is now starting to fuzz random USB devices? And this is the only thing it found for this device? That's not all that bad, I expected much worse. > Signed-off-by: Qasim Ijaz <qasdev00@gmail.com> > --- > drivers/usb/serial/quatech2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/serial/quatech2.c b/drivers/usb/serial/quatech2.c > index a317bdbd00ad..72fe83a6c978 100644 > --- a/drivers/usb/serial/quatech2.c > +++ b/drivers/usb/serial/quatech2.c > @@ -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); Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Sun, Jan 12, 2025 at 09:55:58AM +0000, Qasim Ijaz wrote: > This patch addresses a null-ptr-deref in qt2_process_read_urb() due to > an incorrect bounds check in the following: Thanks for the fix. This looks correct, but your patch is whitespace damaged (tabs replaced with spaces) and does not apply. Can you please fix your mail setup and resend a v3? (Make sure to send it to yourself first.) When doing so, please use the common patch prefix for this driver: USB: serial: quatech2: > "" You can drop these (markup?) quotes. > 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 <syzbot+506479ebf12fe435d01a@syzkaller.appspotmail.com> > Closes: https://syzkaller.appspot.com/bug?extid=506479ebf12fe435d01a > Signed-off-by: Qasim Ijaz <qasdev00@gmail.com> And please include a Fixes and CC stable tag: Fixes: f7a33e608d9a ("USB: serial: add quatech2 usb to serial driver") Cc: stable@vger.kernel.org # 3.5 Johan
diff --git a/drivers/usb/serial/quatech2.c b/drivers/usb/serial/quatech2.c index a317bdbd00ad..72fe83a6c978 100644 --- a/drivers/usb/serial/quatech2.c +++ b/drivers/usb/serial/quatech2.c @@ -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);
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 <syzbot+506479ebf12fe435d01a@syzkaller.appspotmail.com> Closes: https://syzkaller.appspot.com/bug?extid=506479ebf12fe435d01a Signed-off-by: Qasim Ijaz <qasdev00@gmail.com> --- drivers/usb/serial/quatech2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)