Re: [RFC PATCH 03/12] drivers: hv: Reserve memory for VSM secure kernel during early boot

[sashiko-bot@kernel.org]

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 <tgopinath@linux.microsoft.com>
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/20260901165647.3160413-1-tgopinath@linux.microsoft.com?part=3