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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 24 Jun 2025, Jan Beulich wrote:
> 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?

+1


> Overall I'm unconvinced we really want or need this on x86; I'm curious
> what Andrew and Roger think.

To be honest, I had a similar reaction to you, which is why I suggested
on Matrix to:

- deviate the rule in its entirety on x86
- deviate the rule for all mappings except for devmem mappings on ARM

Leaving aside ARM for a second (this is exactly the kind of very
arch-specific behavior that is OK to device differently per
architecture), would you be OK with deviating the rule in its entirety on
x86?

Or do you prefer to continue with this patch?


> 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.)

The patch is simply based on the violations reported by the ECLAIR scan.


> > @@ -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
>