[PATCH v3] USB: serial: quatech2: Fix null-ptr-deref in qt2_process_read_urb()

[PATCH v3] USB: serial: quatech2: Fix null-ptr-deref in qt2_process_read_urb()

[Qasim Ijaz]

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
Fixes: f7a33e608d9a ("USB: serial: add quatech2 usb to serial driver")
Cc: <stable@vger.kernel.org>      # 3.5
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);
-- 
2.39.5

Re: [PATCH v3] USB: serial: quatech2: Fix null-ptr-deref in qt2_process_read_urb()

[Johan Hovold]

On Mon, Jan 13, 2025 at 06:00:34PM +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
> Fixes: f7a33e608d9a ("USB: serial: add quatech2 usb to serial driver")
> Cc: <stable@vger.kernel.org>      # 3.5
> Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
> ---

Thanks for the update. I've applied the patch now after adding Greg's
Reviewed-by tag (for v2).

For your future contributions, try to remember to include any
Reviewed-by or Tested-by tags when updating the patch unless the changes
are non-trivial.

There should typically also be a short change log here under the ---
line to indicate what changes from previous versions.

It is also encouraged to write the commit message in imperative mood
(add, change, fix) and to avoid the phrase "this patch". There are some
more details in

	Documentation/process/submitting-patches.rst

Something to keep in mind for the future, but this patch already looks
really good.

Johan

Re: [PATCH v3] USB: serial: quatech2: Fix null-ptr-deref in qt2_process_read_urb()

[Qasim Ijaz]

On Tue, Jan 14, 2025 at 10:47:33AM +0100, Johan Hovold wrote:
> On Mon, Jan 13, 2025 at 06:00:34PM +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
> > Fixes: f7a33e608d9a ("USB: serial: add quatech2 usb to serial driver")
> > Cc: <stable@vger.kernel.org>      # 3.5
> > Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
> > ---
> 
> Thanks for the update. I've applied the patch now after adding Greg's
> Reviewed-by tag (for v2).
> 
> For your future contributions, try to remember to include any
> Reviewed-by or Tested-by tags when updating the patch unless the changes
> are non-trivial.
> 
> There should typically also be a short change log here under the ---
> line to indicate what changes from previous versions.
> 
> It is also encouraged to write the commit message in imperative mood
> (add, change, fix) and to avoid the phrase "this patch". There are some
> more details in
> 
> 	Documentation/process/submitting-patches.rst
> 
> Something to keep in mind for the future, but this patch already looks
> really good.
> 
> Johan

Hi Johan,

Thanks for reviewing and applying the patch. I appreciate the guidance on patch style and process, and I'll incorporate your suggestions in future submissions.

Best regards,
Qasim