[PATCH] tests/qtest/libqos: Fix UBSan misalignment finding

Nabih Estefan posted 1 patch 6 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250428212041.1301050-1-nabihestefan@google.com
Maintainers: Akihiko Odaki <akihiko.odaki@daynix.com>, Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>, Fabiano Rosas <farosas@suse.de>, Laurent Vivier <lvivier@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
hw/pci/remote_pci.c      | 0
tests/qtest/libqos/igb.c | 8 ++++++--
2 files changed, 6 insertions(+), 2 deletions(-)
create mode 100644 hw/pci/remote_pci.c
[PATCH] tests/qtest/libqos: Fix UBSan misalignment finding
Posted by Nabih Estefan 6 months, 3 weeks ago
Running with `--enable-ubsan` leads to a qtest failure:
```
../tests/qtest/libqos/igb.c:106:5: runtime error: load of misaligned address 0x562040be8e33 for type 'uint32_t', which requires 4 byte alignment
```
Instead of straight casting the uint8_t array, we use memcpy to assure
alignment is correct against uint32_t and uint16_t.

Change-Id: Ibd2bc3d870ea37bcbaf2e459806a22ae17464049
Google-Bug-Id: 391659542
Signed-off-by: Nabih Estefan <nabihestefan@google.com>
---
 hw/pci/remote_pci.c      | 0
 tests/qtest/libqos/igb.c | 8 ++++++--
 2 files changed, 6 insertions(+), 2 deletions(-)
 create mode 100644 hw/pci/remote_pci.c

diff --git a/hw/pci/remote_pci.c b/hw/pci/remote_pci.c
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qtest/libqos/igb.c b/tests/qtest/libqos/igb.c
index f40c4ec4cd..f31b1a7261 100644
--- a/tests/qtest/libqos/igb.c
+++ b/tests/qtest/libqos/igb.c
@@ -103,11 +103,15 @@ static void igb_pci_start_hw(QOSGraphObject *obj)
     e1000e_macreg_write(&d->e1000e, E1000_RDLEN(0), E1000E_RING_LEN);
     e1000e_macreg_write(&d->e1000e, E1000_RDT(0), 0);
     e1000e_macreg_write(&d->e1000e, E1000_RDH(0), 0);
+    uint32_t safe32_address;
+    memcpy(&safe32_address, address, sizeof(uint32_t));
     e1000e_macreg_write(&d->e1000e, E1000_RA,
-                        le32_to_cpu(*(uint32_t *)address));
+                        le32_to_cpu(safe32_address));
+    uint16_t safe16_address;
+    memcpy(&safe16_address, (address + 4), sizeof(uint16_t));
     e1000e_macreg_write(&d->e1000e, E1000_RA + 4,
                         E1000_RAH_AV | E1000_RAH_POOL_1 |
-                        le16_to_cpu(*(uint16_t *)(address + 4)));
+                        le16_to_cpu(safe16_address));
 
     /* Set supported receive descriptor mode */
     e1000e_macreg_write(&d->e1000e,
-- 
2.49.0.901.g37484f566f-goog
Re: [PATCH] tests/qtest/libqos: Fix UBSan misalignment finding
Posted by Laurent Vivier 6 months, 2 weeks ago
On 28/04/2025 23:20, Nabih Estefan wrote:
> Running with `--enable-ubsan` leads to a qtest failure:
> ```
> ../tests/qtest/libqos/igb.c:106:5: runtime error: load of misaligned address 0x562040be8e33 for type 'uint32_t', which requires 4 byte alignment
> ```
> Instead of straight casting the uint8_t array, we use memcpy to assure
> alignment is correct against uint32_t and uint16_t.
> 
> Change-Id: Ibd2bc3d870ea37bcbaf2e459806a22ae17464049
> Google-Bug-Id: 391659542
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> ---
>   hw/pci/remote_pci.c      | 0
>   tests/qtest/libqos/igb.c | 8 ++++++--
>   2 files changed, 6 insertions(+), 2 deletions(-)
>   create mode 100644 hw/pci/remote_pci.c
> 
> diff --git a/hw/pci/remote_pci.c b/hw/pci/remote_pci.c
> new file mode 100644
> index 0000000000..e69de29bb2

It seems you are adding an empty file here.

> diff --git a/tests/qtest/libqos/igb.c b/tests/qtest/libqos/igb.c
> index f40c4ec4cd..f31b1a7261 100644
> --- a/tests/qtest/libqos/igb.c
> +++ b/tests/qtest/libqos/igb.c
> @@ -103,11 +103,15 @@ static void igb_pci_start_hw(QOSGraphObject *obj)
>       e1000e_macreg_write(&d->e1000e, E1000_RDLEN(0), E1000E_RING_LEN);
>       e1000e_macreg_write(&d->e1000e, E1000_RDT(0), 0);
>       e1000e_macreg_write(&d->e1000e, E1000_RDH(0), 0);
> +    uint32_t safe32_address;

We generally don't mix variable declaration and code.

> +    memcpy(&safe32_address, address, sizeof(uint32_t));
>       e1000e_macreg_write(&d->e1000e, E1000_RA,
> -                        le32_to_cpu(*(uint32_t *)address));
> +                        le32_to_cpu(safe32_address));

I think you could use instead ldl_le_p() instead.

> +    uint16_t safe16_address;
> +    memcpy(&safe16_address, (address + 4), sizeof(uint16_t));
>       e1000e_macreg_write(&d->e1000e, E1000_RA + 4,
>                           E1000_RAH_AV | E1000_RAH_POOL_1 |
> -                        le16_to_cpu(*(uint16_t *)(address + 4)));
> +                        le16_to_cpu(safe16_address));

You could use lduw_le_p().

>   
>       /* Set supported receive descriptor mode */
>       e1000e_macreg_write(&d->e1000e,

Thanks,
Laurent