Thank you for your contribution! Sashiko AI review found 1 potential issue(s) 
to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch only 
partially fixes the information leak. While it zeroes the `out` page, the input 
bounce page `in` is still allocated without the `__GFP_ZERO` flag. If a user 
provides an `hvcall.input_size` that is smaller than the input size expected by 
the hypercall (specified by `hvcall.control`), `copy_from_user()` will only 
partially initialize the `in` page. The remainder of the page will contain 
uninitialized, stale kernel memory. This full page is then passed to the 
hypervisor. If the hypervisor processes these uninitialized bytes and reflects 
them into the `out` page or alters its observable behavior based on them, 
sensitive kernel data is leaked to userspace.
--

commit d8767fa45f0b927a8630a3fee3fba738b9ae7851
Author: Yousef Alhouseen <[email protected]>

hyperv: mshv: zero VTL hypercall output page
    
This patch zeroes the hypercall output page to avoid copying stale memory
to userspace and adds NULL pointer checks for the page allocations.

> diff --git a/drivers/hv/mshv_vtl_main.c b/drivers/hv/mshv_vtl_main.c
> index 0d3d4161974f8..0365d207cb110 100644
> --- a/drivers/hv/mshv_vtl_main.c
> +++ b/drivers/hv/mshv_vtl_main.c
> @@ -1147,7 +1147,11 @@ static int mshv_vtl_hvcall_call(struct 
> mshv_vtl_hvcall_fd *fd,
>        * TODO: Take care of this when CVM support is added.
>        */
>       in = (void *)__get_free_page(GFP_KERNEL);

[Severity: High]
This isn't a bug introduced by this patch, but should the "in" page also
be allocated with __GFP_ZERO to prevent leaking stale kernel memory?

If userspace provides an hvcall.input_size that is smaller than what the
hypercall expects, copy_from_user() will only partially initialize the
"in" page. The remainder of the page will contain uninitialized kernel
memory which is then passed to the hypervisor in hv_do_hypercall(). If the
hypervisor reflects this input or alters behavior based on it, could this
result in an information leak?

> -     out = (void *)__get_free_page(GFP_KERNEL);
> +     out = (void *)__get_free_page(GFP_KERNEL | __GFP_ZERO);
> +     if (!in || !out) {
> +             ret = -ENOMEM;
> +             goto free_pages;
> +     }

-- 
Sashiko AI review ยท 
https://sashiko.dev/#/patchset/[email protected]?part=1

Reply via email to