Re: [PATCH 4/7] drivers/usb.c: rework descriptor read logic in get_descriptor()

[Mark Cave-Ayland]

On 24/04/2024 15:48, Philippe Mathieu-Daudé wrote:
> Hi Mark,
>
> On 22/4/24 09:54, Mark Cave-Ayland wrote:
> > When compiling with gcc 12 drivers/usb.c generates the following error:
> >
> > /root/drivers/usb.c: In function 'get_descriptor':
> > /root/drivers/usb.c:200:23: error: array subscript 'device_descriptor_t[0]' is 
> > partly outside array bounds of 'u8[8]' {aka 'unsigned char[8]'} [-Werror=array-bounds]
> >    200 |                 if (dd->bMaxPacketSize0 != 0)
> >        |                       ^~
> > /root/drivers/usb.c:181:12: note: object 'buf' of size 8
> >    181 |         u8 buf[8];
> >        |            ^~~
> > cc1: all warnings being treated as errors
> > make[1]: *** [rules.mak:229: target/drivers/usb.o] Error 1
> > make[1]: Leaving directory '/root/obj-ppc'
> >
> > The compiler is complaining that a device_descriptor_t pointer cast to the 8 byte 
> > buffer
> > means that a pointer dereference to some members of the device_descriptor_t could go
> > beyond the end of the 8 byte buffer. Whilst this sounds as if it should be allowed, 
> > some
> > searching suggests that the behaviour of such a cast is undefined.
> >
> > Rework get_descriptor() so that an entire device_descriptor_t is used to store the 
> > first
> > 8 bytes of the incoming descriptor, and update the buf references to use the 
> > equivalent
> > device_descriptor_t fields instead.
> >
> > Signed-off-by: Mark Cave-Ayland <[e-mail]>
> > ---
> >   drivers/usb.c | 17 ++++++++---------
> >   1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/usb.c b/drivers/usb.c
> > index 88b7580..d0e1ae5 100644
> > --- a/drivers/usb.c
> > +++ b/drivers/usb.c
> > @@ -178,7 +178,7 @@ u8 *
> >   get_descriptor (usbdev_t *dev, unsigned char bmRequestType, int descType,
> >           int descIdx, int langID)
> >   {
> > -    u8 buf[8];
> > +    device_descriptor_t dd;
> >       u8 *result;
> >       dev_req_t dr;
> >       int size;
> > @@ -189,24 +189,23 @@ get_descriptor (usbdev_t *dev, unsigned char bmRequestType, 
> > int descType,
> >       dr.wValue = __cpu_to_le16((descType << 8) | descIdx);
> >       dr.wIndex = __cpu_to_le16(langID);
> >       dr.wLength = __cpu_to_le16(8);
> > -    if (dev->controller->control (dev, IN, sizeof (dr), &dr, 8, buf)) {
> > +    if (dev->controller->control (dev, IN, sizeof (dr), &dr, 8, (u8 *)&dd)) {
>
> Still using dalen=8 seems odd. You only fill the first 8 bytes of dd,
> leaving the rest uninitialized. Looking at which fields are used, it
> is OK, but hard to review and possibly bugprone if this function were
> modified in the future.
>
> Why not use sizeof (dd) instead? That still fits into a page, so
> performance overhead is unlikely.
>
> My 2 cents, otherwise LGTM.

Thanks! I did consider whether this would matter, but given that only the first 8 
bytes were accessed it didn't seem to matter where the extra junk at the end of 
device_descriptor_t came from, so I kept it simple. The corresponding file upstream 
is very different from what is in OpenBIOS, so possibly this could be made clearer 
with an update, but that requires quite a bit more work :/
> >           usb_debug ("getting descriptor size (type %x) failed\n",
> >               descType);
> >       }
> >       if (descType == 1) {
> > -        device_descriptor_t *dd = (device_descriptor_t *) buf;
> > -        usb_debug ("maxPacketSize0: %x\n", dd->bMaxPacketSize0);
> > -        if (dd->bMaxPacketSize0 != 0)
> > -            dev->endpoints[0].maxpacketsize = dd->bMaxPacketSize0;
> > +        usb_debug ("maxPacketSize0: %x\n", dd.bMaxPacketSize0);
> > +        if (dd.bMaxPacketSize0 != 0)
> > +            dev->endpoints[0].maxpacketsize = dd.bMaxPacketSize0;
> >       }
> >       /* special case for configuration descriptors: they carry all their
> >          subsequent descriptors with them, and keep the entire size at a
> >          different location */
> > -    size = buf[0];
> > -    if (buf[1] == 2) {
> > -        int realsize = __le16_to_cpu(((unsigned short *) (buf + 2))[0]);
> > +    size = dd.bLength;
> > +    if (dd.bDescriptorType == 2) {
> > +        int realsize = __le16_to_cpu(dd.bcdUSB);
> >           size = realsize;
> >       }
> >       result = malloc (size);

ATB,

Mark.