linux-kernel - Re: [PATCH] mshv: Allow mappings that overlap in uaddr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <6a5f4ed5-63ae-4760-84c9-7290aaff8bd1@linux.microsoft.com>
Date: Thu, 6 Nov 2025 10:40:50 -0800
From: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.com>,
 "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "magnuskulke@...ux.microsoft.com" <magnuskulke@...ux.microsoft.com>
Cc: "kys@...rosoft.com" <kys@...rosoft.com>,
 "haiyangz@...rosoft.com" <haiyangz@...rosoft.com>,
 "wei.liu@...nel.org" <wei.liu@...nel.org>,
 "decui@...rosoft.com" <decui@...rosoft.com>,
 "longli@...rosoft.com" <longli@...rosoft.com>,
 "skinsburskii@...ux.microsoft.com" <skinsburskii@...ux.microsoft.com>,
 "prapal@...ux.microsoft.com" <prapal@...ux.microsoft.com>,
 "mrathor@...ux.microsoft.com" <mrathor@...ux.microsoft.com>,
 "muislam@...rosoft.com" <muislam@...rosoft.com>
Subject: Re: [PATCH] mshv: Allow mappings that overlap in uaddr

On 11/6/2025 5:38 AM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@...ux.microsoft.com> Sent: Tuesday, November 4, 2025 2:19 PM
>>
>> Currently the MSHV driver rejects mappings that would overlap in
>> userspace.
>>
>> Some VMMs require the same memory to be mapped to different parts of
>> the guest's address space, and so working around this restriction is
>> difficult.
>>
>> The hypervisor itself doesn't prohibit mappings that overlap in uaddr,
>> (really in SPA: system physical addresses), so supporting this in the
>> driver doesn't require any extra work, only the checks need to be
>> removed.
>>
>> Since no userspace code up until has been able to overlap regions in
>> userspace, relaxing this constraint can't break any existing code.
>>
>> Signed-off-by: Magnus Kulke <magnuskulke@...ux.microsoft.com>
>> Signed-off-by: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
>> ---
>>  drivers/hv/mshv_root_main.c | 19 +------------------
>>  include/uapi/linux/mshv.h   |  2 +-
>>  2 files changed, 2 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
>> index 814465a0912d..e5da5f2ab6f7 100644
>> --- a/drivers/hv/mshv_root_main.c
>> +++ b/drivers/hv/mshv_root_main.c
>> @@ -1206,21 +1206,6 @@ mshv_partition_region_by_gfn(struct mshv_partition *partition, u64 gfn)
>>  	return NULL;
>>  }
>>
>> -static struct mshv_mem_region *
>> -mshv_partition_region_by_uaddr(struct mshv_partition *partition, u64 uaddr)
>> -{
>> -	struct mshv_mem_region *region;
>> -
>> -	hlist_for_each_entry(region, &partition->pt_mem_regions, hnode) {
>> -		if (uaddr >= region->start_uaddr &&
>> -		    uaddr < region->start_uaddr +
>> -			    (region->nr_pages << HV_HYP_PAGE_SHIFT))
>> -			return region;
>> -	}
>> -
>> -	return NULL;
>> -}
>> -
>>  /*
>>   * NB: caller checks and makes sure mem->size is page aligned
>>   * Returns: 0 with regionpp updated on success, or -errno
>> @@ -1235,9 +1220,7 @@ static int mshv_partition_create_region(struct mshv_partition *partition,
>>
>>  	/* Reject overlapping regions */
>>  	if (mshv_partition_region_by_gfn(partition, mem->guest_pfn) ||
>> -	    mshv_partition_region_by_gfn(partition, mem->guest_pfn + nr_pages - 1) ||
>> -	    mshv_partition_region_by_uaddr(partition, mem->userspace_addr) ||
>> -	    mshv_partition_region_by_uaddr(partition, mem->userspace_addr + mem->size - 1))
>> +	    mshv_partition_region_by_gfn(partition, mem->guest_pfn + nr_pages - 1))
>>  		return -EEXIST;
> 
> This existing code (and after this patch) checks for overlap by seeing if the
> requested starting and ending GFNs are already in some existing region. But
> is this really sufficient to detect overlap? Consider this example:
> 
> 1. Three regions exist covering these GFNs respectively:  100 thru 199,
> 300 thru 399, and 500 thru 599.
> 2. A request is made to create a new region for GFNs 250 thru 449.
> 
> This new request would pass the check, but would still overlap. Or is there
> something that prevents this scenario?
> 

The logic appears wrong to me. I will create a patch to fix it, and amend this
patch to work with that new logic since it will look a little different. I'll
post the fix + v2 of this patch as a series.

Thanks
Nuno

>>
>>  	region = vzalloc(sizeof(*region) + sizeof(struct page *) * nr_pages);
>> diff --git a/include/uapi/linux/mshv.h b/include/uapi/linux/mshv.h
>> index 9091946cba23..b10c8d1cb2ad 100644
>> --- a/include/uapi/linux/mshv.h
>> +++ b/include/uapi/linux/mshv.h
>> @@ -123,7 +123,7 @@ enum {
>>   * @rsvd: MBZ
>>   *
>>   * Map or unmap a region of userspace memory to Guest Physical Addresses (GPA).
>> - * Mappings can't overlap in GPA space or userspace.
>> + * Mappings can't overlap in GPA space.
>>   * To unmap, these fields must match an existing mapping.
>>   */
>>  struct mshv_user_mem_region {
>> --
>> 2.34.1
> 
> I've given my Reviewed-by: narrowly for this patch, since it appears to be
> correct for what it does. But if the approach for detecting overlap really
> is faulty, an additional patch is needed that might supersede this one.
> 
> Reviewed-by: Michael Kelley <mhklinux@...look.com>