On Fri, 26 Jun 2026 at 11:23, Paolo Bonzini <[email protected]> wrote:
>
> From: Magnus Kulke <[email protected]>
>
> 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 <[email protected]>
> Link:
> https://lore.kernel.org/r/[email protected]
> Signed-off-by: Paolo Bonzini <[email protected]>
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).
thanks
-- PMM
