[Qemu-devel] [PATCH v2] acpi-build: don't access unaligned addresses

[Qemu-devel] [PATCH v2] acpi-build: don't access unaligned addresses

[Michael S. Tsirkin]

casting an unaligned address to e.g.
uint32_t can trigger undefined behaviour in C.
Replace cast + assignment with memcpy.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Changes from v1:
    no need to export bswap_le anymore

 hw/i386/acpi-build.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b667d31..7ecfd70 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -466,9 +466,15 @@ static void acpi_align_size(GArray *blob, unsigned align)
     g_array_set_size(blob, ROUND_UP(acpi_data_len(blob), align));
 }
 
-/* Get pointer within table in a safe manner */
-#define ACPI_BUILD_PTR(table, size, off, type) \
-    ((type *)(acpi_data_get_ptr(table, size, off, sizeof(type))))
+/* Set a value within table in a safe manner */
+#define ACPI_BUILD_SET_LE(table, size, off, bits, val) \
+    do { \
+        uint64_t ACPI_BUILD_SET_LE_val = cpu_to_le64(val); \
+        memcpy(acpi_data_get_ptr(table, size, off, \
+                                 (bits) / BITS_PER_BYTE), \
+               &ACPI_BUILD_SET_LE_val, \
+               (bits) / BITS_PER_BYTE); \
+    } while (0)
 
 static inline void *acpi_data_get_ptr(uint8_t *table_data, unsigned table_size,
                                       unsigned off, unsigned size)
@@ -974,22 +980,17 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
 
 static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size)
 {
-    *ACPI_BUILD_PTR(start, size, acpi_pci32_start[0], uint32_t) =
-        cpu_to_le32(pci->w32.begin);
+    ACPI_BUILD_SET_LE(start, size, acpi_pci32_start[0], 32, pci->w32.begin);
 
-    *ACPI_BUILD_PTR(start, size, acpi_pci32_end[0], uint32_t) =
-        cpu_to_le32(pci->w32.end - 1);
+    ACPI_BUILD_SET_LE(start, size, acpi_pci32_end[0], 32, pci->w32.end - 1);
 
     if (pci->w64.end || pci->w64.begin) {
-        *ACPI_BUILD_PTR(start, size, acpi_pci64_valid[0], uint8_t) = 1;
-        *ACPI_BUILD_PTR(start, size, acpi_pci64_start[0], uint64_t) =
-            cpu_to_le64(pci->w64.begin);
-        *ACPI_BUILD_PTR(start, size, acpi_pci64_end[0], uint64_t) =
-            cpu_to_le64(pci->w64.end - 1);
-        *ACPI_BUILD_PTR(start, size, acpi_pci64_length[0], uint64_t) =
-            cpu_to_le64(pci->w64.end - pci->w64.begin);
+        ACPI_BUILD_SET_LE(start, size, acpi_pci64_valid[0], 8, 1);
+        ACPI_BUILD_SET_LE(start, size, acpi_pci64_start[0], 64, pci->w64.begin);
+        ACPI_BUILD_SET_LE(start, size, acpi_pci64_end[0], 64, pci->w64.end - 1);
+        ACPI_BUILD_SET_LE(start, size, acpi_pci64_length[0], 64, pci->w64.end - pci->w64.begin);
     } else {
-        *ACPI_BUILD_PTR(start, size, acpi_pci64_valid[0], uint8_t) = 0;
+        ACPI_BUILD_SET_LE(start, size, acpi_pci64_valid[0], 8, 0);
     }
 }
 
-- 
MST

Re: [Qemu-devel] [PATCH v2] acpi-build: don't access unaligned addresses

[Peter Maydell]

On 10 March 2014 19:56, Michael S. Tsirkin <mst@redhat.com> wrote:
> casting an unaligned address to e.g.
> uint32_t can trigger undefined behaviour in C.
> Replace cast + assignment with memcpy.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

This does fix the clang warnings.

> -/* Get pointer within table in a safe manner */
> -#define ACPI_BUILD_PTR(table, size, off, type) \
> -    ((type *)(acpi_data_get_ptr(table, size, off, sizeof(type))))
> +/* Set a value within table in a safe manner */
> +#define ACPI_BUILD_SET_LE(table, size, off, bits, val) \
> +    do { \
> +        uint64_t ACPI_BUILD_SET_LE_val = cpu_to_le64(val); \
> +        memcpy(acpi_data_get_ptr(table, size, off, \
> +                                 (bits) / BITS_PER_BYTE), \
> +               &ACPI_BUILD_SET_LE_val, \
> +               (bits) / BITS_PER_BYTE); \
> +    } while (0)

