Re: [PATCH] target/riscv/kvm.c: fix mvendorid size in vcpu_set_machine_ids()

[Andrew Jones]

On Wed, Aug 02, 2023 at 03:00:58PM -0300, Daniel Henrique Barboza wrote:
> cpu->cfg.mvendorid is a 32 bit field and kvm_set_one_reg() always write
> a target_ulong val, i.e. a 64 bit field in a 64 bit host.
> 
> Given that we're passing a pointer to the mvendorid field, the reg is
> reading 64 bits starting from mvendorid and going 32 bits in the next
> field, marchid. Here's an example:
> 
> $ ./qemu-system-riscv64 -machine virt,accel=kvm -m 2G -smp 1 \
>    -cpu rv64,marchid=0xab,mvendorid=0xcd,mimpid=0xef(...)
> 
> (inside the guest)
>  # cat /proc/cpuinfo
> processor     : 0
> hart          : 0
> isa           : rv64imafdc_zicbom_zicboz_zihintpause_zbb_sstc
> mmu           : sv57
> mvendorid     : 0xab000000cd
> marchid               : 0xab
> mimpid                : 0xef
> 
> 'mvendorid' was written as a combination of 0xab (the value from the
> adjacent field, marchid) and its intended value 0xcd.
> 
> Fix it by assigning cpu->cfg.mvendorid to a target_ulong var 'reg' and
> use it as input for kvm_set_one_reg(). Here's the result with this patch
> applied and using the same QEMU command line:
> 
>  # cat /proc/cpuinfo
> processor     : 0
> hart          : 0
> isa           : rv64imafdc_zicbom_zicboz_zihintpause_zbb_sstc
> mmu           : sv57
> mvendorid     : 0xcd
> marchid               : 0xab
> mimpid                : 0xef
> 
> This bug affects only the generic (rv64) CPUs when running with KVM in a
> 64 bit env since the 'host' CPU does not allow the machine IDs to be
> changed via command line.
> 
> Fixes: 1fb5a622f7 ("target/riscv: handle mvendorid/marchid/mimpid for KVM 
> CPUs")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/kvm.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 9d8a8982f9..b1fd2233c0 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -852,12 +852,19 @@ void kvm_arch_init_irq_routing(KVMState *s)
>  static int kvm_vcpu_set_machine_ids(RISCVCPU *cpu, CPUState *cs)
>  {
>      CPURISCVState *env = &cpu->env;
> +    target_ulong reg;

We can use the type of cfg since KVM just gets an address and uses the
KVM register type to determine the size. So here,

 uint32_t reg = cpu->cfg.mvendorid;

and then...

>      uint64_t id;
>      int ret;
>  
>      id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
>                            KVM_REG_RISCV_CONFIG_REG(mvendorid));
> -    ret = kvm_set_one_reg(cs, id, &cpu->cfg.mvendorid);
> +    /*
> +     * cfg.mvendorid is an uint32 but a target_ulong will
> +     * be written. Assign it to a target_ulong var to avoid
> +     * writing pieces of other cpu->cfg fields in the reg.
> +     */

...we don't need this comment since we're not doing anything special.

> +    reg = cpu->cfg.mvendorid;
> +    ret = kvm_set_one_reg(cs, id, &reg);
>      if (ret != 0) {
>          return ret;
>      }
> -- 
> 2.41.0
>

We should audit and fix all uses of &cpu->cfg.* with KVM ioctls. We can
also consider introducing wrappers like

#define kvm_set_one_reg_safe(cs, id, addr)      \
({                                              \
        typeof(*(addr)) _addr = *(addr);        \
        kvm_set_one_reg(cs, id, &_addr)         \
})

Thanks,
drew