Re: [PATCH] USB: cdc-acm: check for descriptors with invalid length
On Tue, Jan 27, 2015 at 08:36:07PM +0100, Oliver Neukum wrote: > Hi, > > could you test and review this? $ git am ~/0001-cdc-acm-add-sanity-checks.patch Applying: cdc-acm: add sanity checks /home/adamlee/ubuntu-vivid/.git/rebase-apply/patch:26: trailing whitespace. goto next_desc; warning: 1 line adds whitespace errors. It works on my env, also the logic of USB_CDC_CALL_MANAGEMENT_TYPE part is the same as my patch, so ACK. Signed-off-by: Adam Lee -- Adam Lee http://adam8157.info -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: cdc-acm: check for descriptors with invalid length
On Fri, 2015-01-23 at 23:08 +0800, Adam Lee wrote:
Hi,
could you test and review this?
Regards
Oliver
>From 1ddabb5731135fe43f67f3b4645445c2de06dada Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Tue, 13 Jan 2015 16:55:52 +0100
Subject: [PATCH] cdc-acm: add sanity checks
Check the special CDC headers for a plausible minimum length.
Another big operating systems ignores such garbage.
Signed-off-by: Oliver Neukum
---
drivers/usb/class/cdc-acm.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 546a17e8..3dd540d 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1091,6 +1091,7 @@ static int acm_probe(struct usb_interface *intf,
unsigned long quirks;
int num_rx_buf;
int i;
+ unsigned int elength = 0;
int combined_interfaces = 0;
struct device *tty_dev;
int rv = -ENOMEM;
@@ -1136,9 +1137,12 @@ static int acm_probe(struct usb_interface *intf,
dev_err(&intf->dev, "skipping garbage\n");
goto next_desc;
}
+ elength = buffer[0];
switch (buffer[2]) {
case USB_CDC_UNION_TYPE: /* we've found it */
+ if (elength < sizeof(struct usb_cdc_union_desc))
+goto next_desc;
if (union_header) {
dev_err(&intf->dev, "More than one "
"union descriptor, skipping ...\n");
@@ -1147,29 +1151,36 @@ static int acm_probe(struct usb_interface *intf,
union_header = (struct usb_cdc_union_desc *)buffer;
break;
case USB_CDC_COUNTRY_TYPE: /* export through sysfs*/
+ if (elength < sizeof(struct usb_cdc_country_functional_desc))
+goto next_desc;
cfd = (struct usb_cdc_country_functional_desc *)buffer;
break;
case USB_CDC_HEADER_TYPE: /* maybe check version */
break; /* for now we ignore it */
case USB_CDC_ACM_TYPE:
+ if (elength < 4)
+goto next_desc;
ac_management_function = buffer[3];
break;
case USB_CDC_CALL_MANAGEMENT_TYPE:
+ if (elength < 5)
+goto next_desc;
call_management_function = buffer[3];
call_interface_num = buffer[4];
break;
default:
- /* there are LOTS more CDC descriptors that
+ /*
+ * there are LOTS more CDC descriptors that
* could legitimately be found here.
*/
dev_dbg(&intf->dev, "Ignoring descriptor: "
- "type %02x, length %d\n",
- buffer[2], buffer[0]);
+ "type %02x, length %ud\n",
+ buffer[2], elength);
break;
}
next_desc:
- buflen -= buffer[0];
- buffer += buffer[0];
+ buflen -= elength;
+ buffer += elength;
}
if (!union_header) {
--
1.8.4.5
Re: [PATCH] USB: cdc-acm: check for descriptors with invalid length
On Fri, 2015-01-23 at 23:02 +0800, Adam Lee wrote:
> On Fri, Jan 23, 2015 at 09:44:38AM +0100, Oliver Neukum wrote:
> > On Fri, 2015-01-23 at 14:54 +0800, Adam Lee wrote:
> > > In my scenario(pull that device then plug into another usb port while
> > > booting), invalid descriptor accessing happens just like Simon reported.
> > > Checking length and ignoring the invalid descriptors works.
> >
> > Hi,
> >
> > interesting. Have a look at what I sent Greg last week.
> >
> > Regards
> > Oliver
> >
> > From 14f823958cb3e99646419f743f3176d1059c3282 Mon Sep 17 00:00:00 2001
> > From: Oliver Neukum
> > Date: Tue, 13 Jan 2015 16:55:52 +0100
> > Subject: [PATCH 1/4] cdc-acm: add sanity checks
> >
> > Check the special CDC headers for a plausible minimum length.
> > Another big operating systems ignores such garbage.
> >
> > Signed-off-by: Oliver Neukum
> > ---
> >
> > ...
> >
> > case USB_CDC_ACM_TYPE:
> > + if (elength < 3)
> > + break;
> > ac_management_function = buffer[3];
> > break;
> > case USB_CDC_CALL_MANAGEMENT_TYPE:
> > + if (elength < 4)
> > + break;
> > call_management_function = buffer[3];
> > call_interface_num = buffer[4];
> > break;
> >
> > ...
> >
> > next_desc:
> > - buflen -= buffer[0];
> > - buffer += buffer[0];
> > + buflen -= elength;
> > + buffer += elength;
> > }
> >
> > if (!union_header) {
> > --
> > 1.8.4.5
>
> Hi, Oliver
>
> Shouldn't the length checks be "if (elength < 4)" and "if (elength <
> 5)"? Think the logic of "buflen -= elength" and "buffer += elength",
> elength must count itself(buffer[0]) in.
>
Yes, you are right. I am making a version for you to test.
Regards
Oliver
--
Oliver Neukum
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: cdc-acm: check for descriptors with invalid length
On Fri, Jan 23, 2015 at 11:02:19PM +0800, Adam Lee wrote:
> On Fri, Jan 23, 2015 at 09:44:38AM +0100, Oliver Neukum wrote:
> > On Fri, 2015-01-23 at 14:54 +0800, Adam Lee wrote:
> > > In my scenario(pull that device then plug into another usb port while
> > > booting), invalid descriptor accessing happens just like Simon reported.
> > > Checking length and ignoring the invalid descriptors works.
> >
> > Hi,
> >
> > interesting. Have a look at what I sent Greg last week.
> >
> > Regards
> > Oliver
> >
> > From 14f823958cb3e99646419f743f3176d1059c3282 Mon Sep 17 00:00:00 2001
> > From: Oliver Neukum
> > Date: Tue, 13 Jan 2015 16:55:52 +0100
> > Subject: [PATCH 1/4] cdc-acm: add sanity checks
> >
> > Check the special CDC headers for a plausible minimum length.
> > Another big operating systems ignores such garbage.
> >
> > Signed-off-by: Oliver Neukum
> > ---
> >
> > ...
> >
> > case USB_CDC_ACM_TYPE:
> > + if (elength < 3)
> > + break;
> > ac_management_function = buffer[3];
> > break;
> > case USB_CDC_CALL_MANAGEMENT_TYPE:
> > + if (elength < 4)
> > + break;
> > call_management_function = buffer[3];
> > call_interface_num = buffer[4];
> > break;
> >
> > ...
> >
> > next_desc:
> > - buflen -= buffer[0];
> > - buffer += buffer[0];
> > + buflen -= elength;
> > + buffer += elength;
> > }
> >
> > if (!union_header) {
> > --
> > 1.8.4.5
>
> Hi, Oliver
>
> Shouldn't the length checks be "if (elength < 4)" and "if (elength <
> 5)"? Think the logic of "buflen -= elength" and "buffer += elength",
> elength must count itself(buffer[0]) in.
Also I think you should "goto next_desc" but not "break".
--
Adam Lee
http://adam8157.info
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: cdc-acm: check for descriptors with invalid length
On Fri, Jan 23, 2015 at 09:44:38AM +0100, Oliver Neukum wrote:
> On Fri, 2015-01-23 at 14:54 +0800, Adam Lee wrote:
> > In my scenario(pull that device then plug into another usb port while
> > booting), invalid descriptor accessing happens just like Simon reported.
> > Checking length and ignoring the invalid descriptors works.
>
> Hi,
>
> interesting. Have a look at what I sent Greg last week.
>
> Regards
> Oliver
>
> From 14f823958cb3e99646419f743f3176d1059c3282 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum
> Date: Tue, 13 Jan 2015 16:55:52 +0100
> Subject: [PATCH 1/4] cdc-acm: add sanity checks
>
> Check the special CDC headers for a plausible minimum length.
> Another big operating systems ignores such garbage.
>
> Signed-off-by: Oliver Neukum
> ---
>
> ...
>
> case USB_CDC_ACM_TYPE:
> + if (elength < 3)
> + break;
> ac_management_function = buffer[3];
> break;
> case USB_CDC_CALL_MANAGEMENT_TYPE:
> + if (elength < 4)
> + break;
> call_management_function = buffer[3];
> call_interface_num = buffer[4];
> break;
>
> ...
>
> next_desc:
> - buflen -= buffer[0];
> - buffer += buffer[0];
> + buflen -= elength;
> + buffer += elength;
> }
>
> if (!union_header) {
> --
> 1.8.4.5
Hi, Oliver
Shouldn't the length checks be "if (elength < 4)" and "if (elength <
5)"? Think the logic of "buflen -= elength" and "buffer += elength",
elength must count itself(buffer[0]) in.
--
Adam Lee
http://adam8157.info
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: cdc-acm: check for descriptors with invalid length
On Fri, 2015-01-23 at 14:54 +0800, Adam Lee wrote:
> In my scenario(pull that device then plug into another usb port while
> booting), invalid descriptor accessing happens just like Simon reported.
> Checking length and ignoring the invalid descriptors works.
Hi,
interesting. Have a look at what I sent Greg last week.
Regards
Oliver
>From 14f823958cb3e99646419f743f3176d1059c3282 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Tue, 13 Jan 2015 16:55:52 +0100
Subject: [PATCH 1/4] cdc-acm: add sanity checks
Check the special CDC headers for a plausible minimum length.
Another big operating systems ignores such garbage.
Signed-off-by: Oliver Neukum
---
drivers/usb/class/cdc-acm.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 546a17e8..8eadb38 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1091,6 +1091,7 @@ static int acm_probe(struct usb_interface *intf,
unsigned long quirks;
int num_rx_buf;
int i;
+ unsigned int elength = 0;
int combined_interfaces = 0;
struct device *tty_dev;
int rv = -ENOMEM;
@@ -1136,9 +1137,12 @@ static int acm_probe(struct usb_interface *intf,
dev_err(&intf->dev, "skipping garbage\n");
goto next_desc;
}
+ elength = buffer[0];
switch (buffer[2]) {
case USB_CDC_UNION_TYPE: /* we've found it */
+ if (elength < sizeof(struct usb_cdc_union_desc))
+ break;
if (union_header) {
dev_err(&intf->dev, "More than one "
"union descriptor, skipping ...\n");
@@ -1147,14 +1151,20 @@ static int acm_probe(struct usb_interface *intf,
union_header = (struct usb_cdc_union_desc *)buffer;
break;
case USB_CDC_COUNTRY_TYPE: /* export through sysfs*/
+ if (elength < sizeof(struct
usb_cdc_country_functional_desc))
+ break;
cfd = (struct usb_cdc_country_functional_desc *)buffer;
break;
case USB_CDC_HEADER_TYPE: /* maybe check version */
break; /* for now we ignore it */
case USB_CDC_ACM_TYPE:
+ if (elength < 3)
+ break;
ac_management_function = buffer[3];
break;
case USB_CDC_CALL_MANAGEMENT_TYPE:
+ if (elength < 4)
+ break;
call_management_function = buffer[3];
call_interface_num = buffer[4];
break;
@@ -1163,13 +1173,13 @@ static int acm_probe(struct usb_interface *intf,
* could legitimately be found here.
*/
dev_dbg(&intf->dev, "Ignoring descriptor: "
- "type %02x, length %d\n",
- buffer[2], buffer[0]);
+ "type %02x, length %ud\n",
+ buffer[2], elength);
break;
}
next_desc:
- buflen -= buffer[0];
- buffer += buffer[0];
+ buflen -= elength;
+ buffer += elength;
}
if (!union_header) {
--
1.8.4.5
--
Oliver Neukum
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: cdc-acm: check for descriptors with invalid length
In my scenario(pull that device then plug into another usb port while
booting), invalid descriptor accessing happens just like Simon reported.
Checking length and ignoring the invalid descriptors works.
References: https://bugzilla.kernel.org/show_bug.cgi?id=83551
Reported-by: Simon Schubert <2+ker...@0x2c.org>
Cc: stable
Signed-off-by: Adam Lee
---
drivers/usb/class/cdc-acm.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 546a17e8..347c14a 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1155,6 +1155,12 @@ static int acm_probe(struct usb_interface *intf,
ac_management_function = buffer[3];
break;
case USB_CDC_CALL_MANAGEMENT_TYPE:
+ if (buffer[0] < 5) {
+ dev_err(&intf->dev, "Ignoring descriptor with "
+ "invalid length: type %02x, length %d "
+ "instead of 5\n", buffer[2], buffer[0]);
+ goto next_desc;
+ }
call_management_function = buffer[3];
call_interface_num = buffer[4];
break;
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
