Re: [RFC PATCH 06/12] drivers: hv: Add VSM boot driver and enable VTL1 at the partition level

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect validation of the MBEC enablement bitmask allows silent failure of VTL1 security configuration.
- [High] Missing MBEC validation when VTL1 is found to be already enabled, allowing acceptance of insecure pre-configured VTL1 states.
- [Medium] hv_vsm_boot_init pins initialization to cpumask_first(cpu_online_mask), which is incorrect for kexec/kdump where the boot CPU may not be CPU 0.
--

commit cce0c5216b1bbddd78ce31614c3d1587f8bcd4ea
Author: Thara Gopinath <tgopinath@linux.microsoft.com>

drivers: hv: Add VSM boot driver and enable VTL1 at the partition level

This commit introduces a driver to bring up Linux VBS (LVBS) using Hyper-V's
Virtual Secure Mode. It provides the initial sequence to probe privileges, pin
execution to the VTL0 boot CPU, and enable VTL1 at the partition level with
MBEC.

> diff --git a/drivers/hv/hv_vsm_boot.c b/drivers/hv/hv_vsm_boot.c
> new file mode 100644
> index 0000000000000..99e4dc8695837
> --- /dev/null
> +++ b/drivers/hv/hv_vsm_boot.c
[ ... ]
> +static int __init hv_vsm_bootstrap_vtl(void)
> +{
> +	u16 partition_enabled_vtl_set = 0, partition_mbec_enabled_vtl_set = 0;
> +	u8 partition_max_vtl;
> +	int ret;
> +
> +	/* Check and enable VTL1 at the partition level */
> +	ret = hv_vsm_get_partition_status(&partition_enabled_vtl_set, &partition_max_vtl,
> +					  &partition_mbec_enabled_vtl_set);
> +	if (ret)
> +		return ret;
> +
> +	if (partition_max_vtl < HV_VTL_SECURE) {
> +		pr_err("VTL1 is not supported by the partition\n");
> +		return -EINVAL;
> +	}
> +
> +	if (partition_enabled_vtl_set & HV_VTL1_ENABLE_BIT) {
> +		pr_info("Partition VTL1 is already enabled\n");
> +	} else {

[Severity: High]
Does this code skip verifying MBEC when VTL1 is already enabled?

If an attacker or previous boot stage races to enable VTL1 but omits
enabling MBEC, checking partition_enabled_vtl_set here finds VTL1 enabled
and skips further configuration without validating
partition_mbec_enabled_vtl_set. Could this allow acceptance of insecure
pre-configured VTL1 states?

> +		ret = hv_vsm_enable_partition_vtl();
> +		if (ret) {
> +			pr_err("Enabling Partition VTL1 failed with status 0x%x\n",
> +			       ret);
> +			return -EINVAL;
> +		}
> +		ret = hv_vsm_get_partition_status(&partition_enabled_vtl_set, &partition_max_vtl,
> +						  &partition_mbec_enabled_vtl_set);
> +		if (ret)
> +			return ret;
> +		if (!(partition_enabled_vtl_set & HV_VTL1_ENABLE_BIT)) {
> +			pr_err("Tried Enabling Partition VTL 1 and still failed\n");
> +			return -EINVAL;
> +		}
> +		if (!partition_mbec_enabled_vtl_set) {

[Severity: High]
Since partition_mbec_enabled_vtl_set is a bitmask of all VTLs with MBEC,
does this validation only check if the mask is completely empty?

If VTL0 already has MBEC enabled (bit 0 is 1), the mask is non-zero, causing
this check to falsely pass even if the hypervisor failed to enable MBEC for
VTL1. Should this instead isolate the VTL1 bit, for example:
!(partition_mbec_enabled_vtl_set & HV_VTL1_ENABLE_BIT) ?

> +			pr_err("Tried Enabling Partition MBEC and failed\n");
> +			return -EINVAL;
> +		}
> +	}
> +	return 0;
> +}
[ ... ]
> +static int __init hv_vsm_boot_init(void)
> +{
> +	cpumask_var_t mask;
> +	unsigned int boot_cpu;
> +	int ret;
> +
> +	if (!vsm_arch_has_vsm_access())
> +		return 0;
> +
> +	/*
> +	 * Copy the current cpu mask and pin rest of the running code to boot cpu.
> +	 * Important since we want boot cpu of VTL0 to be the boot cpu for VTL1.
> +	 * ToDo: Check if copying and restoring current->cpus_mask is enough
> +	 * ToDo: Verify the assumption that cpumask_first(cpu_online_mask) is
> +	 * the boot cpu
> +	 */
> +	if (!alloc_cpumask_var(&mask, GFP_KERNEL))
> +		panic("Could not allocate cpumask");
> +
> +	cpumask_copy(mask, &current->cpus_mask);
> +	boot_cpu = cpumask_first(cpu_online_mask);
> +	set_cpus_allowed_ptr(current, cpumask_of(boot_cpu));

[Severity: Medium]
Will this incorrectly pin the task to the lowest-numbered online CPU rather
than the actual boot CPU during a kexec or kdump?

If the kernel is booted via kexec or kdump on a non-zero CPU, lower-numbered
CPUs might be brought online during SMP initialization. In this scenario,
cpumask_first(cpu_online_mask) returns the lowest numbered online CPU, which
is not necessarily the Bootstrap Processor (BSP). Is get_boot_cpu_id() the
proper API to use here?

> +
> +	ret = hv_vsm_bootstrap_vtl();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901165647.3160413-1-tgopinath@linux.microsoft.com?part=6