Fix two misaligned reads from the FDT in the opal setup code to avoid
tripping UBSAN failures. Without this change, UBSAN-enabled builds on
PPC will fail on boot before the serial console is even initialized.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
 xen/arch/ppc/opal.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/xen/arch/ppc/opal.c b/xen/arch/ppc/opal.c
index 1183b7d5ef..3d0e4daf27 100644
--- a/xen/arch/ppc/opal.c
+++ b/xen/arch/ppc/opal.c
@@ -34,8 +34,9 @@ static void opal_putchar(char c)
 void __init boot_opal_init(const void *fdt)
 {
     int opal_node;
-    const __be64 *opal_base;
-    const __be64 *opal_entry;
+    const __be64 *opal_base_p;
+    const __be64 *opal_entry_p;
+    __be64 opal_base, opal_entry;
 
     if ( fdt_check_header(fdt) < 0 )
     {
@@ -54,17 +55,20 @@ void __init boot_opal_init(const void *fdt)
         die();
     }
 
-    opal_base = fdt_getprop(fdt, opal_node, "opal-base-address", NULL);
-    opal_entry = fdt_getprop(fdt, opal_node, "opal-entry-address", NULL);
-    if ( !opal_base || !opal_entry )
+    opal_base_p = fdt_getprop(fdt, opal_node, "opal-base-address", NULL);
+    opal_entry_p = fdt_getprop(fdt, opal_node, "opal-entry-address", NULL);
+    if ( !opal_base_p || !opal_entry_p )
     {
         early_printk("Failed to get opal-base-address/opal-entry-address "
                      "property from DT!\n");
         die();
     }
 
-    opal.base = be64_to_cpu(*opal_base);
-    opal.entry = be64_to_cpu(*opal_entry);
+    memcpy(&opal_base, opal_base_p, sizeof(opal_base));
+    memcpy(&opal_entry, opal_entry_p, sizeof(opal_entry));
+
+    opal.base = be64_to_cpu(opal_base);
+    opal.entry = be64_to_cpu(opal_entry);
 
     early_printk_init(opal_putchar);
 
-- 
2.30.2

On 21/02/2025 8:08 pm, Shawn Anastasio wrote:
> Fix two misaligned reads from the FDT in the opal setup code to avoid
> tripping UBSAN failures. Without this change, UBSAN-enabled builds on
> PPC will fail on boot before the serial console is even initialized.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> ---
>  xen/arch/ppc/opal.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/ppc/opal.c b/xen/arch/ppc/opal.c
> index 1183b7d5ef..3d0e4daf27 100644
> --- a/xen/arch/ppc/opal.c
> +++ b/xen/arch/ppc/opal.c
> @@ -34,8 +34,9 @@ static void opal_putchar(char c)
>  void __init boot_opal_init(const void *fdt)
>  {
>      int opal_node;
> -    const __be64 *opal_base;
> -    const __be64 *opal_entry;
> +    const __be64 *opal_base_p;
> +    const __be64 *opal_entry_p;
> +    __be64 opal_base, opal_entry;
>  
>      if ( fdt_check_header(fdt) < 0 )
>      {
> @@ -54,17 +55,20 @@ void __init boot_opal_init(const void *fdt)
>          die();
>      }
>  
> -    opal_base = fdt_getprop(fdt, opal_node, "opal-base-address", NULL);
> -    opal_entry = fdt_getprop(fdt, opal_node, "opal-entry-address", NULL);
> -    if ( !opal_base || !opal_entry )
> +    opal_base_p = fdt_getprop(fdt, opal_node, "opal-base-address", NULL);
> +    opal_entry_p = fdt_getprop(fdt, opal_node, "opal-entry-address", NULL);
> +    if ( !opal_base_p || !opal_entry_p )
>      {
>          early_printk("Failed to get opal-base-address/opal-entry-address "
>                       "property from DT!\n");
>          die();
>      }
>  
> -    opal.base = be64_to_cpu(*opal_base);
> -    opal.entry = be64_to_cpu(*opal_entry);
> +    memcpy(&opal_base, opal_base_p, sizeof(opal_base));
> +    memcpy(&opal_entry, opal_entry_p, sizeof(opal_entry));
> +
> +    opal.base = be64_to_cpu(opal_base);
> +    opal.entry = be64_to_cpu(opal_entry);

Thanks for getting to the bottom of this.

However, you can use get_unaligned_*() and friends which is probably a
bit nicer, and doesn't involve the extra local variables.

~Andrew

On 21/02/2025 8:10 pm, Andrew Cooper wrote:
> On 21/02/2025 8:08 pm, Shawn Anastasio wrote:
>> Fix two misaligned reads from the FDT in the opal setup code to avoid
>> tripping UBSAN failures. Without this change, UBSAN-enabled builds on
>> PPC will fail on boot before the serial console is even initialized.
>>
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
>> ---
>>  xen/arch/ppc/opal.c | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/ppc/opal.c b/xen/arch/ppc/opal.c
>> index 1183b7d5ef..3d0e4daf27 100644
>> --- a/xen/arch/ppc/opal.c
>> +++ b/xen/arch/ppc/opal.c
>> @@ -34,8 +34,9 @@ static void opal_putchar(char c)
>>  void __init boot_opal_init(const void *fdt)
>>  {
>>      int opal_node;
>> -    const __be64 *opal_base;
>> -    const __be64 *opal_entry;
>> +    const __be64 *opal_base_p;
>> +    const __be64 *opal_entry_p;
>> +    __be64 opal_base, opal_entry;
>>  
>>      if ( fdt_check_header(fdt) < 0 )
>>      {
>> @@ -54,17 +55,20 @@ void __init boot_opal_init(const void *fdt)
>>          die();
>>      }
>>  
>> -    opal_base = fdt_getprop(fdt, opal_node, "opal-base-address", NULL);
>> -    opal_entry = fdt_getprop(fdt, opal_node, "opal-entry-address", NULL);
>> -    if ( !opal_base || !opal_entry )
>> +    opal_base_p = fdt_getprop(fdt, opal_node, "opal-base-address", NULL);
>> +    opal_entry_p = fdt_getprop(fdt, opal_node, "opal-entry-address", NULL);
>> +    if ( !opal_base_p || !opal_entry_p )
>>      {
>>          early_printk("Failed to get opal-base-address/opal-entry-address "
>>                       "property from DT!\n");
>>          die();
>>      }
>>  
>> -    opal.base = be64_to_cpu(*opal_base);
>> -    opal.entry = be64_to_cpu(*opal_entry);
>> +    memcpy(&opal_base, opal_base_p, sizeof(opal_base));
>> +    memcpy(&opal_entry, opal_entry_p, sizeof(opal_entry));
>> +
>> +    opal.base = be64_to_cpu(opal_base);
>> +    opal.entry = be64_to_cpu(opal_entry);
> Thanks for getting to the bottom of this.
>
> However, you can use get_unaligned_*() and friends which is probably a
> bit nicer, and doesn't involve the extra local variables.

Sorry, the other thing to say is that if PPC is generally fine with
unaligned accesses, then you might want to follow what x86 does.

We use -fno-sanitize=alignment generally, because there's a whole pile
of things which are misaigned and spec'd that way for backwards
compatibility.

~Andrew

On 2/21/25 2:15 PM, Andrew Cooper wrote:
> Sorry, the other thing to say is that if PPC is generally fine with
> unaligned accesses, then you might want to follow what x86 does.
> 
> We use -fno-sanitize=alignment generally, because there's a whole pile
> of things which are misaigned and spec'd that way for backwards
> compatibility.

Oh perfect -- I did do an initial grep to see if this was done but
couldn't immediately find it. The Power ISA does guarantee that
unaligned word/doubleword reads are handled transparently by the
hardware so enabling this seems like a reasonable approach on PPC as
well. I'll submit a v2 that does this.

> ~Andrew

Thanks,
Shawn