Re: [PATCH v2 06/13] mshv: detect and report SEV-SNP support at init

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The shared per-CPU hypercall output buffer is read after local interrupts are restored, creating a race condition that can lead to data corruption.

Pre-existing issues:
- [High] Userspace can access the `/dev/mshv` character device before the root driver initialization is complete, exposing uninitialized structures.
--

--- Patch [6]: [PATCH v2 06/13] mshv: detect and report SEV-SNP support at init ---
commit e5cd5a244ce2b3b2547714151c9b94d944d9fd7b
Author: Wei Liu <[email protected]>

mshv: detect and report SEV-SNP support at init

Query Hyper-V dynamic processor features during mshv initialization to
determine SEV-SNP availability, and report the result. On success, log the
maximum number of encrypted partitions, otherwise log the decoded SNP status
string so misconfigurations are diagnosable.

> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index 8c87dd8e0378f..712f45dcef60c 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c

[ ... ]

> @@ -2705,6 +2705,100 @@ static int mshv_root_scheduler_cleanup(unsigned int cpu)
>  }
>  
>  /* Must be called after retrieving the scheduler type */
> +#if defined(__x86_64__)

[ ... ]

> +static void mshv_print_max_sev_snp_partitions(struct device *dev)
> +{
> +	struct hv_input_get_system_property *input;
> +	struct hv_output_get_system_property *output;
> +	unsigned long flags;
> +	u64 status;
> +
> +	local_irq_save(flags);
> +	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> +	output = *this_cpu_ptr(hyperv_pcpu_output_arg);
> +
> +	memset(input, 0, sizeof(*input));
> +	input->property_id = HV_DYNAMIC_PROCESSOR_FEATURE_PROPERTY;
> +	input->hv_processor_feature =
> +		HV_X64_DYNAMIC_PROCESSOR_FEATURE_MAX_ENCRYPTED_PARTITIONS;
> +
> +	status = hv_do_hypercall(HVCALL_GET_SYSTEM_PROPERTY, input, output);
> +	local_irq_restore(flags);
> +	if (!hv_result_success(status)) {
> +		dev_warn(dev, "Failed to get max SNP partitions: %s\n",
> +			 hv_result_to_string(status));
> +		return;
> +	}
> +
> +	dev_info(dev, "Maximum supported SEV-SNP partitions are: %llu\n",
> +		 output->hv_processor_feature_value);

[Severity: Medium]
Could this result in a corrupted value being read from the output buffer?

By calling local_irq_restore(flags) before reading
output->hv_processor_feature_value, an interrupt could occur immediately
after IRQs are restored. If the interrupt handler executes another hypercall,
the per-CPU output buffer (hyperv_pcpu_output_arg) will be overwritten before
the value is logged. 

Should the value be copied to a local variable before restoring local
interrupts?

> +}
> +
> +static void __init mshv_check_sev_snp_support(struct device *dev)
> +{
> +	struct hv_input_get_system_property *input;
> +	struct hv_output_get_system_property *output;
> +	unsigned long flags;
> +	enum hv_snp_status snp_status;
> +	u64 status;
> +
> +	local_irq_save(flags);
> +	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> +	output = *this_cpu_ptr(hyperv_pcpu_output_arg);
> +
> +	memset(input, 0, sizeof(*input));
> +	input->property_id = HV_DYNAMIC_PROCESSOR_FEATURE_PROPERTY;
> +	input->hv_processor_feature = HV_X64_DYNAMIC_PROCESSOR_FEATURE_SNP_STATUS;
> +
> +	status = hv_do_hypercall(HVCALL_GET_SYSTEM_PROPERTY, input, output);
> +	local_irq_restore(flags);
> +	if (!hv_result_success(status)) {
> +		dev_warn(dev, "Failed to get SNP support: %s\n",
> +			 hv_result_to_string(status));
> +		return;
> +	}
> +
> +	snp_status = output->hv_processor_feature_value;

[Severity: Medium]
If an interrupt overwrites the buffer after local_irq_restore(flags) but
before snp_status is assigned, snp_status could read a corrupted value.

Is there a similar race condition here as seen in
mshv_print_max_sev_snp_partitions()?

> +	dev_info(dev, "SEV-SNP support status: %s (%u)\n",
> +		 hv_snp_status_to_string(snp_status), snp_status);
> +
> +	if (snp_status == HV_SNP_STATUS_AVAILABLE)
> +		mshv_print_max_sev_snp_partitions(dev);
> +}

[ ... ]

> @@ -2808,6 +2902,8 @@ static int __init mshv_parent_partition_init(void)
>  	if (ret)
>  		goto synic_cleanup;
>  
> +	mshv_check_sev_snp_support(dev);
> +

[Severity: High]
This is a pre-existing issue, but is it safe to expose the character device
before the root driver initialization is complete?

Earlier in mshv_parent_partition_init(), misc_register(&mshv_dev) is called,
which immediately exposes the device to userspace. A racing userspace thread
could open the device and trigger the MSHV_CREATE_PARTITION ioctl before
initialization finishes.

The ioctl invokes add_partition(), which attempts to acquire
mshv_root.pt_ht_lock. The lock initialization spin_lock_init(&mshv_root.pt_ht_lock)
isn't executed until much later in the setup sequence.

Wouldn't this trigger a kernel oops if accessed early?

>  	ret = root_scheduler_init(dev);
>  	if (ret)
>  		goto synic_cleanup;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6