diff mbox series

[2/7] target/riscv: env->misa_mxl is a constant

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

Commit Message

Paolo Bonzini Feb. 18, 2025, 4:57 p.m. UTC
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(+)

Comments

Alistair Francis March 6, 2025, 1:16 a.m. UTC | #1
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
>
>
Paolo Bonzini March 6, 2025, 1 p.m. UTC | #2
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
>>
>>
> 
>
Alistair Francis March 7, 2025, 12:44 a.m. UTC | #3
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
> >>
> >>
> >
> >
>
Paolo Bonzini March 10, 2025, 5:34 p.m. UTC | #4
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
> > >>
> > >>
> > >
> > >
> >
>
Alistair Francis March 10, 2025, 10:18 p.m. UTC | #5
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
> > > >>
> > > >>
> > > >
> > > >
> > >
> >
>
Paolo Bonzini March 11, 2025, 6:17 a.m. UTC | #6
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 mbox series

Patch

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),