Re: [PULL 23/45] accel/mshv: store partition proc features

[Magnus Kulke <magnuskulke@linux.microsoft.com>]

On Mon, Jun 29, 2026 at 03:27:40PM +0100, Peter Maydell wrote:
> On Fri, 26 Jun 2026 at 11:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > From: Magnus Kulke <magnuskulke@linux.microsoft.com>
> >
> > We retrieve and store processor features on the state, so we can query
> > them later when deciding which MSRs to migrate.
> >
> > Signed-off-by: Magnus Kulke <magnuskulke@linux.microsoft.com>
> > Link: https://lore.kernel.org/r/20260417105618.3621-19-magnuskulke@linux.microsoft.com
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Coverity points out what looks like an error in this commit
> (CID 1660876):
> 
> > ---
> >  include/hw/hyperv/hvhdk.h |  2 +-
> >  include/system/mshv_int.h |  3 +++
> >  accel/mshv/mshv-all.c     | 57 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 61 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/hyperv/hvhdk.h b/include/hw/hyperv/hvhdk.h
> > index 9ad16c47da9..9e6dcb22f61 100644
> > --- a/include/hw/hyperv/hvhdk.h
> > +++ b/include/hw/hyperv/hvhdk.h
> > @@ -353,7 +353,7 @@ union hv_partition_processor_features {
> >          uint64_t lass_support:1;
> >          uint64_t idle_hlt_intercept_support:1;
> >          uint64_t msr_list_support:1;
> > -    };
> > +    } QEMU_PACKED;
> >  };
> 
> hv_partition_processor_features is a union of a 2-element
> uint64_t array and a pile of bitfields...
> 
> >
> >  enum hv_translate_gva_result_code {
> > diff --git a/include/system/mshv_int.h b/include/system/mshv_int.h
> > index 5eef2e6e2cb..0cf68d2879e 100644
> > --- a/include/system/mshv_int.h
> > +++ b/include/system/mshv_int.h
> > @@ -14,6 +14,8 @@
> >  #ifndef QEMU_MSHV_INT_H
> >  #define QEMU_MSHV_INT_H
> >
> > +#include "hw/hyperv/hvhdk.h"
> > +
> >  #define MSHV_MSR_ENTRIES_COUNT 64
> >
> >  typedef struct hyperv_message hv_message;
> > @@ -53,6 +55,7 @@ struct MshvState {
> >      int nr_allocated_irq_routes;
> >      unsigned long *used_gsi_bitmap;
> >      unsigned int gsi_count;
> > +    union hv_partition_processor_features processor_features;
> 
> ...and processor_features is a field with one of those unions.
> 
> >  };
> >
> 
> > +static int get_proc_features(int vm_fd,
> > +                             union hv_partition_processor_features *features)
> 
> In this function we take a pointer to a union hv_partition_processor_features...
> 
> > +{
> > +    int ret;
> > +
> > +    ret = get_partition_property(vm_fd,
> > +                                 HV_PARTITION_PROPERTY_PROCESSOR_FEATURES0,
> > +                                 features[0].as_uint64);
> > +    if (ret < 0) {
> > +        error_report("Failed to get processor features bank 0");
> > +        return -1;
> > +    }
> > +
> > +    ret = get_partition_property(vm_fd,
> > +                                 HV_PARTITION_PROPERTY_PROCESSOR_FEATURES1,
> > +                                 features[1].as_uint64);
> 
> ...and treat that pointer as an array of at least 2 elements.
> 
> > +    if (ret < 0) {
> > +        error_report("Failed to get processor features bank 1");
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static int create_partition(int mshv_fd, int *vm_fd)
> >  {
> >      int ret;
> > @@ -520,6 +571,12 @@ static int mshv_init(AccelState *as, MachineState *ms)
> >hv_partition_processor_features
> >      s->vm = vm_fd;
> >      s->fd = mshv_fd;
> > +
> > +    ret = get_proc_features(vm_fd, &s->processor_features);
> 
> But here we pass in s->processor_features, which has only
> one hv_partition_processor_features, not 2. So the [1]
> element access will read beyond the size of the field.
> 
> My guess is that the intention here was "features.as_uint64[1]"
> rather than "features[1].as_uint64" (and similarly for 0,
> though there it doesn't actually walk off the end of the field).
> 

Hi Peter,

yes, indeed. this is a bug, it should be uint64_t[0] and uint64_t[1]
instead of features[0] and features[1]. Ironically I spend some time
debugging this very bug today, albeit _before_ reading this email.

it's causing some breakage for migrating guest with CET support (or any
feature that is part of the second bank of processor features, I guess).

thanks!

magnus

> thanks
> -- PMM