Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] The boot CPU is incorrectly included in the `cpu_online_mask` passed to VTL1. - [High] AP VTL1 boot failures are silently ignored, leaving the system in a partially-booted secure state. - [High] Unsafe usage of `smp_processor_id()` in preemptible context and task migration race. - [Medium] Unprotected snapshot and iteration of `cpu_online_mask`. --
commit 99445d1f4eda186e7be403a5b296f1c80a9cdae6 Author: Thara Gopinath <[email protected]> drivers: hv: hv_vsm_boot: Boot secondary processors in VTL1 This commit adds support to boot secondary processors into VTL1 after the primary CPU has transitioned. It passes an online CPU mask to VTL1 via a shared page and uses a CPU-bound kthread to sequentially issue a vtlcall for each AP. > diff --git a/drivers/hv/hv_vsm_boot.c b/drivers/hv/hv_vsm_boot.c > index c4f15c42df1f6..ad161b2f56653 100644 > --- a/drivers/hv/hv_vsm_boot.c > +++ b/drivers/hv/hv_vsm_boot.c [ ... ] > +static int __init hv_vsm_boot_sec_vp_thread_fn(void *arg) > +{ > + struct hv_vsm_ap_boot_ctx *ctx = arg; > + struct hv_vtlcall_param args = {0}; > + int cpu = smp_processor_id(); > + u16 vp_enabled_vtl_set = 0; > + u8 active_mbec_enabled = 0; > + s64 sk_status; > + int ret = 0; > + > + pr_info("cpu%d entering vtl1 boot thread\n", cpu); > + sk_status = hv_vsm_vtlcall(&args); > + if (sk_status) > + pr_warn("VP%d VTL1 boot returned status %lld\n", cpu, > sk_status); > + > + ret = hv_vsm_get_vp_status(&vp_enabled_vtl_set, &active_mbec_enabled); > + if (ret) > + goto out; > + > + if (!active_mbec_enabled) { > + pr_err("Failed to enable MBEC for VP%d\n", cpu); > + hv_vsm_mbec_enabled = false; > + } > +out: > + ctx->ret = ret; [Severity: High] If hv_vsm_vtlcall() fails and returns an error in sk_status, will this error be silently ignored? The warning is printed, but ret remains 0 before calling hv_vsm_get_vp_status(). If hv_vsm_get_vp_status() succeeds, ret is unconditionally set to 0. Does this leave the caller believing the AP booted successfully instead of surfacing the error? > + complete(&ctx->done); > + return 0; > +} [ ... ] > +static int __init hv_vsm_boot_ap_vtl(void) > +{ > + struct hv_vtlcall_param args = {0}; > + struct page *cpu_online_page; > + unsigned int cpu, cur_cpu = smp_processor_id(); [Severity: High] Is it safe to use smp_processor_id() here in a preemptible context? Fetching it here triggers a kernel BUG warning. Also, because the thread can migrate between CPU evaluation and the sleepable operations like alloc_page() and wait_for_completion() below, could cur_cpu become stale? This might cause the AP loop to skip the new CPU and incorrectly attempt to boot the original primary CPU as an AP. > + s64 sk_status; > + int ret; > + > + cpu_online_page = hv_vsm_alloc_shared_page(); > + if (IS_ERR(cpu_online_page)) > + return PTR_ERR(cpu_online_page); > + > + cpumask_copy(page_address(cpu_online_page), cpu_online_mask); [Severity: Medium] Should the snapshot of cpu_online_mask and subsequent iteration be protected by cpus_read_lock()? Accessing this without hotplug protection could allow concurrent CPU hotplug events to alter the mask, potentially passing an inconsistent snapshot to VTL1 or binding an AP boot kthread to a newly offlined CPU. [Severity: High] Does this verbatim copy of cpu_online_mask incorrectly include the already-booted primary CPU in the mask passed to VTL1? If cur_cpu isn't cleared from the mask first, could VTL1 mistakenly expect the primary CPU to issue an AP boot vtlcall and hang or corrupt the primary CPU's secure context? > + > + /* > + * Hand VTL1 the set of APs to expect. VTL1 copies the mask > + * synchronously inside this vtlcall and does not reference the > + * page after it returns, so freeing it here is safe. > + */ > + args.a0 = VSM_VTL_CALL_FUNC_ID_BOOT_APS; > + args.a1 = page_to_pfn(cpu_online_page); > + sk_status = hv_vsm_vtlcall(&args); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
