Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Using global `current->tgid` to track and authorize partition ownership introduces a PID reuse vulnerability and breaks support for VMMs managing multiple partitions. - [High] `mshv_current_partid()` returns a raw partition ID without acquiring a reference, leading to a TOCTOU race where the partition can be freed and its ID reused. Pre-existing issues: - [High] VPs are freed before their parent partition is unlinked from the RCU hash table, causing a Use-After-Free for concurrent RCU readers. -- --- Patch [1]: [PATCH V4 1/9] mshv: Provide a way to get partition ID if running in a VMM process --- commit ce04f426ae72519f5c20b1bad8ffd34a07810200 Author: Mukesh R <[email protected]> mshv: Provide a way to get partition ID if running in a VMM process Many PCI passthru related hypercalls require partition ID of the target guest. Guests are actually managed by MSHV driver and the partition ID is only maintained there. Add a field in the partition struct in MSHV driver to save the tgid of the VMM process creating the partition, and add a function there to retrieve partition ID if the current process is a VMM process. > diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c > index 146726cc4e9ba..f1ff45a77b42c 100644 > --- a/drivers/hv/mshv_root_main.c > +++ b/drivers/hv/mshv_root_main.c > @@ -1908,6 +1908,27 @@ mshv_partition_release(struct inode *inode, struct > file *filp) > return 0; > } [Severity: High] This is a pre-existing issue, but there appears to be a use-after-free condition when a partition is being destroyed. In destroy_partition(), child mshv_vp objects are freed while the parent partition remains linked in the RCU-protected mshv_root.pt_htable: destroy_partition() { ... kfree(vp); partition->pt_vp_array[i] = NULL; ... remove_partition(partition); } If a concurrent asynchronous interrupt (like handle_bitset_message) looks up the partition via mshv_partition_find() and accesses the VP before remove_partition() unlinks the partition and waits for a grace period, could the ISR call kick_vp(vp) on freed memory? > +/* Given a process tgid, return partition id if it is a VMM process */ > +u64 mshv_current_partid(void) > +{ > + struct mshv_partition *pt; > + int i; > + u64 ret_ptid = HV_PARTITION_ID_INVALID; > + > + rcu_read_lock(); > + > + hash_for_each_rcu(mshv_root.pt_htable, i, pt, pt_hnode) { > + if (pt->pt_vmm_tgid == current->tgid) { > + ret_ptid = pt->pt_id; > + break; > + } > + } [Severity: High] Is it safe to track partition ownership using a global transient identifier like current->tgid instead of an explicit file descriptor or reference-counted token? If a single VMM process creates multiple partitions, wouldn't mshv_current_partid() arbitrarily return the first one found in the hash table, breaking PCI passthru operations for all subsequent partitions? Furthermore, could PID recycling lead to a situation where a new VMM process accidentally matches an old partition whose fd was passed to a long-running process that outlived the original creator? > + > + rcu_read_unlock(); > + return ret_ptid; [Severity: High] Does returning the raw pt_id here without acquiring a reference to the partition object introduce a race condition? Once the RCU read lock is dropped, a concurrent thread could close the partition fd, causing destroy_partition() to delete it in the hypervisor. If the hypervisor reuses this ID for a newly created partition belonging to a different VMM, could the original caller's subsequent PCI passthru hypercall mistakenly target the unrelated partition? > +} > +EXPORT_SYMBOL_GPL(mshv_current_partid); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
