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 mark.cave-ayland@ilande.co.uk
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.