[PATCH v2] Drivers: hv: mshv: fix integer overflow in memory region overlap check

[PATCH v2] Drivers: hv: mshv: fix integer overflow in memory region overlap check

[Junrui Luo]

mshv_partition_create_region() computes mem->guest_pfn + nr_pages to
check for overlapping regions without verifying u64 wraparound. A
sufficiently large guest_pfn can cause the addition to overflow,
bypassing the overlap check and allowing creation of regions that wrap
around the address space.

Fix by using check_add_overflow() to reject such regions early, and
validate that the region end does not exceed MAX_PHYSMEM_BITS. These
checks also protect downstream callers that compute start_gfn +
nr_pages on stored regions without overflow guards.

Fixes: 621191d709b1 ("Drivers: hv: Introduce mshv_root module to expose /dev/mshv to VMMs")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Suggested-by: Roman Kisel <romank@linux.microsoft.com>
Cc: stable@vger.kernel.org
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
Changes in v2:
- Add a maximum check suggested by Roman Kisel
- Link to v1: https://lore.kernel.org/all/SYBPR01MB7881689C0F58149DD986A6D1AF49A@SYBPR01MB7881.ausprd01.prod.outlook.com/
---
 drivers/hv/mshv_root_main.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
index 6f42423f7faa..32826247dbce 100644
--- a/drivers/hv/mshv_root_main.c
+++ b/drivers/hv/mshv_root_main.c
@@ -1174,11 +1174,20 @@ static int mshv_partition_create_region(struct mshv_partition *partition,
 {
 	struct mshv_mem_region *rg;
 	u64 nr_pages = HVPFN_DOWN(mem->size);
+	u64 new_region_end;
+
+	/* Reject regions whose end address would wrap around */
+	if (check_add_overflow(mem->guest_pfn, nr_pages, &new_region_end))
+		return -EOVERFLOW;
+
+	/* Reject regions beyond the maximum physical address */
+	if (new_region_end > HVPFN_DOWN(1ULL << MAX_PHYSMEM_BITS))
+		return -EINVAL;
 
 	/* Reject overlapping regions */
 	spin_lock(&partition->pt_mem_regions_lock);
 	hlist_for_each_entry(rg, &partition->pt_mem_regions, hnode) {
-		if (mem->guest_pfn + nr_pages <= rg->start_gfn ||
+		if (new_region_end <= rg->start_gfn ||
 		    rg->start_gfn + rg->nr_pages <= mem->guest_pfn)
 			continue;
 		spin_unlock(&partition->pt_mem_regions_lock);

---
base-commit: c369299895a591d96745d6492d4888259b004a9e
change-id: 20260328-fixes-0296eb3dbb52

Best regards,
-- 
Junrui Luo <moonafterrain@outlook.com>

Re: [PATCH v2] Drivers: hv: mshv: fix integer overflow in memory region overlap check

[Stanislav Kinsburskii]

On Sat, Mar 28, 2026 at 05:18:45PM +0800, Junrui Luo wrote:
> mshv_partition_create_region() computes mem->guest_pfn + nr_pages to
> check for overlapping regions without verifying u64 wraparound. A
> sufficiently large guest_pfn can cause the addition to overflow,
> bypassing the overlap check and allowing creation of regions that wrap
> around the address space.
> 
> Fix by using check_add_overflow() to reject such regions early, and
> validate that the region end does not exceed MAX_PHYSMEM_BITS. These
> checks also protect downstream callers that compute start_gfn +
> nr_pages on stored regions without overflow guards.
> 
> Fixes: 621191d709b1 ("Drivers: hv: Introduce mshv_root module to expose /dev/mshv to VMMs")
> Reported-by: Yuhao Jiang <danisjiang@gmail.com>
> Suggested-by: Roman Kisel <romank@linux.microsoft.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
> ---
> Changes in v2:
> - Add a maximum check suggested by Roman Kisel
> - Link to v1: https://lore.kernel.org/all/SYBPR01MB7881689C0F58149DD986A6D1AF49A@SYBPR01MB7881.ausprd01.prod.outlook.com/
> ---
>  drivers/hv/mshv_root_main.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index 6f42423f7faa..32826247dbce 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
> @@ -1174,11 +1174,20 @@ static int mshv_partition_create_region(struct mshv_partition *partition,
>  {
>  	struct mshv_mem_region *rg;
>  	u64 nr_pages = HVPFN_DOWN(mem->size);
> +	u64 new_region_end;
> +

Minor nit: just "end" or even "tmp" would be sufficient, since it's only
used for the overflow checks. "new_region_end" is a bit verbose and it's
not really "new" per se.

> +	/* Reject regions whose end address would wrap around */
> +	if (check_add_overflow(mem->guest_pfn, nr_pages, &new_region_end))
> +		return -EOVERFLOW;
> +
> +	/* Reject regions beyond the maximum physical address */
> +	if (new_region_end > HVPFN_DOWN(1ULL << MAX_PHYSMEM_BITS))

This is a PFN, so the check should be against MAX_PHYSMEM_BITS -
PAGE_SHIFT, right?
Or maybe it's even better to use "pfn_valid"?

Thanks,
Stanislav

> +		return -EINVAL;
>  
>  	/* Reject overlapping regions */
>  	spin_lock(&partition->pt_mem_regions_lock);
>  	hlist_for_each_entry(rg, &partition->pt_mem_regions, hnode) {
> -		if (mem->guest_pfn + nr_pages <= rg->start_gfn ||
> +		if (new_region_end <= rg->start_gfn ||
>  		    rg->start_gfn + rg->nr_pages <= mem->guest_pfn)
>  			continue;
>  		spin_unlock(&partition->pt_mem_regions_lock);
> 
> ---
> base-commit: c369299895a591d96745d6492d4888259b004a9e
> change-id: 20260328-fixes-0296eb3dbb52
> 
> Best regards,
> -- 
> Junrui Luo <moonafterrain@outlook.com>

Re: [PATCH v2] Drivers: hv: mshv: fix integer overflow in memory region overlap check

[Junrui Luo]

Hi Stanislav,

Thanks for the review.

On Mon, Mar 30, 2026 at 02:13:53PM -0700, Stanislav Kinsburskii wrote:
> Minor nit: just "end" or even "tmp" would be sufficient, since it's only
> used for the overflow checks. "new_region_end" is a bit verbose and it's
> not really "new" per se.

I will rename it to “region_end" in v3.

> This is a PFN, so the check should be against MAX_PHYSMEM_BITS -
> PAGE_SHIFT, right?

HVPFN_DOWN() is defined as (x) >> HV_HYP_PAGE_SHIFT, so
HVPFN_DOWN(1ULL << MAX_PHYSMEM_BITS) already expands to
1ULL << (MAX_PHYSMEM_BITS - HV_HYP_PAGE_SHIFT).

Thanks,
Junrui Luo

Re: [PATCH v2] Drivers: hv: mshv: fix integer overflow in memory region overlap check

[Stanislav Kinsburskii]

On Sat, Mar 28, 2026 at 05:18:45PM +0800, Junrui Luo wrote:
> mshv_partition_create_region() computes mem->guest_pfn + nr_pages to
> check for overlapping regions without verifying u64 wraparound. A
> sufficiently large guest_pfn can cause the addition to overflow,
> bypassing the overlap check and allowing creation of regions that wrap
> around the address space.
> 
> Fix by using check_add_overflow() to reject such regions early, and
> validate that the region end does not exceed MAX_PHYSMEM_BITS. These
> checks also protect downstream callers that compute start_gfn +
> nr_pages on stored regions without overflow guards.
> 
> Fixes: 621191d709b1 ("Drivers: hv: Introduce mshv_root module to expose /dev/mshv to VMMs")
> Reported-by: Yuhao Jiang <danisjiang@gmail.com>
> Suggested-by: Roman Kisel <romank@linux.microsoft.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
> ---
> Changes in v2:
> - Add a maximum check suggested by Roman Kisel
> - Link to v1: https://lore.kernel.org/all/SYBPR01MB7881689C0F58149DD986A6D1AF49A@SYBPR01MB7881.ausprd01.prod.outlook.com/
> ---
>  drivers/hv/mshv_root_main.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index 6f42423f7faa..32826247dbce 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
> @@ -1174,11 +1174,20 @@ static int mshv_partition_create_region(struct mshv_partition *partition,
>  {
>  	struct mshv_mem_region *rg;
>  	u64 nr_pages = HVPFN_DOWN(mem->size);
> +	u64 new_region_end;
> +
> +	/* Reject regions whose end address would wrap around */
> +	if (check_add_overflow(mem->guest_pfn, nr_pages, &new_region_end))
> +		return -EOVERFLOW;
> +
> +	/* Reject regions beyond the maximum physical address */

nit: both comments are redundant - the meaning is clear from the code
itself.

> +	if (new_region_end > HVPFN_DOWN(1ULL << MAX_PHYSMEM_BITS))

This maximum value check bugs me a bit.

First of all, why does it matter what is the region end? Potentially, there can be
regions not backed by host address space (leave alone host RAM), so why
intropducing this limitation?

Second, this check takes a host-specific constant (MAX_PHYSMEM_BITS) and rounds it down
to hypervisor-specific units which may not be aligned with the host page
size. Should this be host pages instead?

Thanks,
Stanislav

> +		return -EINVAL;
>  
>  	/* Reject overlapping regions */
>  	spin_lock(&partition->pt_mem_regions_lock);
>  	hlist_for_each_entry(rg, &partition->pt_mem_regions, hnode) {
> -		if (mem->guest_pfn + nr_pages <= rg->start_gfn ||
> +		if (new_region_end <= rg->start_gfn ||
>  		    rg->start_gfn + rg->nr_pages <= mem->guest_pfn)
>  			continue;
>  		spin_unlock(&partition->pt_mem_regions_lock);
> 
> ---
> base-commit: c369299895a591d96745d6492d4888259b004a9e
> change-id: 20260328-fixes-0296eb3dbb52
> 
> Best regards,
> -- 
> Junrui Luo <moonafterrain@outlook.com>

Re: [PATCH v2] Drivers: hv: mshv: fix integer overflow in memory region overlap check

[Junrui Luo]

On Thu, Apr 02, 2026 at 04:25:02PM -0700, Stanislav Kinsburskii wrote:
> nit: both comments are redundant - the meaning is clear from the code
> itself.

I will drop them in v3.

> This maximum value check bugs me a bit.
> 
> First of all, why does it matter what is the region end? Potentially, there can be
> regions not backed by host address space (leave alone host RAM), so why
> intropducing this limitation?
> 
> Second, this check takes a host-specific constant (MAX_PHYSMEM_BITS) and rounds it down
> to hypervisor-specific units which may not be aligned with the host page
> size. Should this be host pages instead?
 
This check was suggested by Roman in v1 review. Roman, could you
share your thoughts on Stanislav's concerns? I'd like to align on whether an upper
bound check is needed here.

Thanks,
Junrui Luo

Re: [PATCH v2] Drivers: hv: mshv: fix integer overflow in memory region overlap check

[vdso]

> On 04/09/2026 8:06 PM PDT Junrui Luo <moonafterrain@outlook.com> wrote:
> 
>  
> On Thu, Apr 02, 2026 at 04:25:02PM -0700, Stanislav Kinsburskii wrote:
> > nit: both comments are redundant - the meaning is clear from the code
> > itself.
> 
> I will drop them in v3.
> 
> > This maximum value check bugs me a bit.
> > 
> > First of all, why does it matter what is the region end? Potentially, there can be
> > regions not backed by host address space (leave alone host RAM), so why
> > intropducing this limitation?
> > 
> > Second, this check takes a host-specific constant (MAX_PHYSMEM_BITS) and rounds it down
> > to hypervisor-specific units which may not be aligned with the host page
> > size. Should this be host pages instead?
>  
> This check was suggested by Roman in v1 review. Roman, could you
> share your thoughts on Stanislav's concerns? I'd like to align on whether an upper
> bound check is needed here.

Hey Junrui, Stanislav,

The idea was that there is a max PFN/GFN going over which is _architecturally_ impossible.
It might be ((1 << 52) - 1) << 12 or ((1 << 48) - 1) << 12 or similar depending on how
many bits are used for the guest phys. address (52, 48, 36) and page sizes. The gist is
that max is way below 0xFFFF_FFFF_FFFF_FFFF (1<<64 - 1) used for overflow checking in the
original v1 patch. Hence, instead of checking for overflow, one may check for that max
PFN/GFN catching way more bad input. Checking for that max also feels as more
domain-specific compared to the "generic" overflow check.

I went for the trade-off where the "impossible" PFN/GFN is computed simply as
(1 << max_possible_bits_in_the_address)/the_min_possible_page_size. Again, that's simple
and better than the oveflow check as it catches the region with PFNs/GFNs that just cannot
be used. I agree that may overshoot, and an even more specific check would have to fetch the
CPUID for the VM (or its ARM64 MMFR regs), etc. to get more specifics and a lower bound
on the bad GFN (but at the cost of more computation).

All in all, from the three options of (generic check for overflow, simple check
for arch bad PFNs/GFNs, an elaborated check with all specifics) I suggested the simple check.
Fast and still more useful than checking for overflow in my opinion.

Below I go into various details on "why impossible" and these 48 and 52 bits, and I most
likely will be re-iterating things that you know - just for the convenience of other
folks who don't play with MMU/EPT/etc often, and might find the topic interesting.
It may happen that I'll also learn something from your critique, thanks in advance :)

That max PFN/GFN is dictated by the maximum number of bits allowed to be used in
the physical address. Above that, an address would not be mapped or cached by
the machines that run the code in question. The page tables won't work as the number
of bits used for the physical address here is the sum of the bits used to index
into a page table + the offset bits. That also gives the number of bits in the virtual
address which the CPU needs to address the memory (again, in this modern case;
there were many years ago machines with 32 bit virt addresses and 36 bit phys
addresses used via PAE/AWE).

E.g. for the x64_86 and 48 bits in the phys addresses: the VAs have 9+9+9+9 bits per
each page table hierarchy + 12 bits for the page offset in 4KiB (1<<12) pages. That
also can be played as 9+9+9 and 21 bits which gives 3 level paging and 2MiB (1<<21)
pages but still _48_ bits for the VA and the phys. address. Sure can be 9+9 bits for
2-level pages and 30 bits for the page offset (1GiB pages, 1 << 30). Still, _48_
bits. Similar aruphmetic goes for _52_ bit addresses but going to need 5-level pages
this time.

Now, can you have 53 bits in the phys address (to address 1<<53 bytes)? No, you
cannot as that doesn't work with the existing x86_64 MMUs (and ARM64) - no virtual
address can be constructed to address 53 bits - not enough levels in the page table
hierarchy. As no VA can be constructed, the code won't be able to access the data.

Neither the EPT/2nd level set up by the hypervisor, nor for 1st level translation
set up by the guest or the host can go higher than 52 bits, even for MMIO. When MMIO
happens, that's a 2nd level fault/not present page and goes to the hypervisor but
_still_ the hv builds the EPT/2nd level map by the rules of the architecture and
cannot go above the architecturally imposed limits.

--
Cheers,
Roman

> 
> Thanks,
> Junrui Luo