Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Missing host access restoration on successful PSP guest request permanently revokes host permissions. - [High] Unbounded user-controlled array allocations and non-preemptible loops cause CPU soft lockups. --
commit ff3cccc86d268ae551d99e559a8b63d138066868 Author: Wei Liu <[email protected]> Subject: mshv: wire SEV-SNP partition ioctls This commit wires the SEV-SNP UAPI into partition creation, memory ownership, and isolated-page import. It adds tracking for host access permissions and handles transitions to release/acquire host access. > diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c > --- a/drivers/hv/mshv_root_main.c > +++ b/drivers/hv/mshv_root_main.c > @@ -643,6 +643,68 @@ mshv_partition_region_by_gfn_get(struct mshv_partition > *p, u64 gfn) > return region; > } > > +#ifdef HV_SUPPORTS_SEV_SNP_GUESTS > +static int > +mshv_gpfns_to_host_pages(struct mshv_partition *partition, const u64 *gpfns, > + u64 page_count, > + struct mshv_host_access_page *host_pages) > +{ > + struct mshv_mem_region *region; > + u64 i; > + int ret = 0; > + > + for (i = 0; i < page_count; i++) { > + u64 offset; > + > + region = mshv_partition_region_by_gfn_get(partition, gpfns[i]); [Severity: High] Does this code allow an unbounded, user-controlled loop without yielding? If a user provides an extremely large args.page_count in mshv_partition_ioctl_modify_gpa_host_access(), this loop will repeatedly acquire and release region mutexes for the entire array. Without a cond_resched(), this could cause a CPU soft lockup. [ ... ] > +bool mshv_u64_array_has_duplicates(u64 *values, u64 count) > +{ > + u64 i; > + > + sort(values, count, sizeof(*values), mshv_u64_cmp, NULL); [Severity: High] Can this sort() call cause a CPU soft lockup? Since count originates from the user-controlled args.page_count without an upper bound, sort() (which is non-preemptible) might block the CPU for an O(N log N) duration on a massive array. > + for (i = 1; i < count; i++) > + if (values[i - 1] == values[i]) > + return true; > + return false; > +} [ ... ] > +static long > +mshv_partition_ioctl_modify_gpa_host_access(struct mshv_partition *partition, > + void __user *user_args) > +{ [ ... ] > + guest_gpas = u64_to_user_ptr(args.guest_gpas); > + gpfns = vmemdup_array_user(guest_gpas, args.page_count, > + sizeof(*gpfns)); > + if (IS_ERR(gpfns)) { > + ret = PTR_ERR(gpfns); > + gpfns = NULL; > + goto out; > + } > + host_pages = kvmalloc_array(args.page_count, sizeof(*host_pages), > + GFP_KERNEL); > + duplicates = kvmalloc_array(args.page_count, sizeof(*duplicates), > + GFP_KERNEL); [Severity: High] Are these allocations sufficiently bounded to prevent kernel memory exhaustion? Since args.page_count is controlled by userspace, allocating multiple arrays that scale with this count could lead to severe memory pressure or a denial of service. [ ... ] > +static long > +mshv_partition_ioctl_issue_psp_guest_request(struct mshv_partition > *partition, > + void __user *user_args) > +{ [ ... ] > + ret = hv_call_issue_psp_guest_request(partition->pt_id, gpfns[0], > + gpfns[1], > + mshv_async_hvcall_handler, > + partition); > + if (!ret) > + goto out_put_pages; > + > +restore_host_access: [Severity: High] Does a successful hypercall skip host access restoration? If hv_call_issue_psp_guest_request() succeeds and ret is 0, the code jumps to out_put_pages, bypassing the restore_host_access block. This leaves the host permissions permanently revoked for the request and response pages, which could break functionality or leak memory. > + { > + int restore_ret; > + > + restore_ret = mshv_restore_psp_pages(partition, host_pages, > + original, transitioned, > + transitioned_count); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
