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

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

On 8/3/23 06:29, Andrew Jones wrote:
> 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.

I tried it out and it doesn't seem to work. Here's the result:

/ # cat /proc/cpuinfo
processor	: 0
hart		: 0
isa		: rv64imafdc_zicbom_zicboz_zihintpause_zbb_sstc
mmu		: sv57
mvendorid	: 0xaaaaaa000000cd
marchid		: 0xab
mimpid		: 0xef


The issue here is that the kernel considers 'mvendorid' as an unsigned long (or
what QEMU calls target_ulong). kvm_set_one_reg() will write an unsigned long
regardless of the uint32_t typing of 'reg', meaning that it'll end up writing
32 bits of uninitialized stuff from the stack.

target_ulong seems that the right choice here. We could perhaps work with
uint64_t (other parts of the code does that) but target_ulong is nicer with
32-bit setups.


Thanks,

Daniel

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