Re: [XEN v3] xen/arm64: io: Decode ldr/str post-indexing instructions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.01.2022 19:41, Stefano Stabellini wrote:
> On Mon, 24 Jan 2022, Ayan Kumar Halder wrote:
>> As for the patch, I will mention this issue (as a comment in the code) where
>> we are loading the instruction from PC. Stefano/Julien/Bertrand/Volodymyr:-
>> Does it look fine with you ?
> 
> As this issue could happen on any architecture (the guest could change
> the instruction from another vcpu while the other is trapping in Xen)
> and given that we do quite a bit of emulation on x86 I asked Jan on IRC
> how do we handle this kind of things on x86 today. He had a good answer:
> "By not making any assumptions on what we're going to find."
> 
> In other words, don't assume you are going to find a store or a load
> instruction at the memory location pointed by the PC. You could find
> total garbage (because it was changed in between). Make sure to check
> everything is as expected before taking any actions.
> 
> And I think you are already doing that in decode_loadstore_postindexing.
> 
> These are the fields:
> 
> + * 31 30 29  27 26 25  23   21 20              11   9         4       0
> + * ___________________________________________________________________
> + * |size|1 1 1 |V |0 0 |opc |0 |      imm9     |0 1 |  Rn     |  Rt   |
> + * |____|______|__|____|____|__|_______________|____|_________|_______|
> + */
> +union ldr_str_instr_class {
> +    uint32_t value;
> +    struct ldr_str {
> +        unsigned int rt:5;     /* Rt register */
> +        unsigned int rn:5;     /* Rn register */
> +        unsigned int fixed1:2; /* value == 01b */
> +        signed int imm9:9;            /* imm9 */
> +        unsigned int fixed2:1; /* value == 0b */
> +        unsigned int opc:2;    /* opc */
> +        unsigned int fixed3:2; /* value == 00b */
> +        unsigned int v:1;      /* vector */
> +        unsigned int fixed4:3; /* value == 111b */
> +        unsigned int size:2;   /* size */
> +    } code;
> +};
> 
> 
> This patch already checks for:
> - the fixed values
> - v
> - opc
> - some special rt and rn values
> 
> Considering that:
> - size is fine either way
> - as rt and rn are 5 bits wide, all values are acceptable (x0->x31)
> 
> It doesn't look like we are missing anything, unless imm9 is restricted
> to some ranges only.

Beyond decoding there's at least one further assumption one may
mistakenly make: The address may not be suitably aligned and it may
not reference MMIO (or, should that matter, not the specific region
of MMIO that other trap-provided info my hint at).

Jan