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 <[email protected]> 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, ¤t->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/[email protected]?part=6
