On Tue, Mar 11, 2025 at 3:34 AM Paolo Bonzini <pbonz...@redhat.com> wrote:
>
> On Fri, Mar 7, 2025 at 1:45 AM Alistair Francis <alistai...@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 <pbonz...@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
> > > >>
> > > >>
> > > >
> > > >
> > >
> >
>
