[PATCH] USB: hub: make sure stale buffers are not enumerated

[PATCH] USB: hub: make sure stale buffers are not enumerated

[Oliver Neukum]

Quoting Alan Stern on why we cannot just check errors:

The operation carried out here is deliberately unsafe (for full-speed
devices).  It is made before we know the actual maxpacket size for ep0,
and as a result it might return an error code even when it works okay.
This shouldn't happen, but a lot of USB hardware is unreliable.

Therefore we must not ignore the result merely because r < 0.  If we do
that, the kernel might stop working with some devices.

He is absolutely right. However, we must make sure that in case
we read nothing or a short answer, the buffer contains nothing
that can be misinterpreted as a valid answer.
So we have to zero it before we use it for IO.

Reported-by: liulongfang <liulongfang@huawei.com>
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/core/hub.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a739403a9e45..9772716925c3 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4873,7 +4873,8 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
 			}
 
 #define GET_DESCRIPTOR_BUFSIZE	64
-			buf = kmalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO);
+			/* zeroed so we don't operate on a stale buffer on errors */
+			buf = kzalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO);
 			if (!buf) {
 				retval = -ENOMEM;
 				continue;
-- 
2.41.0

Re: [PATCH] USB: hub: make sure stale buffers are not enumerated

[Johan Hovold]

On Mon, Jul 24, 2023 at 02:40:57PM +0200, Oliver Neukum wrote:
> Quoting Alan Stern on why we cannot just check errors:
> 
> The operation carried out here is deliberately unsafe (for full-speed
> devices).  It is made before we know the actual maxpacket size for ep0,
> and as a result it might return an error code even when it works okay.
> This shouldn't happen, but a lot of USB hardware is unreliable.
> 
> Therefore we must not ignore the result merely because r < 0.  If we do
> that, the kernel might stop working with some devices.
> 
> He is absolutely right. However, we must make sure that in case
> we read nothing or a short answer, the buffer contains nothing
> that can be misinterpreted as a valid answer.
> So we have to zero it before we use it for IO.

This patch is neither correct or needed. The current implementation sets
	
	buf->bMaxPacketSize0 = 0

before reading the descriptor and makes sure that that field is non-zero
before accessing buf->bDescriptorType which lies before bMaxPacketSize0.

It may be subtle, but it looks correct.

Johan

Re: [PATCH] USB: hub: make sure stale buffers are not enumerated

[Alan Stern]

On Mon, Jul 24, 2023 at 02:40:57PM +0200, Oliver Neukum wrote:
> Quoting Alan Stern on why we cannot just check errors:
> 
> The operation carried out here is deliberately unsafe (for full-speed
> devices).  It is made before we know the actual maxpacket size for ep0,
> and as a result it might return an error code even when it works okay.
> This shouldn't happen, but a lot of USB hardware is unreliable.
> 
> Therefore we must not ignore the result merely because r < 0.  If we do
> that, the kernel might stop working with some devices.
> 
> He is absolutely right. However, we must make sure that in case
> we read nothing or a short answer, the buffer contains nothing
> that can be misinterpreted as a valid answer.
> So we have to zero it before we use it for IO.
> 
> Reported-by: liulongfang <liulongfang@huawei.com>
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/usb/core/hub.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index a739403a9e45..9772716925c3 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4873,7 +4873,8 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
>  			}
>  
>  #define GET_DESCRIPTOR_BUFSIZE	64
> -			buf = kmalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO);
> +			/* zeroed so we don't operate on a stale buffer on errors */
> +			buf = kzalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO);
>  			if (!buf) {
>  				retval = -ENOMEM;
>  				continue;

There is no need for kzalloc.

The only accesses we do to buf (besides feeding it to usb_control_msg) 
are to read buf->bMaxPacketSize0 and buf->bDescriptorType.  The only 
field that actually needs to be initialized is bMaxPacketSize0, and the 
code already sets it to 0 before calling usb_control_msg.  Furthermore, 
we don't try to read bDescriptorType if bMaxPacketSize0 is invalid after 
the I/O operation.

I guess if you really want to be safe, you could do:

-			udev->descriptor.bMaxPacketSize0 =
-					buf->bMaxPacketSize0;
+			if (r == 0)
+				udev->descriptor.bMaxPacketSize0 =
+						buf->bMaxPacketSize0;

at line 4918.  But even that's not necessary, since the following call 
to hub_port_reset doesn't use udev->descriptor.bMaxPacketSize0, so it 
doesn't matter if that field contains garbage.

Alan Stern

Re: [PATCH] USB: hub: make sure stale buffers are not enumerated

[Oliver Neukum]

On 24.07.23 15:52, Johan Hovold wrote:

> 
> This patch is neither correct or needed. The current implementation sets
> 	
> 	buf->bMaxPacketSize0 = 0
> 
> before reading the descriptor and makes sure that that field is non-zero
> before accessing buf->bDescriptorType which lies before bMaxPacketSize0.
> 
> It may be subtle, but it looks correct.

True, but I am afraid not sufficient. It neglects the case of getting
a partial read. That is

buf->bMaxPacketSize0

can be genuine, but the later test
if (buf->bDescriptorType ==
             USB_DT_DEVICE) {

still spuriously succeed

	Regards
		Oliver

Re: [PATCH] USB: hub: make sure stale buffers are not enumerated

[Alan Stern]

On Mon, Jul 24, 2023 at 04:24:55PM +0200, Oliver Neukum wrote:
> On 24.07.23 15:52, Johan Hovold wrote:
> 
> > 
> > This patch is neither correct or needed. The current implementation sets
> > 	
> > 	buf->bMaxPacketSize0 = 0
> > 
> > before reading the descriptor and makes sure that that field is non-zero
> > before accessing buf->bDescriptorType which lies before bMaxPacketSize0.
> > 
> > It may be subtle, but it looks correct.
> 
> True, but I am afraid not sufficient. It neglects the case of getting
> a partial read. That is
> 
> buf->bMaxPacketSize0
> 
> can be genuine, but the later test
> if (buf->bDescriptorType ==
>             USB_DT_DEVICE) {
> 
> still spuriously succeed

How can it?  bDescriptorType is at the start of the device descriptor, 
whereas bMaxPacketSize0 is more towards the end.  If the later part get 
transferred from the device, the earlier part must have been transferred 
as well.  Even if the transfer was short.

Alan Stern

Re: [PATCH] USB: hub: make sure stale buffers are not enumerated

[Oliver Neukum]

On 24.07.23 16:29, Alan Stern wrote:
  
> How can it?  bDescriptorType is at the start of the device descriptor,
> whereas bMaxPacketSize0 is more towards the end.  If the later part get
> transferred from the device, the earlier part must have been transferred
> as well.  Even if the transfer was short.

Do we really guarantee that in an error case the buffer is filled from front
to back?

	Regards
		Oliver

Re: [PATCH] USB: hub: make sure stale buffers are not enumerated

[Alan Stern]

On Mon, Jul 24, 2023 at 04:31:08PM +0200, Oliver Neukum wrote:
> On 24.07.23 16:29, Alan Stern wrote:
> > How can it?  bDescriptorType is at the start of the device descriptor,
> > whereas bMaxPacketSize0 is more towards the end.  If the later part get
> > transferred from the device, the earlier part must have been transferred
> > as well.  Even if the transfer was short.
> 
> Do we really guarantee that in an error case the buffer is filled from front
> to back?

We don't guarantee anything in an error case.  But for short transfers 
with no other errors, yes, this is guaranteed.

However if you still want to address this concern, just set 
buf->bDescriptorType to 0 at the same time as buf->bMaxPacketSize0.

Alan Stern