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

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

On 8/3/23 09:05, Andrew Jones wrote:
> On Thu, Aug 03, 2023 at 08:36:57AM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> 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.
> 
> Indeed, I managed to reverse the problem in my head. We need to to worry
> about KVM's notion of the type, not QEMU's. I feel like we still need some
> sort of helper, but one that takes the type of the KVM register into
> account. KVM_REG_RISCV_CONFIG registers are KVM ulongs, which may be
> different than QEMU's ulongs, if we ever supported 32-bit userspace on
> 64-bit kernels. Also, not all KVM register types are ulong, some are
> explicitly u32s and others u64s. I see kvm_riscv_reg_id() is used to try
> and get the right KVM reg size set, but it's broken for RISCV_FP_F_REG(),
> since those are all u32s, even when KVM has 64-bit ulong (I guess nobody
> is testing get/set-one-reg with F registers using that helper, otherwise
> we'd be getting EINVAL from KVM). And KVM_REG_RISCV_FP_D_REG(fcsr) is also
> broken and RISCV_TIMER_REG() looks broken with 32-bit KVM, since it should
> always be u64. I guess all that stuff needs an audit.

KVM_REG_RISCV_FP_D_REG(fcsr) does not work ATM because we're missing the ptrace.h
linux header (same problem as RVV I mentioned in private).

But otherwise, yeah, we need an audit for the other cases to see if we're messing
stuff up by accident.


Daniel


> 
> So, I think we need a helper that has a switch on the KVM register type
> and provides the right sized buffer for each case.
> 
> Thanks,
> drew
> 
> 
>>
>> 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