device-tree/kernel: Use memcpy in output_length()

[PATCH] device-tree/kernel: Use memcpy in output_length()

Posted by Michal Orzel 3 months, 1 week ago

Trying to boot a compressed kernel with UBSAN enabled, results in the
following warning:
(XEN) UBSAN: Undefined behaviour in common/device-tree/kernel.c:21:12
(XEN) load of misaligned address 00000a0040f89867 for type 'uint32_t'
(XEN) which requires 4 byte alignment
...
(XEN)    [<00000a0000529964>] kernel_decompress+0x2bc/0x5bc
(XEN)    [<00000a000052a354>] kernel_probe+0x6f0/0x734
(XEN)    [<00000a0000528714>] dom0less-build.c#construct_domU+0x188/0x9d8

If &image[image_len - 4] is not aligned to 4B boundary it causes
unaligned access which is undefined behavior on Arm. Use memcpy instead
to be safe.

Fixes: c1be0b102e0e ("xen/arm: support gzip compressed kernels")
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/common/device-tree/kernel.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/common/device-tree/kernel.c b/xen/common/device-tree/kernel.c
index ef393182b691..28096121a52d 100644
--- a/xen/common/device-tree/kernel.c
+++ b/xen/common/device-tree/kernel.c
@@ -18,7 +18,11 @@
 
 static uint32_t __init output_length(char *image, unsigned long image_len)
 {
-    return *(uint32_t *)&image[image_len - 4];
+    uint32_t val;
+
+    memcpy(&val, &image[image_len - 4], sizeof(val));
+
+    return val;
 }
 
 int __init kernel_decompress(struct boot_module *mod, uint32_t offset)
-- 
2.43.0

Re: [PATCH] device-tree/kernel: Use memcpy in output_length()

Posted by Jason Andryuk 3 months, 1 week ago

On 2025-07-22 03:46, Michal Orzel wrote:
> Trying to boot a compressed kernel with UBSAN enabled, results in the
> following warning:
> (XEN) UBSAN: Undefined behaviour in common/device-tree/kernel.c:21:12
> (XEN) load of misaligned address 00000a0040f89867 for type 'uint32_t'
> (XEN) which requires 4 byte alignment
> ...
> (XEN)    [<00000a0000529964>] kernel_decompress+0x2bc/0x5bc
> (XEN)    [<00000a000052a354>] kernel_probe+0x6f0/0x734
> (XEN)    [<00000a0000528714>] dom0less-build.c#construct_domU+0x188/0x9d8
> 
> If &image[image_len - 4] is not aligned to 4B boundary it causes
> unaligned access which is undefined behavior on Arm. Use memcpy instead
> to be safe.
> 
> Fixes: c1be0b102e0e ("xen/arm: support gzip compressed kernels")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
>   xen/common/device-tree/kernel.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/device-tree/kernel.c b/xen/common/device-tree/kernel.c
> index ef393182b691..28096121a52d 100644
> --- a/xen/common/device-tree/kernel.c
> +++ b/xen/common/device-tree/kernel.c
> @@ -18,7 +18,11 @@
>   
>   static uint32_t __init output_length(char *image, unsigned long image_len)
>   {
> -    return *(uint32_t *)&image[image_len - 4];

Maybe just:
     return get_unaligned_le32(&image[image_len - 4]);

You'll also need:
#include <xen/unaligned.h>

The gzip size is little endian, 
https://datatracker.ietf.org/doc/html/rfc1952:
       Within a computer, a number may occupy multiple bytes.  All
       multi-byte numbers in the format described here are stored with
       the least-significant byte first (at the lower memory address).

Regards,
Jason

> +    uint32_t val;
> +
> +    memcpy(&val, &image[image_len - 4], sizeof(val));
> +
> +    return val;
>   }
>   
>   int __init kernel_decompress(struct boot_module *mod, uint32_t offset)

Re: [PATCH] device-tree/kernel: Use memcpy in output_length()

Posted by Orzel, Michal 3 months, 1 week ago


