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@xxxxxxxxxx>
> Signed-off-by: Oliver Neukum <oneukum@xxxxxxxx>
> ---
> 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
