target/riscv/kvm.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
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.0On 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 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 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 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
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 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
>>
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 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
© 2016 - 2024 Red Hat, Inc.