Personally I would have done:

#define acpi_stb(table, size, off, val) \
    stb_le_p(acpi_data_get_ptr(table, size, off, 1), val)
#define acpi_stw(table, size, off, val) \
    stw_le_p(acpi_data_get_ptr(table, size, off, 2), val)
#define acpi_stl(table, size, off, val) \
    stl_le_p(acpi_data_get_ptr(table, size, off, 4), val)
#define acpi_stq(table, size, off, val) \
    stq_le_p(acpi_data_get_ptr(table, size, off, 8), val)

which keeps the grubby details of memcpy and byteswapping
in bswap.h. However since it's purely inside this file and
not specifying an API to the rest of QEMU I don't object
if you prefer the approach you've taken.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] acpi-build: don't access unaligned addresses

[Michael S. Tsirkin]

On Tue, Mar 11, 2014 at 12:29:05PM +0000, Peter Maydell wrote:
> On 10 March 2014 19:56, Michael S. Tsirkin <mst@redhat.com> wrote:
> > casting an unaligned address to e.g.
> > uint32_t can trigger undefined behaviour in C.
> > Replace cast + assignment with memcpy.
> >
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> This does fix the clang warnings.
> 
> > -/* Get pointer within table in a safe manner */
> > -#define ACPI_BUILD_PTR(table, size, off, type) \
> > -    ((type *)(acpi_data_get_ptr(table, size, off, sizeof(type))))
> > +/* Set a value within table in a safe manner */
> > +#define ACPI_BUILD_SET_LE(table, size, off, bits, val) \
> > +    do { \
> > +        uint64_t ACPI_BUILD_SET_LE_val = cpu_to_le64(val); \
> > +        memcpy(acpi_data_get_ptr(table, size, off, \
> > +                                 (bits) / BITS_PER_BYTE), \
> > +               &ACPI_BUILD_SET_LE_val, \
> > +               (bits) / BITS_PER_BYTE); \
> > +    } while (0)
> 
> Personally I would have done:
> 
> #define acpi_stb(table, size, off, val) \
>     stb_le_p(acpi_data_get_ptr(table, size, off, 1), val)
> #define acpi_stw(table, size, off, val) \
>     stw_le_p(acpi_data_get_ptr(table, size, off, 2), val)
> #define acpi_stl(table, size, off, val) \
>     stl_le_p(acpi_data_get_ptr(table, size, off, 4), val)
> #define acpi_stq(table, size, off, val) \
>     stq_le_p(acpi_data_get_ptr(table, size, off, 8), val)
> 
> which keeps the grubby details of memcpy and byteswapping
> in bswap.h. However since it's purely inside this file and
> not specifying an API to the rest of QEMU I don't object
> if you prefer the approach you've taken.
> 
> thanks
> -- PMM

I'll think about it some more - this can be
a patch on top. Let's fix the bug for now.

Re: [Qemu-devel] [PATCH v2] acpi-build: don't access unaligned addresses

[Peter Maydell]

On 11 March 2014 12:29, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 10 March 2014 19:56, Michael S. Tsirkin <mst@redhat.com> wrote:
>> casting an unaligned address to e.g.
>> uint32_t can trigger undefined behaviour in C.
>> Replace cast + assignment with memcpy.
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> This does fix the clang warnings.

I missed that there's still one more warning generated by a
different bit of this file:

/home/petmay01/linaro/qemu-for-merges/hw/i386/acpi-build.c:1022:5:
runtime error: store to misaligned address 0x2b5aa47dfb19 for type
'uint16_t' (aka 'unsigned short'), which requires 2 byte alignment
0x2b5aa47dfb19: note: pointer points here
 45 53 54  0b ff ff 5b 80 50 45 4f  52 01 50 45 53 54 01 5b  81 0b 50
45 4f 52 01 50  45 50 54 08 14
              ^

    *(uint16_t *)(ssdt_ptr + *ssdt_isa_pest) =
        cpu_to_le16(misc->pvpanic_port);

in build_ssdt(). You could change that to:
    stw_le_p(ssdt_ptr + *ssdt_isa_pest, misc->pvpanic_port);
(untested).

thanks
-- PMM