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 <[email protected]> --- 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, ®); if (ret != 0) { return ret; } -- 2.41.0We 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
