| Message ID | ad1ee829-401a-d051-1da8-f9e01caa7b85@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] How else could a malicious device sabotage endpoints for usbnet | expand |
On Thu, Dec 09, 2021 at 04:33:29PM +0100, Oliver Neukum wrote: > Hi, > > I have checked for type, direction and number of endpoints. > But I keep thinking that I have overlooked a way to make broken > endpoint descriptors. Any suggestions? > > Regards > Oliver > > >From 853e421630f82fb3b7005ad0b294c091a064ac39 Mon Sep 17 00:00:00 2001 > From: Oliver Neukum <oneukum@suse.com> > Date: Thu, 18 Nov 2021 18:15:03 +0100 > Subject: [PATCH] usbnet: sanity check for endpoint types > > A malicious device can pretend to be a device with a known > configuration of endpoints yet present endpoints of the wrong type > or too few or none at all. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > --- > drivers/net/usb/usbnet.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index 9a6450f796dc..b1f93810a6f3 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -91,6 +91,31 @@ static const char * const usbnet_event_names[] = { > [EVENT_NO_IP_ALIGN] = "EVENT_NO_IP_ALIGN", > }; > > +bool usbnet_validate_endpoints(struct usbnet *dev, struct usb_interface *intf, const struct driver_info *info) > +{ > + struct usb_host_interface *alt = intf->cur_altsetting; > + struct usb_host_endpoint *e; > + int num_endpoints = alt->desc.bNumEndpoints; > + > + if (info->in > num_endpoints) > + return false; > + e = alt->endpoint + info->in; > + if (!e) > + return false; > + if (!usb_endpoint_is_bulk_in(&e->desc)) > + return false; > + > + if (info->out > num_endpoints) > + return false; > + e = alt->endpoint + info->out; > + if (!e) > + return false; > + if (!usb_endpoint_is_bulk_out(&e->desc)) > + return false; > + > + return true; Why not use usb_find_common_endpoints() and/or the other helper functions instead? that's what they were created for. thanks, greg k-h
On 09.12.21 16:47, Greg KH wrote: > > Why not use usb_find_common_endpoints() and/or the other helper > functions instead? that's what they were created for. Hi, which one would I use? In this case I already know the endpoints to be verified. Regards Oliver
On Wed, Dec 15, 2021 at 03:47:55PM +0100, Oliver Neukum wrote: > > On 09.12.21 16:47, Greg KH wrote: > > > > Why not use usb_find_common_endpoints() and/or the other helper > > functions instead? that's what they were created for. > > Hi, > > which one would I use? In this case I already know the endpoints > to be verified. I have no context here so I have no idea, sorry. greg k-h
On 15.12.21 15:57, Greg KH wrote: > On Wed, Dec 15, 2021 at 03:47:55PM +0100, Oliver Neukum wrote: >> On 09.12.21 16:47, Greg KH wrote: >>> Why not use usb_find_common_endpoints() and/or the other helper >>> functions instead? that's what they were created for. >> Hi, >> >> which one would I use? In this case I already know the endpoints >> to be verified. > I have no context here so I have no idea, sorry. usbnet has three ways to match the endpoints 1) the subdriver provides a method 2) a heuristic to find the endpoints is used (that should be converted to the new API) 3) they are given nummerically by the subdriver It turns out that #3 needs to verify the endpoints against malicious devices. So the following questions a) should that verification go into usbcore b) what possible ways for a malicious device to spoof us can you come up with Regards Oliver
On Thu, Dec 16, 2021 at 11:16:26AM +0100, Oliver Neukum wrote: > > On 15.12.21 15:57, Greg KH wrote: > > On Wed, Dec 15, 2021 at 03:47:55PM +0100, Oliver Neukum wrote: > >> On 09.12.21 16:47, Greg KH wrote: > >>> Why not use usb_find_common_endpoints() and/or the other helper > >>> functions instead? that's what they were created for. > >> Hi, > >> > >> which one would I use? In this case I already know the endpoints > >> to be verified. > > I have no context here so I have no idea, sorry. > > usbnet has three ways to match the endpoints > > 1) the subdriver provides a method > > 2) a heuristic to find the endpoints is used (that should be converted > to the new API) > > 3) they are given nummerically by the subdriver > > It turns out that #3 needs to verify the endpoints against malicious > devices. > So the following questions > > a) should that verification go into usbcore the usb_find_common_endpoints() functions are in the usbcore for drivers to use for this type of problem. > b) what possible ways for a malicious device to spoof us can you come > up with Start with: - invalid endpoint sizes and types - invalid data being sent on valid endpoint types and you will catch almost all possible errors. thanks, greg k-h
On 21.12.21 08:54, Greg KH wrote: > On Thu, Dec 16, 2021 at 11:16:26AM +0100, Oliver Neukum wrote: >> >> 2) a heuristic to find the endpoints is used (that should be converted >> to the new API) >> >> 3) they are given nummerically by the subdriver >> >> It turns out that #3 needs to verify the endpoints against malicious >> devices. >> So the following questions >> >> a) should that verification go into usbcore > the usb_find_common_endpoints() functions are in the usbcore for drivers > to use for this type of problem. That API insist on finding the endpoints. It is a heuristic, so we need to have a fallback in case it fails. >> b) what possible ways for a malicious device to spoof us can you come >> up with > Start with: > - invalid endpoint sizes and types > - invalid data being sent on valid endpoint types > and you will catch almost all possible errors. > OK. But I still need a way to do verification _only_. Regards Oliver
From 853e421630f82fb3b7005ad0b294c091a064ac39 Mon Sep 17 00:00:00 2001 From: Oliver Neukum <oneukum@suse.com> Date: Thu, 18 Nov 2021 18:15:03 +0100 Subject: [PATCH] usbnet: sanity check for endpoint types A malicious device can pretend to be a device with a known configuration of endpoints yet present endpoints of the wrong type or too few or none at all. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/net/usb/usbnet.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 9a6450f796dc..b1f93810a6f3 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -91,6 +91,31 @@ static const char * const usbnet_event_names[] = { [EVENT_NO_IP_ALIGN] = "EVENT_NO_IP_ALIGN", }; +bool usbnet_validate_endpoints(struct usbnet *dev, struct usb_interface *intf, const struct driver_info *info) +{ + struct usb_host_interface *alt = intf->cur_altsetting; + struct usb_host_endpoint *e; + int num_endpoints = alt->desc.bNumEndpoints; + + if (info->in > num_endpoints) + return false; + e = alt->endpoint + info->in; + if (!e) + return false; + if (!usb_endpoint_is_bulk_in(&e->desc)) + return false; + + if (info->out > num_endpoints) + return false; + e = alt->endpoint + info->out; + if (!e) + return false; + if (!usb_endpoint_is_bulk_out(&e->desc)) + return false; + + return true; +} + /* handles CDC Ethernet and many other network "bulk data" interfaces */ int usbnet_get_endpoints(struct usbnet *dev, struct usb_interface *intf) { @@ -1772,6 +1797,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod) } else if (!info->in || !info->out) status = usbnet_get_endpoints (dev, udev); else { + if (!usbnet_validate_endpoints(dev, udev, info)) + goto out3; dev->in = usb_rcvbulkpipe (xdev, info->in); dev->out = usb_sndbulkpipe (xdev, info->out); if (!(info->flags & FLAG_NO_SETINT)) -- 2.26.2