| Message ID | 20230724124057.12975-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: hub: make sure stale buffers are not enumerated | expand |
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
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
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
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
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
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
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;
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(-)