Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] The automatic memory reservation fallback is never executed if the `securekernel` parameter is omitted from the kernel command line. - [High] Manual string parsing of boot_command_line uses a fragile substring match, misparsing unrelated parameters and ignoring the pre-parsed setup argument. - [Medium] The `__setup` handler returns 0 on success and -EINVAL on failure, inverting the API contract and leaking the parameter to the init process. - [Low] parse_securekernel_simple fails to validate trailing characters after parsing the base address. --
commit c057f006b3a9db9a754349283e3c7dd23550b133 Author: Thara Gopinath <[email protected]> Date: Tue Sep 1 16:55:17 2026 +0000 drivers: hv: Reserve memory for VSM secure kernel during early boot The VSM secure kernel runs in VTL1 and requires its memory to be reserved early. This commit introduces a driver to handle early boot reservation of this memory, either parsed from the securekernel= command line parameter or calculated automatically as a fallback. > diff --git a/drivers/hv/hv_vsm_securekernel.c > b/drivers/hv/hv_vsm_securekernel.c > new file mode 100644 > index 0000000000000..f90f6204b4d18 > --- /dev/null > +++ b/drivers/hv/hv_vsm_securekernel.c [ ... ] > +static int __init parse_securekernel_simple(char *cmdline, > + unsigned long long > *securekernel_size, > + unsigned long long > *securekernel_base) > +{ > + char *cur = cmdline; > + > + *securekernel_size = memparse(cmdline, &cur); > + if (cmdline == cur) { > + pr_warn("securekernel: memory value expected\n"); > + return -EINVAL; > + } > + > + if (*cur == '@') { > + *securekernel_base = memparse(cur + 1, &cur); > + } else if (*cur != ' ' && *cur != '\0') { [Severity: Low] Does this code skip trailing character validation if @ was found? Because the validation is inside an else if, if a user specifies a base address with trailing non-space characters (e.g., securekernel=16M@2G_foo), memparse() consumes the valid number, but the remaining garbage characters are silently ignored without raising an error. > + pr_warn("securekernel: unrecognized char: %c\n", *cur); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static __init char *get_last_securekernel(char *cmdline, const char *name) > +{ > + char *p = cmdline, *sk_cmdline = NULL; > + > + /* find securekernel and use the last one if there are more */ > + p = strstr(p, name); > + while (p) { > + sk_cmdline = p; > + p = strstr(p + 1, name); [Severity: High] Does this unbounded substring search misparse unrelated parameters? Because it lacks word-boundary or prefix validation, strstr() will match the substring anywhere in the command line. The driver could misparse independent kernel parameters (e.g., nosecurekernel=100M). Furthermore, since it explicitly searches for the last occurrence, an unrelated parameter at the end of the command line containing the substring securekernel= could overwrite a valid reservation parameter. > + } > + > + if (!sk_cmdline) > + return NULL; > + > + return sk_cmdline; > +} [ ... ] > +static int __init hv_vsm_seckernel_mem_init(char *__unused) [Severity: High] Is it intentional to discard the correctly pre-parsed parameter value provided by the __setup infrastructure? The __setup() macro already does exact matching and provides the specific parameter value to the callback via its argument. By explicitly ignoring this argument (naming it __unused) and implementing a custom parser over the global boot_command_line, the driver uses a fragile manual parsing routine instead of the established API. > +{ > + unsigned long long securekernel_size = 0, securekernel_base = 0; > + int ret; [ ... ] > + /* Default: Find the base address automatically */ > + if (!securekernel_base) { > + securekernel_base = > memblock_phys_alloc_range(securekernel_size, SECKERNEL_ALIGN, > + 0, > SECKERNEL_ADDR_MAX); > + if (!securekernel_base) { > + pr_err("%s: Securekernel reservation failed-VSM will > not be enabled.\n", > + __func__); > + return -EINVAL; [Severity: Medium] Will returning -EINVAL (non-zero) on the error path incorrectly tell the kernel's parser that the parameter was successfully handled? The kernel API documentation requires __setup handlers to return 1 (or non-zero) if handled and 0 if not. Returning a non-zero error code treats the failed parameter as successfully consumed. > + } > + } > + > + pr_info("Reserving %ldMB of memory at 0x%llx(%ld MB) for > securekernel(System RAM:%ldMB)\n", > + (unsigned long)(securekernel_size >> 20), > + securekernel_base, > + (unsigned long)(securekernel_base >> 20), > + (unsigned long)(memblock_phys_mem_size() >> 20)); > + > + sk_res.start = securekernel_base; > + sk_res.end = securekernel_base + securekernel_size - 1; > + insert_resource(&iomem_resource, &sk_res); > + > + return 0; [Severity: Medium] Will returning 0 on the success path inform the kernel's setup infrastructure that the parameter was not handled? This causes the kernel to erroneously pass the securekernel=... parameter as an environment variable to the init process. The handler should return 1 when the parameter is successfully handled. > +} > +__setup("securekernel", hv_vsm_seckernel_mem_init); [Severity: High] Can the automatic memory reservation fallback ever execute if the parameter is omitted from the kernel command line? Functions registered with __setup() are only invoked by init/main.c if the target string matches a command-line parameter. If absent, the initialization function is completely bypassed, meaning the core feature (automatic memory reservation) fails entirely by default, and the VSM secure kernel will not boot due to lack of reserved memory. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
