Message ID | 20250218165757.554178-3-pbonzini@redhat.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | target/riscv: store max SATP mode as a single integer in RISCVCPUConfig | expand |
On Wed, Feb 19, 2025 at 3:01 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > There is nothing that overwrites env->misa_mxl, so it is a constant. Do The idea is that misa_mxl can change, although that's not supported now. > not let a corrupted migration stream change the value; changing misa_mxl Does this actually happen? If the migration data is corrupted won't we have all sorts of strange issues? Alistair > would have a snowball effect on, for example, the valid VM modes. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > target/riscv/machine.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/target/riscv/machine.c b/target/riscv/machine.c > index d8445244ab2..c3d8e7c4005 100644 > --- a/target/riscv/machine.c > +++ b/target/riscv/machine.c > @@ -375,6 +375,18 @@ static const VMStateDescription vmstate_ssp = { > } > }; > > +static bool riscv_validate_misa_mxl(void *opaque, int version_id) > +{ > + RISCVCPU *cpu = RISCV_CPU(opaque); > + CPURISCVState *env = &cpu->env; > + RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu); > + uint32_t misa_mxl_saved = env->misa_mxl; > + > + /* Preserve misa_mxl even if the migration stream corrupted it */ > + env->misa_mxl = mcc->misa_mxl_max; > + return misa_mxl_saved == mcc->misa_mxl_max; > +} > + > const VMStateDescription vmstate_riscv_cpu = { > .name = "cpu", > .version_id = 10, > @@ -394,6 +406,7 @@ const VMStateDescription vmstate_riscv_cpu = { > VMSTATE_UINTTL(env.priv_ver, RISCVCPU), > VMSTATE_UINTTL(env.vext_ver, RISCVCPU), > VMSTATE_UINT32(env.misa_mxl, RISCVCPU), > + VMSTATE_VALIDATE("MXL must match", riscv_validate_misa_mxl), > VMSTATE_UINT32(env.misa_ext, RISCVCPU), > VMSTATE_UNUSED(4), > VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU), > -- > 2.48.1 > >
On 3/6/25 02:16, Alistair Francis wrote: > On Wed, Feb 19, 2025 at 3:01 AM Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> There is nothing that overwrites env->misa_mxl, so it is a constant. Do > > The idea is that misa_mxl can change, although that's not supported now. At run-time, or only at configuration time (before realize)? >> not let a corrupted migration stream change the value; changing misa_mxl > > Does this actually happen? If the migration data is corrupted won't we > have all sorts of strange issues? Generally migration data (just like disk image formats) is treated as security-sensitive, overriding any other considerations. So you have to assume that the corruption is intentional, and sneaky enough to cause trouble. Paolo > Alistair > >> would have a snowball effect on, for example, the valid VM modes. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> target/riscv/machine.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/target/riscv/machine.c b/target/riscv/machine.c >> index d8445244ab2..c3d8e7c4005 100644 >> --- a/target/riscv/machine.c >> +++ b/target/riscv/machine.c >> @@ -375,6 +375,18 @@ static const VMStateDescription vmstate_ssp = { >> } >> }; >> >> +static bool riscv_validate_misa_mxl(void *opaque, int version_id) >> +{ >> + RISCVCPU *cpu = RISCV_CPU(opaque); >> + CPURISCVState *env = &cpu->env; >> + RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu); >> + uint32_t misa_mxl_saved = env->misa_mxl; >> + >> + /* Preserve misa_mxl even if the migration stream corrupted it */ >> + env->misa_mxl = mcc->misa_mxl_max; >> + return misa_mxl_saved == mcc->misa_mxl_max; >> +} >> + >> const VMStateDescription vmstate_riscv_cpu = { >> .name = "cpu", >> .version_id = 10, >> @@ -394,6 +406,7 @@ const VMStateDescription vmstate_riscv_cpu = { >> VMSTATE_UINTTL(env.priv_ver, RISCVCPU), >> VMSTATE_UINTTL(env.vext_ver, RISCVCPU), >> VMSTATE_UINT32(env.misa_mxl, RISCVCPU), >> + VMSTATE_VALIDATE("MXL must match", riscv_validate_misa_mxl), >> VMSTATE_UINT32(env.misa_ext, RISCVCPU), >> VMSTATE_UNUSED(4), >> VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU), >> -- >> 2.48.1 >> >> > >
On Thu, Mar 6, 2025 at 11:00 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 3/6/25 02:16, Alistair Francis wrote: > > On Wed, Feb 19, 2025 at 3:01 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > >> > >> There is nothing that overwrites env->misa_mxl, so it is a constant. Do > > > > The idea is that misa_mxl can change, although that's not supported now. > > At run-time, or only at configuration time (before realize)? Runtime, at least kind of The RISC-V spec 1.12 and earlier allows MXL to change at runtime by writing to misa.MXL. QEMU doesn't support this and AFAIK no hardware does either, but it was something that we might support in the future (hence the split). The latest RISC-V priv spec has changed misa.MXL to be read only though. So I guess although in theory it can be changed at runtime, we are probably never going to support that now that it's deprecated. Now that the latest priv spec has dropped the ability to write to misa.MXL we will probably work towards just consolidating misa_mxl_max and misa_mxl into a single value that is constant after realise. > > >> not let a corrupted migration stream change the value; changing misa_mxl > > > > Does this actually happen? If the migration data is corrupted won't we > > have all sorts of strange issues? > > Generally migration data (just like disk image formats) is treated as > security-sensitive, overriding any other considerations. So you have to > assume that the corruption is intentional, and sneaky enough to cause > trouble. I'm not convinced that this is the thing that we should be checking for. If someone can corrupt the migration data for an attack there are better things to change then the MXL Alistair > > Paolo > > > Alistair > > > >> would have a snowball effect on, for example, the valid VM modes. > >> > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > >> --- > >> target/riscv/machine.c | 13 +++++++++++++ > >> 1 file changed, 13 insertions(+) > >> > >> diff --git a/target/riscv/machine.c b/target/riscv/machine.c > >> index d8445244ab2..c3d8e7c4005 100644 > >> --- a/target/riscv/machine.c > >> +++ b/target/riscv/machine.c > >> @@ -375,6 +375,18 @@ static const VMStateDescription vmstate_ssp = { > >> } > >> }; > >> > >> +static bool riscv_validate_misa_mxl(void *opaque, int version_id) > >> +{ > >> + RISCVCPU *cpu = RISCV_CPU(opaque); > >> + CPURISCVState *env = &cpu->env; > >> + RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu); > >> + uint32_t misa_mxl_saved = env->misa_mxl; > >> + > >> + /* Preserve misa_mxl even if the migration stream corrupted it */ > >> + env->misa_mxl = mcc->misa_mxl_max; > >> + return misa_mxl_saved == mcc->misa_mxl_max; > >> +} > >> + > >> const VMStateDescription vmstate_riscv_cpu = { > >> .name = "cpu", > >> .version_id = 10, > >> @@ -394,6 +406,7 @@ const VMStateDescription vmstate_riscv_cpu = { > >> VMSTATE_UINTTL(env.priv_ver, RISCVCPU), > >> VMSTATE_UINTTL(env.vext_ver, RISCVCPU), > >> VMSTATE_UINT32(env.misa_mxl, RISCVCPU), > >> + VMSTATE_VALIDATE("MXL must match", riscv_validate_misa_mxl), > >> VMSTATE_UINT32(env.misa_ext, RISCVCPU), > >> VMSTATE_UNUSED(4), > >> VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU), > >> -- > >> 2.48.1 > >> > >> > > > > >
On Fri, Mar 7, 2025 at 1:45 AM Alistair Francis <alistair23@gmail.com> wrote: > I'm not convinced that this is the thing that we should be checking > for. If someone can corrupt the migration data for an attack there are > better things to change then the MXL In principle you could have code that uses 2 << MXL to compute the size of a memcpy, or something like that... never say never. :) Do you prefer turning all the priv_ver, vext_ver, misa_mxl, misa_ext_mask fields into VMSTATE_UNUSED? That would also be okay. I would also add a check for misa_ext against misa_ext_mask and riscv_cpu_validate_set_extensions(). Paolo > Alistair > > > > > Paolo > > > > > Alistair > > > > > >> would have a snowball effect on, for example, the valid VM modes. > > >> > > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > >> --- > > >> target/riscv/machine.c | 13 +++++++++++++ > > >> 1 file changed, 13 insertions(+) > > >> > > >> diff --git a/target/riscv/machine.c b/target/riscv/machine.c > > >> index d8445244ab2..c3d8e7c4005 100644 > > >> --- a/target/riscv/machine.c > > >> +++ b/target/riscv/machine.c > > >> @@ -375,6 +375,18 @@ static const VMStateDescription vmstate_ssp = { > > >> } > > >> }; > > >> > > >> +static bool riscv_validate_misa_mxl(void *opaque, int version_id) > > >> +{ > > >> + RISCVCPU *cpu = RISCV_CPU(opaque); > > >> + CPURISCVState *env = &cpu->env; > > >> + RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu); > > >> + uint32_t misa_mxl_saved = env->misa_mxl; > > >> + > > >> + /* Preserve misa_mxl even if the migration stream corrupted it */ > > >> + env->misa_mxl = mcc->misa_mxl_max; > > >> + return misa_mxl_saved == mcc->misa_mxl_max; > > >> +} > > >> + > > >> const VMStateDescription vmstate_riscv_cpu = { > > >> .name = "cpu", > > >> .version_id = 10, > > >> @@ -394,6 +406,7 @@ const VMStateDescription vmstate_riscv_cpu = { > > >> VMSTATE_UINTTL(env.priv_ver, RISCVCPU), > > >> VMSTATE_UINTTL(env.vext_ver, RISCVCPU), > > >> VMSTATE_UINT32(env.misa_mxl, RISCVCPU), > > >> + VMSTATE_VALIDATE("MXL must match", riscv_validate_misa_mxl), > > >> VMSTATE_UINT32(env.misa_ext, RISCVCPU), > > >> VMSTATE_UNUSED(4), > > >> VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU), > > >> -- > > >> 2.48.1 > > >> > > >> > > > > > > > > >
On Tue, Mar 11, 2025 at 3:34 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On Fri, Mar 7, 2025 at 1:45 AM Alistair Francis <alistair23@gmail.com> wrote: > > I'm not convinced that this is the thing that we should be checking > > for. If someone can corrupt the migration data for an attack there are > > better things to change then the MXL > > In principle you could have code that uses 2 << MXL to compute the > size of a memcpy, or something like that... never say never. :) But couldn't they also change the PC at that point and execute malicious code? Or MTVEC or anything else? It just seems like this check doesn't actually provide any security. In the future we will want to merge misa_mxl and misa_mxl_max and this patch just makes that a little bit harder, for no real gains that I can see. > > Do you prefer turning all the priv_ver, vext_ver, misa_mxl, > misa_ext_mask fields into VMSTATE_UNUSED? That would also be okay. I think we do want to migrate them, they contain important information. Alistair > > I would also add a check for misa_ext against misa_ext_mask and > riscv_cpu_validate_set_extensions(). > > Paolo > > > Alistair > > > > > > > > Paolo > > > > > > > Alistair > > > > > > > >> would have a snowball effect on, for example, the valid VM modes. > > > >> > > > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > > >> --- > > > >> target/riscv/machine.c | 13 +++++++++++++ > > > >> 1 file changed, 13 insertions(+) > > > >> > > > >> diff --git a/target/riscv/machine.c b/target/riscv/machine.c > > > >> index d8445244ab2..c3d8e7c4005 100644 > > > >> --- a/target/riscv/machine.c > > > >> +++ b/target/riscv/machine.c > > > >> @@ -375,6 +375,18 @@ static const VMStateDescription vmstate_ssp = { > > > >> } > > > >> }; > > > >> > > > >> +static bool riscv_validate_misa_mxl(void *opaque, int version_id) > > > >> +{ > > > >> + RISCVCPU *cpu = RISCV_CPU(opaque); > > > >> + CPURISCVState *env = &cpu->env; > > > >> + RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu); > > > >> + uint32_t misa_mxl_saved = env->misa_mxl; > > > >> + > > > >> + /* Preserve misa_mxl even if the migration stream corrupted it */ > > > >> + env->misa_mxl = mcc->misa_mxl_max; > > > >> + return misa_mxl_saved == mcc->misa_mxl_max; > > > >> +} > > > >> + > > > >> const VMStateDescription vmstate_riscv_cpu = { > > > >> .name = "cpu", > > > >> .version_id = 10, > > > >> @@ -394,6 +406,7 @@ const VMStateDescription vmstate_riscv_cpu = { > > > >> VMSTATE_UINTTL(env.priv_ver, RISCVCPU), > > > >> VMSTATE_UINTTL(env.vext_ver, RISCVCPU), > > > >> VMSTATE_UINT32(env.misa_mxl, RISCVCPU), > > > >> + VMSTATE_VALIDATE("MXL must match", riscv_validate_misa_mxl), > > > >> VMSTATE_UINT32(env.misa_ext, RISCVCPU), > > > >> VMSTATE_UNUSED(4), > > > >> VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU), > > > >> -- > > > >> 2.48.1 > > > >> > > > >> > > > > > > > > > > > > > >
Il lun 10 mar 2025, 23:18 Alistair Francis <alistair23@gmail.com> ha scritto: > On Tue, Mar 11, 2025 at 3:34 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > On Fri, Mar 7, 2025 at 1:45 AM Alistair Francis <alistair23@gmail.com> > wrote: > > > I'm not convinced that this is the thing that we should be checking > > > for. If someone can corrupt the migration data for an attack there are > > > better things to change then the MXL > > > > In principle you could have code that uses 2 << MXL to compute the > > size of a memcpy, or something like that... never say never. :) > > But couldn't they also change the PC at that point and execute > malicious code? Or MTVEC or anything else? > The point is exploiting the host, not the guest. It just seems like this check doesn't actually provide any security. In the future we will want to merge misa_mxl and misa_mxl_max and this > patch just makes that a little bit harder, for no real gains that I > can see. > > > Do you prefer turning all the priv_ver, vext_ver, misa_mxl, > > misa_ext_mask fields into VMSTATE_UNUSED? That would also be okay. > > I think we do want to migrate them, they contain important information. > They are constant though, aren't they? Properties aren't migrated, they are set on the command line of the destination. I will drop the patch, but I think there's some work to do on the migration of RISC-V CPU state. Paolo > Alistair > > > > > I would also add a check for misa_ext against misa_ext_mask and > > riscv_cpu_validate_set_extensions(). > > > > Paolo > > > > > Alistair > > > > > > > > > > > Paolo > > > > > > > > > Alistair > > > > > > > > > >> would have a snowball effect on, for example, the valid VM modes. > > > > >> > > > > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > > > >> --- > > > > >> target/riscv/machine.c | 13 +++++++++++++ > > > > >> 1 file changed, 13 insertions(+) > > > > >> > > > > >> diff --git a/target/riscv/machine.c b/target/riscv/machine.c > > > > >> index d8445244ab2..c3d8e7c4005 100644 > > > > >> --- a/target/riscv/machine.c > > > > >> +++ b/target/riscv/machine.c > > > > >> @@ -375,6 +375,18 @@ static const VMStateDescription vmstate_ssp > = { > > > > >> } > > > > >> }; > > > > >> > > > > >> +static bool riscv_validate_misa_mxl(void *opaque, int version_id) > > > > >> +{ > > > > >> + RISCVCPU *cpu = RISCV_CPU(opaque); > > > > >> + CPURISCVState *env = &cpu->env; > > > > >> + RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu); > > > > >> + uint32_t misa_mxl_saved = env->misa_mxl; > > > > >> + > > > > >> + /* Preserve misa_mxl even if the migration stream corrupted > it */ > > > > >> + env->misa_mxl = mcc->misa_mxl_max; > > > > >> + return misa_mxl_saved == mcc->misa_mxl_max; > > > > >> +} > > > > >> + > > > > >> const VMStateDescription vmstate_riscv_cpu = { > > > > >> .name = "cpu", > > > > >> .version_id = 10, > > > > >> @@ -394,6 +406,7 @@ const VMStateDescription vmstate_riscv_cpu = { > > > > >> VMSTATE_UINTTL(env.priv_ver, RISCVCPU), > > > > >> VMSTATE_UINTTL(env.vext_ver, RISCVCPU), > > > > >> VMSTATE_UINT32(env.misa_mxl, RISCVCPU), > > > > >> + VMSTATE_VALIDATE("MXL must match", > riscv_validate_misa_mxl), > > > > >> VMSTATE_UINT32(env.misa_ext, RISCVCPU), > > > > >> VMSTATE_UNUSED(4), > > > > >> VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU), > > > > >> -- > > > > >> 2.48.1 > > > > >> > > > > >> > > > > > > > > > > > > > > > > > > > > >
diff --git a/target/riscv/machine.c b/target/riscv/machine.c index d8445244ab2..c3d8e7c4005 100644 --- a/target/riscv/machine.c +++ b/target/riscv/machine.c @@ -375,6 +375,18 @@ static const VMStateDescription vmstate_ssp = { } }; +static bool riscv_validate_misa_mxl(void *opaque, int version_id) +{ + RISCVCPU *cpu = RISCV_CPU(opaque); + CPURISCVState *env = &cpu->env; + RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu); + uint32_t misa_mxl_saved = env->misa_mxl; + + /* Preserve misa_mxl even if the migration stream corrupted it */ + env->misa_mxl = mcc->misa_mxl_max; + return misa_mxl_saved == mcc->misa_mxl_max; +} + const VMStateDescription vmstate_riscv_cpu = { .name = "cpu", .version_id = 10, @@ -394,6 +406,7 @@ const VMStateDescription vmstate_riscv_cpu = { VMSTATE_UINTTL(env.priv_ver, RISCVCPU), VMSTATE_UINTTL(env.vext_ver, RISCVCPU), VMSTATE_UINT32(env.misa_mxl, RISCVCPU), + VMSTATE_VALIDATE("MXL must match", riscv_validate_misa_mxl), VMSTATE_UINT32(env.misa_ext, RISCVCPU), VMSTATE_UNUSED(4), VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU),
There is nothing that overwrites env->misa_mxl, so it is a constant. Do not let a corrupted migration stream change the value; changing misa_mxl would have a snowball effect on, for example, the valid VM modes. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- target/riscv/machine.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)