Re: [PATCH 1/2] xen/ppc: Fix opal.c's misaligned DT reads to avoid tripping UBSAN

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  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