Re: [PATCH 2/2] xen/x86: address violations of Rule 11.3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.06.2025 02:20, victorm.lira@xxxxxxx wrote:
> From: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> 
> Use {get,put}_unaligned_t to ensure that reads and writes are
> safe to perform even on potentially misaligned pointers.

Also applicable to the Arm patch: Please can such patches mention the
main subject of the rule, not just the number?

Overall I'm unconvinced we really want or need this on x86; I'm curious
what Andrew and Roger think. Further, even beyond the respective remark
below, I'd be pretty surprised if these were all of the places that
would need fiddling with. Mind me asking how the places to touch were
identified? (This may actually be a good thing to mention in the
description.)

> @@ -388,7 +392,7 @@ static int init_or_livepatch apply_alt_calls(
>              return -EINVAL;
>          }
> 
> -        disp = *(int32_t *)(orig + 2);
> +        disp = get_unaligned_t(int32_t, orig + 2);
>          dest = *(const void **)(orig + 6 + disp);

Why is this latter line not also adjusted? The field is expected to be
aligned, yes, but for the code here there's no guarantee. Imo if this
was left alone along with applying the suggested change, a code comment
would need adding.

> --- a/xen/arch/x86/include/asm/hvm/vlapic.h
> +++ b/xen/arch/x86/include/asm/hvm/vlapic.h
> @@ -10,6 +10,7 @@
>  #define __ASM_X86_HVM_VLAPIC_H__
> 
>  #include <xen/tasklet.h>
> +#include <xen/unaligned.h>
>  #include <asm/hvm/vpt.h>
> 
>  #define vcpu_vlapic(x)   (&(x)->arch.hvm.vlapic)
> @@ -85,13 +86,13 @@ struct vlapic {
>  static inline uint32_t vlapic_get_reg(const struct vlapic *vlapic,
>                                        uint32_t reg)
>  {
> -    return *((uint32_t *)(&vlapic->regs->data[reg]));
> +    return get_unaligned_t(uint32_t, &vlapic->regs->data[reg]);

This, aiui (or should I say "I hope"), also addresses another violation
(casting away of const). Such will want mentioning in the description,
imo.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1249,7 +1249,7 @@ void asmlinkage __init noreturn __start_xen(void)
>                 (caps & 2) ? " V2" : "",
>                 !(caps & 3) ? " none" : "");
>          printk("EDID transfer time: %d seconds\n", caps >> 8);
> -        if ( *(u32 *)bootsym(boot_edid_info) == 0x13131313 )
> +        if ( get_unaligned_t(u32, bootsym(boot_edid_info)) == 0x13131313 )

When touching such, please can you also convert to uint<N>_t?

Jan