OpenBIOS April 2024

openbios@openbios.org

3 participants
5 discussions

[PATCH] cuda: fix len calculation in cuda_adb_req
by Amadeusz Sławiński 28 Apr '24

28 Apr '24

When advancing buffer by 1, len should be shorthened by appropriate amount, instead of setting it to -1. Found with cppcheck. Signed-off-by: Amadeusz Sławiński <amade(a)asmblr.net> --- drivers/cuda.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/cuda.c b/drivers/cuda.c index 86d2ddb..93a935a 100644 --- a/drivers/cuda.c +++ b/drivers/cuda.c @@ -162,7 +162,7 @@ static int cuda_adb_req (void *host, const uint8_t *snd_buf, int len, } } else { pos = buffer + 1; - len = -1; + len -= 1; } memcpy(rcv_buf, pos, len); -- 2.44.0

1 0

Re: [PATCH 4/7] drivers/usb.c: rework descriptor read logic in get_descriptor()
by Mark Cave-Ayland 25 Apr '24

25 Apr '24

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(a)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.

1 0

[PATCH 0/7] Build updates and improvements
by Mark Cave-Ayland 24 Apr '24

24 Apr '24

This series is based upon Amadeusz Sławiński's recent series at https://mail.coreboot.org/hyperkitty/list/openbios@openbios.org/thread/OLS2… and updates the code to fix build issues on gcc 12 (as used by Debian stable), whilst also correcting the compiler flags so that the binaries are generated correctly. Finally the github workflows are updated to use the Debian cross-compilers, and the GitHub release action changed to use an actively maintained fork. [MCA: switching to the Debian cross-compilers substantially lowers the barrier to entry for people setting up their own compile environments and also brings OpenBIOS in line with QEMU's own CI cross-compiler containers]. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland(a)ilande.co.uk> Mark Cave-Ayland (7): .github/workflows: update to latest action releases arch/ppc/qemu/init.c: work around GCC bug for ppc64_patch_handlers() packages/pc-parts.c: fix bug in extended partition logic drivers/usb.c: rework descriptor read logic in get_descriptor() switch-arch: disable generation of position independent code (-fno-pic) docker: switch from kernel crosstools compilers to Debian cross-compilers .github/workflows: migrate release action to crowbarmaster/GH-Automatic-Releases .github/workflows/build-openbios-builder.yml | 8 ++++---- .github/workflows/main.yml | 8 ++++---- .github/workflows/release.yml | 8 ++++---- arch/ppc/qemu/init.c | 12 ++++++++++++ config/scripts/switch-arch | 10 +++++----- docker/Dockerfile.builder | 18 ++---------------- drivers/usb.c | 17 ++++++++--------- packages/pc-parts.c | 7 +++++-- 8 files changed, 44 insertions(+), 44 deletions(-) -- 2.39.2

2 11

[PATCH] kernel: fix spelling of endianness
by Mark Cave-Ayland 24 Apr '24

24 Apr '24

This is based upon a similar patch currently being carried in the QEMU Debian package sources. Reported-by: Michael Tokarev <mjt(a)tls.msk.ru> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland(a)ilande.co.uk> --- kernel/bootstrap.c | 4 ++-- kernel/dict.c | 2 +- kernel/include/dict.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/kernel/bootstrap.c b/kernel/bootstrap.c index 9bc94bb..8ad3efe 100644 --- a/kernel/bootstrap.c +++ b/kernel/bootstrap.c @@ -171,9 +171,9 @@ static void write_dictionary(const char *filename) .version = 2, .cellsize = sizeof(ucell), #ifdef CONFIG_BIG_ENDIAN - .endianess = -1, + .endianness = -1, #else - .endianess = 0, + .endianness = 0, #endif .checksum = 0, .compression = 0, diff --git a/kernel/dict.c b/kernel/dict.c index 0986cb1..51afc67 100644 --- a/kernel/dict.c +++ b/kernel/dict.c @@ -252,7 +252,7 @@ void dump_header(dictionary_header_t *header) printk("OpenBIOS dictionary:\n"); printk(" version: %d\n", header->version); printk(" cellsize: %d\n", header->cellsize); - printk(" endianess: %s\n", header->endianess?"big":"little"); + printk(" endianness: %s\n", header->endianness?"big":"little"); printk(" compression: %s\n", header->compression?"yes":"no"); printk(" relocation: %s\n", header->relocation?"yes":"no"); printk(" checksum: %08x\n", target_long(header->checksum)); diff --git a/kernel/include/dict.h b/kernel/include/dict.h index 749fd6f..1e735fa 100644 --- a/kernel/include/dict.h +++ b/kernel/include/dict.h @@ -27,7 +27,7 @@ typedef struct dictionary_header { char signature[8]; u8 version; u8 cellsize; - u8 endianess; + u8 endianness; u8 compression; u8 relocation; u8 reserved[3]; -- 2.39.2

1 0

[PATCH 0/7] Fix build issues
by Amadeusz Sławiński 24 Apr '24

24 Apr '24

Set of patches for few problems found when trying to build project. Also enable some additional checks and also fix issues found. Amadeusz Sławiński (7): adb_kbd.c: Fix function key definitions Change malloc to take size_t as argument Label fallthroughs in switch()es Makefile.target: Enable few additional warnings Disable stack protector on all architectures bootstrap: Check result of fread Tell linker that stack is not executable Makefile.target | 2 ++ arch/amd64/lib.c | 2 +- arch/ppc/build.xml | 8 ++++---- arch/ppc/ofmem.c | 2 +- arch/ppc/qemu/ofmem.c | 2 +- arch/sparc32/lib.c | 2 +- arch/sparc64/lib.c | 2 +- arch/x86/lib.c | 2 +- config/scripts/switch-arch | 14 +++++++------- drivers/adb_bus.c | 2 +- drivers/adb_kbd.c | 2 +- include/libc/stdlib.h | 2 +- kernel/bootstrap.c | 6 +++++- libc/vsprintf.c | 1 + 14 files changed, 28 insertions(+), 21 deletions(-) -- 2.43.2

2 9