On 22/07/2025 06:50, Jason Andryuk wrote:
> On 2025-07-22 03:46, Michal Orzel wrote:
>> Trying to boot a compressed kernel with UBSAN enabled, results in the
>> following warning:
>> (XEN) UBSAN: Undefined behaviour in common/device-tree/kernel.c:21:12
>> (XEN) load of misaligned address 00000a0040f89867 for type 'uint32_t'
>> (XEN) which requires 4 byte alignment
>> ...
>> (XEN)    [<00000a0000529964>] kernel_decompress+0x2bc/0x5bc
>> (XEN)    [<00000a000052a354>] kernel_probe+0x6f0/0x734
>> (XEN)    [<00000a0000528714>] dom0less-build.c#construct_domU+0x188/0x9d8
>>
>> If &image[image_len - 4] is not aligned to 4B boundary it causes
>> unaligned access which is undefined behavior on Arm. Use memcpy instead
>> to be safe.
>>
>> Fixes: c1be0b102e0e ("xen/arm: support gzip compressed kernels")
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>>   xen/common/device-tree/kernel.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/common/device-tree/kernel.c b/xen/common/device-tree/kernel.c
>> index ef393182b691..28096121a52d 100644
>> --- a/xen/common/device-tree/kernel.c
>> +++ b/xen/common/device-tree/kernel.c
>> @@ -18,7 +18,11 @@
>>   
>>   static uint32_t __init output_length(char *image, unsigned long image_len)
>>   {
>> -    return *(uint32_t *)&image[image_len - 4];
> 
> Maybe just:
>      return get_unaligned_le32(&image[image_len - 4]);
> 
> You'll also need:
> #include <xen/unaligned.h>
> 
> The gzip size is little endian, 
I thought about this solution but decided to use memcpy because one day we may
want to support other compression algorithms and forcing LE could cause issues.

I'm ok either way. Let other maintainers decide.

~Michal

Re: [PATCH] device-tree/kernel: Use memcpy in output_length()

Posted by Bertrand Marquis 3 months, 1 week ago

Hi,

> On 23 Jul 2025, at 08:33, Orzel, Michal <Michal.Orzel@amd.com> wrote:
> 
> 
> 
> On 22/07/2025 06:50, Jason Andryuk wrote:
>> On 2025-07-22 03:46, Michal Orzel wrote:
>>> Trying to boot a compressed kernel with UBSAN enabled, results in the
>>> following warning:
>>> (XEN) UBSAN: Undefined behaviour in common/device-tree/kernel.c:21:12
>>> (XEN) load of misaligned address 00000a0040f89867 for type 'uint32_t'
>>> (XEN) which requires 4 byte alignment
>>> ...
>>> (XEN)    [<00000a0000529964>] kernel_decompress+0x2bc/0x5bc
>>> (XEN)    [<00000a000052a354>] kernel_probe+0x6f0/0x734
>>> (XEN)    [<00000a0000528714>] dom0less-build.c#construct_domU+0x188/0x9d8
>>> 
>>> If &image[image_len - 4] is not aligned to 4B boundary it causes
>>> unaligned access which is undefined behavior on Arm. Use memcpy instead
>>> to be safe.
>>> 
>>> Fixes: c1be0b102e0e ("xen/arm: support gzip compressed kernels")
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> ---
>>>  xen/common/device-tree/kernel.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/xen/common/device-tree/kernel.c b/xen/common/device-tree/kernel.c
>>> index ef393182b691..28096121a52d 100644
>>> --- a/xen/common/device-tree/kernel.c
>>> +++ b/xen/common/device-tree/kernel.c
>>> @@ -18,7 +18,11 @@
>>> 
>>>  static uint32_t __init output_length(char *image, unsigned long image_len)
>>>  {
>>> -    return *(uint32_t *)&image[image_len - 4];
>> 
>> Maybe just:
>>     return get_unaligned_le32(&image[image_len - 4]);
>> 
>> You'll also need:
>> #include <xen/unaligned.h>
>> 
>> The gzip size is little endian,
> I thought about this solution but decided to use memcpy because one day we may
> want to support other compression algorithms and forcing LE could cause issues.

That makes sense so:

Acked-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> 
> I'm ok either way. Let other maintainers decide.
> 
> ~Michal
>