This series is based upon Amadeusz Sławiński's recent series at https://mail.coreboot.org/hyperkitty/list/openbios@openbios.org/thread/OLS25... 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@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(-)
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- .github/workflows/build-openbios-builder.yml | 8 ++++---- .github/workflows/main.yml | 6 +++--- .github/workflows/release.yml | 6 +++--- 3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/.github/workflows/build-openbios-builder.yml b/.github/workflows/build-openbios-builder.yml index b31af8a..9715e50 100644 --- a/.github/workflows/build-openbios-builder.yml +++ b/.github/workflows/build-openbios-builder.yml @@ -17,10 +17,10 @@ jobs:
steps: - name: Checkout repository - uses: actions/checkout@v2 + uses: actions/checkout@v4
- name: Log in to the Container registry - uses: docker/login-action@f054a8b539a109f9f41c372932f1ae047eff08c9 + uses: docker/login-action@v3 with: registry: ${{ env.REGISTRY }} username: ${{ github.actor }} @@ -28,12 +28,12 @@ jobs:
- name: Extract metadata (tags, labels) for Docker id: meta - uses: docker/metadata-action@98669ae865ea3cffbcbaa878cf57c20bbf1c6c38 + uses: docker/metadata-action@v5 with: images: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}
- name: Build and push Docker image - uses: docker/build-push-action@ad44023a93711e3deb337508980b4b5e9bcdc5dc + uses: docker/build-push-action@v5 with: context: . file: docker/Dockerfile.builder diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 41ec7ab..626fc0a 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -16,7 +16,7 @@ jobs: image: ghcr.io/openbios/openbios-builder:master steps: - name: Checkout OpenBIOS - uses: actions/checkout@v2 + uses: actions/checkout@v4
- name: Backup Makefile.target run: cp Makefile.target Makefile.target.orig @@ -44,7 +44,7 @@ jobs: run: "mkdir -p release && mv obj-* release"
- name: Store artifacts - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v4 with: name: openbios-multiarch-latest path: | @@ -84,7 +84,7 @@ jobs: release/obj-sparc64/QEMU,VGA.bin
- name: Prepare pre-release from artifacts - uses: actions/download-artifact@v2 + uses: actions/download-artifact@v4 with: name: openbios-multiarch-latest path: archive diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 908837f..1fc587c 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -16,7 +16,7 @@ jobs: image: ghcr.io/openbios/openbios-builder:master steps: - name: Checkout OpenBIOS - uses: actions/checkout@v2 + uses: actions/checkout@v4
- name: Backup Makefile.target run: cp Makefile.target Makefile.target.orig @@ -44,7 +44,7 @@ jobs: run: "mkdir -p release && mv obj-* release"
- name: Store artifacts - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v4 with: name: "openbios-multiarch-${{ github.ref_name }}" path: | @@ -84,7 +84,7 @@ jobs: release/obj-sparc64/QEMU,VGA.bin
- name: Prepare release from artifacts - uses: actions/download-artifact@v2 + uses: actions/download-artifact@v4 with: name: "openbios-multiarch-${{ github.ref_name }}" path: archive
Dereferencing a constant address pointer when -Warray-bounds is enabled in gcc generates an erroneous warning:
In function 'ppc64_patch_handlers', inlined from 'cpu_970_init' at /root/arch/ppc/qemu/init.c:433:5: /root/arch/ppc/qemu/init.c:400:10: error: array subscript 0 is outside array bounds of 'uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds] 400 | *dsi = 0x48002002; | ~~~~~^~~~~~~~~~~~ /root/arch/ppc/qemu/init.c:403:10: error: array subscript 0 is outside array bounds of 'uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds] 403 | *isi = 0x48002202; | ~~~~~^~~~~~~~~~~~ cc1: all warnings being treated as errors make[1]: *** [rules.mak:323: target/arch/ppc/qemu/init.o] Error 1 make[1]: Leaving directory '/root/obj-ppc'
This is a known bug in gcc 12 as described at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105523. Work around the bug for now by disabling the warning for ppc64_patch_handlers().
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- arch/ppc/qemu/init.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/arch/ppc/qemu/init.c b/arch/ppc/qemu/init.c index e40385a..253394c 100644 --- a/arch/ppc/qemu/init.c +++ b/arch/ppc/qemu/init.c @@ -390,6 +390,16 @@ cpu_g4_init(const struct cpudef *cpu) /* In order to get 64 bit aware handlers that rescue all our GPRs from getting truncated to 32 bits, we need to patch the existing handlers so they jump to our 64 bit aware ones. */ + +/* + * Disable -Warray-bounds for ppc64_patch_handlers() because accesses + * to low memory locations via constant pointers triggers a warning + * in gcc 12 (see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105523) + */ + +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Warray-bounds" + static void ppc64_patch_handlers(void) { @@ -406,6 +416,8 @@ ppc64_patch_handlers(void) asm ("icbi 0, %0" : : "r"(dsi)); asm ("icbi 0, %0" : : "r"(isi)); } + +#pragma GCC diagnostic pop #endif
static void
When compiled with gcc 12 packages/pc-parts.c generates the following error:
/root/packages/pc-parts.c:243:64: error: array subscript 1 is outside array bounds of 'struct pc_partition[1]' [-Werror=array-bounds] 243 | cur_table = ext_start + __le32_to_cpu(p[1].start_sect); | ~^~~ /root/include/libc/byteorder.h:12:13: note: in definition of macro '__bswap32' 12 | ((((x) & 0xff000000) >> 24) | (((x) & 0x00ff0000) >> 8) | \ | ^ /root/packages/pc-parts.c:243:49: note: in expansion of macro '__le32_to_cpu' 243 | cur_table = ext_start + __le32_to_cpu(p[1].start_sect); | ^~~~~~~~~~~~~ /root/packages/pc-parts.c:143:13: note: at offset 16 into object of size 16 allocated by 'malloc' 143 | p = malloc(sizeof(struct pc_partition)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors make[1]: *** [rules.mak:191: target/packages/pc-parts.o] Error 1 make[1]: Leaving directory '/root/obj-ppc'
Upon inspection this appears to be a genuine bug whereby the attempt to access the second extended partition entry incorrectly accesses the memory beyond the end of the aligned copy of the first extended partition entry.
Copy the second extended partition entry into aligned extended partition buffer and access the values from there to resolve the issue.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- packages/pc-parts.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/packages/pc-parts.c b/packages/pc-parts.c index dbbb2d4..ddc68e1 100644 --- a/packages/pc-parts.c +++ b/packages/pc-parts.c @@ -235,12 +235,15 @@ pcparts_open( pcparts_info_t *di ) }
/* Second entry is link to next partition */ - if (!is_pc_extended_part(p[1].type)) { + partition = (struct pc_partition *) (buf + 0x1ce); + memcpy(p, partition, sizeof(struct pc_partition)); + + if (!is_pc_extended_part(p->type)) { DPRINTF("no link\n"); break; }
- cur_table = ext_start + __le32_to_cpu(p[1].start_sect); + cur_table = ext_start + __le32_to_cpu(p->start_sect); cur_part++; }
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)) { 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);
This is required to ensure that the output binaries are generated in a form that will execute correctly as firmware at a fixed location.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- config/scripts/switch-arch | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/config/scripts/switch-arch b/config/scripts/switch-arch index f4da5c6..013116b 100755 --- a/config/scripts/switch-arch +++ b/config/scripts/switch-arch @@ -266,7 +266,7 @@ for ARCH in $arch_list; do select_prefix powerpc powerpc64 if [ "$unix" = "no" ]; then # 604 cpu includes support for PReP as well as Mac - CFLAGS="-m32 -mcpu=604 -msoft-float -fno-builtin-bcopy -fno-builtin-log2 -fno-stack-protector" + CFLAGS="-m32 -mcpu=604 -msoft-float -fno-builtin-bcopy -fno-builtin-log2 -fno-stack-protector -fno-pic" AS_FLAGS="-m32" else CFLAGS="-fno-builtin -fno-stack-protector" @@ -278,25 +278,25 @@ for ARCH in $arch_list; do select_prefix powerpc64
# 970 cpu is used in all 64-bit Macs but disable altivec - CFLAGS="-mcpu=970 -mno-altivec -Wa,-a64 -m64 -msoft-float -fno-builtin -fno-stack-protector" + CFLAGS="-mcpu=970 -mno-altivec -Wa,-a64 -m64 -msoft-float -fno-builtin -fno-stack-protector -fno-pic" AS_FLAGS="-Wa,-a64" ;;
sparc32) select_prefix sparc sparc64 - CFLAGS="-Wa,-xarch=v8 -Wa,-32 -m32 -mcpu=supersparc -fno-builtin -fno-stack-protector" + CFLAGS="-Wa,-xarch=v8 -Wa,-32 -m32 -mcpu=supersparc -fno-builtin -fno-stack-protector -fno-pic" AS_FLAGS="-Wa,-xarch=v8 -Wa,-32" ;;
sparc64) select_prefix sparc64 - CFLAGS="-Wa,-xarch=v9b -Wa,-64 -m64 -mcpu=ultrasparc -mcmodel=medany -fno-builtin -fno-stack-protector" + CFLAGS="-Wa,-xarch=v9b -Wa,-64 -m64 -mcpu=ultrasparc -mcmodel=medany -fno-builtin -fno-stack-protector -fno-pic" AS_FLAGS="-Wa,-xarch=v9b -Wa,-64" ;;
x86) select_prefix i486 x86_64 - CFLAGS="-fno-builtin -m32 -fno-stack-protector" + CFLAGS="-fno-builtin -m32 -fno-stack-protector -fno-pic" AS_FLAGS="-Wa,-32" ;; esac
Now it is possible to build fully functional OpenBIOS binaries for all architectures using the Debian cross-compiler packages, so switch the builder image from the kernel crosstools compilers to the packaged Debian cross-compilers.
Sigend-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- docker/Dockerfile.builder | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-)
diff --git a/docker/Dockerfile.builder b/docker/Dockerfile.builder index b4abf7f..fd8d46a 100644 --- a/docker/Dockerfile.builder +++ b/docker/Dockerfile.builder @@ -1,19 +1,5 @@ FROM ghcr.io/openbios/fcode-utils:master AS cross
RUN apt-get update && \ - apt-get install -y wget xz-utils tar && \ - wget https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/10.1.0/... && \ - tar Jxf x86_64-gcc-10.1.0-nolibc-sparc64-linux.tar.xz && \ - rm -f x86_64-gcc-10.1.0-nolibc-sparc64-linux.tar.xz && \ - wget https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/10.1.0/... && \ - tar Jxf x86_64-gcc-10.1.0-nolibc-powerpc-linux.tar.xz && \ - rm -f x86_64-gcc-10.1.0-nolibc-powerpc-linux.tar.xz - -FROM ghcr.io/openbios/fcode-utils:master AS builder - -COPY --from=cross /gcc-10.1.0-nolibc /gcc-10.1.0-nolibc - -RUN apt-get update && \ - apt-get install -y make xsltproc gcc gcc-multilib zip - -ENV PATH /gcc-10.1.0-nolibc/sparc64-linux/bin:/gcc-10.1.0-nolibc/powerpc-linux/bin:$PATH + apt-get install -y \ + make xsltproc zip libc6-dev-i386 gcc gcc-multilib-powerpc-linux-gnu gcc-multilib-sparc64-linux-gnu
The marvinpinto/action-automatic-releases action is currently unmaintained and displays several GitHub deprecation warnings during use. Switch over to using the forked crowbarmaster/GH-Automatic-Releases action which is an updated version of the original marvinpinto/action-automatic-releases action.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- .github/workflows/main.yml | 2 +- .github/workflows/release.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 626fc0a..a903697 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -94,7 +94,7 @@ jobs:
- name: Upload pre-release (upstream repository only) if: "${{ github.repository_owner == 'openbios' }}" - uses: "marvinpinto/action-automatic-releases@latest" + uses: "crowbarmaster/GH-Automatic-Releases@latest" with: repo_token: "${{ secrets.GITHUB_TOKEN }}" prerelease: true diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1fc587c..a8b8895 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -93,7 +93,7 @@ jobs: run: cd archive && zip -r ../openbios-multiarch-${{ github.ref_name }}.zip debug release && cd ..
- name: Upload release - uses: "marvinpinto/action-automatic-releases@latest" + uses: "crowbarmaster/GH-Automatic-Releases@latest" with: repo_token: "${{ secrets.GITHUB_TOKEN }}" prerelease: false
On Mon, 22 Apr 2024, 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.
Originally this came from libpayload of coreboot. To avoid diverging from that, aybe you can consider checking the changes there and updating this file from the current version of libpayload to also get the fixes and enhancements made there. This function seems to have been replaced in coreboot and not there any more.
Regards, BALATON Zoltan
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)) { 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);
On 22/04/2024 22:07, BALATON Zoltan wrote:
On Mon, 22 Apr 2024, 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.
Originally this came from libpayload of coreboot. To avoid diverging from that, aybe you can consider checking the changes there and updating this file from the current version of libpayload to also get the fixes and enhancements made there. This function seems to have been replaced in coreboot and not there any more.
I've had a quick look over at coreboot and it seems the latest version of this file is quite different from what is used here in OpenBIOS. Given that I've had requests to use the Debian compilers for upstream and reproducible builds, this would be better done as a follow-up exercise.
Can you remember which version of libpayload you used when importing into OpenBIOS and what the changes were that you made?
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)) { 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.
On 22/04/2024 08:54, Mark Cave-Ayland wrote:
This series is based upon Amadeusz Sławiński's recent series at https://mail.coreboot.org/hyperkitty/list/openbios@openbios.org/thread/OLS25... 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@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(-)
Pushed to master.
ATB,
Mark.
On Wed, 24 Apr 2024, Mark Cave-Ayland wrote:
On 22/04/2024 22:07, BALATON Zoltan wrote:
On Mon, 22 Apr 2024, 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.
Originally this came from libpayload of coreboot. To avoid diverging from that, aybe you can consider checking the changes there and updating this file from the current version of libpayload to also get the fixes and enhancements made there. This function seems to have been replaced in coreboot and not there any more.
I've had a quick look over at coreboot and it seems the latest version of this file is quite different from what is used here in OpenBIOS. Given that I've had requests to use the Debian compilers for upstream and reproducible builds, this would be better done as a follow-up exercise.
Can you remember which version of libpayload you used when importing into OpenBIOS and what the changes were that you made?
I've used the version that was current (probably in coreboot's git) at the time I've ported this driver. I still have a coreboot git clone with HEAD at commit d5403773901d from May 1 00:02:43 2014 so maybe it was around that time. I've deliberately kept changes minimal so it would be possible to later update the file so maybe you can get a patch with the needed changes by comparing to that coreboot commit. It looks like I had to do mostly endianness fixes, don't knoe if coreboot has fixed that already.
Regards, BALATON Zoltan
-
BALATON Zoltan -
Mark Cave-Ayland