Message ID | 20230802180058.281385-1-dbarboza@ventanamicro.com |
---|---|
State | New |
Headers | show |
Series | target/riscv/kvm.c: fix mvendorid size in vcpu_set_machine_ids() | expand |
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, ®); > 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
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, ®); >> 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
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. 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, ®); > > > 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
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, ®); >>>> 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
Drew, 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. > > 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. Is this a suggestion to do this right now in this patch? I didn't understand whether you're ok with the fix as is for 8.1 or if you want more things done right away. Thanks, Daniel > > 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, ®); >>>> 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
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; > 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. > + */ > + reg = cpu->cfg.mvendorid; > + ret = kvm_set_one_reg(cs, id, ®); > if (ret != 0) { > return ret; > } > -- > 2.41.0 > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
On Wed, Aug 09, 2023 at 07:16:00PM -0300, Daniel Henrique Barboza wrote: > Drew, > > On 8/3/23 09:05, Andrew Jones wrote: > > On Thu, Aug 03, 2023 at 08:36:57AM -0300, Daniel Henrique Barboza wrote: ... > > 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. > > Is this a suggestion to do this right now in this patch? I didn't understand > whether you're ok with the fix as is for 8.1 or if you want more things done > right away. > The fix looks good for 8.1. Using target_ulong looks right to me for all KVM ULONG-sized registers. When we try to build an API which provides the right sized buffers (which is future work), we'll want to use target_ulong there too. Thanks, drew
On Wed, Aug 2, 2023 at 2:02 PM Daniel Henrique Barboza <dbarboza@ventanamicro.com> 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> Acked-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > 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; > 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. > + */ > + reg = cpu->cfg.mvendorid; > + ret = kvm_set_one_reg(cs, id, ®); > if (ret != 0) { > return ret; > } > -- > 2.41.0 > >
On Wed, Aug 2, 2023 at 2:02 PM Daniel Henrique Barboza <dbarboza@ventanamicro.com> 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> Thanks! Applied to riscv-to-apply.next Alistair > --- > 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; > 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. > + */ > + reg = cpu->cfg.mvendorid; > + ret = kvm_set_one_reg(cs, id, ®); > if (ret != 0) { > return ret; > } > -- > 2.41.0 > >
On Wed, Aug 9, 2023 at 6:17 PM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > Drew, > > 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. > > > > 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. > > Is this a suggestion to do this right now in this patch? I didn't understand > whether you're ok with the fix as is for 8.1 or if you want more things done > right away. Do you want this in for 8.1? It might be a bit late. It helps a lot if you put in the title [PATCH 8.1] Alistair > > > Thanks, > > Daniel > > > > > 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, ®); > >>>> 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 >
On 8/10/23 14:01, Alistair Francis wrote: > On Wed, Aug 9, 2023 at 6:17 PM Daniel Henrique Barboza > <dbarboza@ventanamicro.com> wrote: >> >> Drew, >> >> 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. >>> >>> 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. >> >> Is this a suggestion to do this right now in this patch? I didn't understand >> whether you're ok with the fix as is for 8.1 or if you want more things done >> right away. > > Do you want this in for 8.1? It might be a bit late. It helps a lot if > you put in the title [PATCH 8.1] Yeah, sorry for not putting that in the subject. At this point I don't think it's worth spinning a rc4 because of this patch. If you have something else critical for 8.1 then it would be nice to slide this in in the mix, but it's not critical to be pushed standalone for rc4. Thanks, Daniel > > Alistair > >> >> >> Thanks, >> >> Daniel >> >>> >>> 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, ®); >>>>>> 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 >>
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; 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. + */ + reg = cpu->cfg.mvendorid; + ret = kvm_set_one_reg(cs, id, ®); if (ret != 0) { return ret; }
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(-)