There's currently no verification for host issued ranges in most of the
pKVM memory transitions. The subsequent end boundary might therefore be
subject to overflow and could evade the later checks.
Close this loophole with an additional check_range_args() check on a per
public function basis.
host_unshare_guest transition is already protected via
__check_host_shared_guest(), while assert_host_shared_guest() callers
are already ignoring host checks.
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
---
v1 -> v2:
- Also check for (nr_pages * PAGE_SIZE) overflow. (Quentin)
- Rename to check_range_args().
diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
index 8957734d6183..65fcd2148f59 100644
--- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
+++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
@@ -712,6 +712,14 @@ static int __guest_check_page_state_range(struct pkvm_hyp_vm *vm, u64 addr,
return check_page_state_range(&vm->pgt, addr, size, &d);
}
+static bool check_range_args(u64 start, u64 nr_pages, u64 *size)
+{
+ if (check_mul_overflow(nr_pages, PAGE_SIZE, size))
+ return false;
+
+ return start < (start + *size);
+}
+
int __pkvm_host_share_hyp(u64 pfn)
{
u64 phys = hyp_pfn_to_phys(pfn);
@@ -772,10 +780,13 @@ int __pkvm_host_unshare_hyp(u64 pfn)
int __pkvm_host_donate_hyp(u64 pfn, u64 nr_pages)
{
u64 phys = hyp_pfn_to_phys(pfn);
- u64 size = PAGE_SIZE * nr_pages;
void *virt = __hyp_va(phys);
+ u64 size;
int ret;
+ if (!check_range_args(phys, nr_pages, &size))
+ return -EINVAL;
+
host_lock_component();
hyp_lock_component();
@@ -800,10 +811,13 @@ int __pkvm_host_donate_hyp(u64 pfn, u64 nr_pages)
int __pkvm_hyp_donate_host(u64 pfn, u64 nr_pages)
{
u64 phys = hyp_pfn_to_phys(pfn);
- u64 size = PAGE_SIZE * nr_pages;
u64 virt = (u64)__hyp_va(phys);
+ u64 size;
int ret;
+ if (!check_range_args(phys, nr_pages, &size))
+ return -EINVAL;
+
host_lock_component();
hyp_lock_component();
@@ -884,9 +898,12 @@ void hyp_unpin_shared_mem(void *from, void *to)
int __pkvm_host_share_ffa(u64 pfn, u64 nr_pages)
{
u64 phys = hyp_pfn_to_phys(pfn);
- u64 size = PAGE_SIZE * nr_pages;
+ u64 size;
int ret;
+ if (!check_range_args(phys, nr_pages, &size))
+ return -EINVAL;
+
host_lock_component();
ret = __host_check_page_state_range(phys, size, PKVM_PAGE_OWNED);
if (!ret)
@@ -899,9 +916,12 @@ int __pkvm_host_share_ffa(u64 pfn, u64 nr_pages)
int __pkvm_host_unshare_ffa(u64 pfn, u64 nr_pages)
{
u64 phys = hyp_pfn_to_phys(pfn);
- u64 size = PAGE_SIZE * nr_pages;
+ u64 size;
int ret;
+ if (!check_range_args(phys, nr_pages, &size))
+ return -EINVAL;
+
host_lock_component();
ret = __host_check_page_state_range(phys, size, PKVM_PAGE_SHARED_OWNED);
if (!ret)
@@ -945,6 +965,9 @@ int __pkvm_host_share_guest(u64 pfn, u64 gfn, u64 nr_pages, struct pkvm_hyp_vcpu
if (prot & ~KVM_PGTABLE_PROT_RWX)
return -EINVAL;
+ if (!check_range_args(phys, nr_pages, &size))
+ return -EINVAL;
+
ret = __guest_check_transition_size(phys, ipa, nr_pages, &size);
if (ret)
return ret;
base-commit: 8b789f2b7602a818e7c7488c74414fae21392b63
--
2.51.0.470.ga7dc726c21-googOn Fri, 19 Sep 2025 16:50:56 +0100,
Vincent Donnefort <vdonnefort@google.com> wrote:
>
> There's currently no verification for host issued ranges in most of the
> pKVM memory transitions. The subsequent end boundary might therefore be
> subject to overflow and could evade the later checks.
>
> Close this loophole with an additional check_range_args() check on a per
> public function basis.
>
> host_unshare_guest transition is already protected via
> __check_host_shared_guest(), while assert_host_shared_guest() callers
> are already ignoring host checks.
>
> Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
>
> ---
>
> v1 -> v2:
> - Also check for (nr_pages * PAGE_SIZE) overflow. (Quentin)
> - Rename to check_range_args().
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index 8957734d6183..65fcd2148f59 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> @@ -712,6 +712,14 @@ static int __guest_check_page_state_range(struct pkvm_hyp_vm *vm, u64 addr,
> return check_page_state_range(&vm->pgt, addr, size, &d);
> }
>
> +static bool check_range_args(u64 start, u64 nr_pages, u64 *size)
> +{
> + if (check_mul_overflow(nr_pages, PAGE_SIZE, size))
> + return false;
> +
> + return start < (start + *size);
I will echo Oliver's concern on v1: you probably want to convert the
boundary check to be inclusive of the end of the range. Otherwise, a
range that ends at the top of the 64bit range will be represented as
0, and fail the check despite being perfectly valid.
That's not a problem for PAs, as we will be stuck with at most 56bit
PAs for quite a while, but VAs are a different story, and this sort of
range check should be valid for VAs as well.
Thanks,
M.
--
Jazz isn't dead. It just smells funny.
On Sun, Sep 21, 2025 at 12:29:08PM +0100, Marc Zyngier wrote:
> On Fri, 19 Sep 2025 16:50:56 +0100,
> Vincent Donnefort <vdonnefort@google.com> wrote:
> >
> > There's currently no verification for host issued ranges in most of the
> > pKVM memory transitions. The subsequent end boundary might therefore be
> > subject to overflow and could evade the later checks.
> >
> > Close this loophole with an additional check_range_args() check on a per
> > public function basis.
> >
> > host_unshare_guest transition is already protected via
> > __check_host_shared_guest(), while assert_host_shared_guest() callers
> > are already ignoring host checks.
> >
> > Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> >
> > ---
> >
> > v1 -> v2:
> > - Also check for (nr_pages * PAGE_SIZE) overflow. (Quentin)
> > - Rename to check_range_args().
> >
> > diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > index 8957734d6183..65fcd2148f59 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > @@ -712,6 +712,14 @@ static int __guest_check_page_state_range(struct pkvm_hyp_vm *vm, u64 addr,
> > return check_page_state_range(&vm->pgt, addr, size, &d);
> > }
> >
> > +static bool check_range_args(u64 start, u64 nr_pages, u64 *size)
> > +{
> > + if (check_mul_overflow(nr_pages, PAGE_SIZE, size))
> > + return false;
> > +
> > + return start < (start + *size);
>
> I will echo Oliver's concern on v1: you probably want to convert the
> boundary check to be inclusive of the end of the range. Otherwise, a
> range that ends at the top of the 64bit range will be represented as
> 0, and fail the check despite being perfectly valid.
Do you mean allowing something like start == 0xfffffffffffff000 and size ==
4096?
But I guess that would still put all the following checks using "addr + size" at
risk. Also, I believe even the code in pgtable.c wouldn't support a such range
as it is also using a u64 end boundary.
>
> That's not a problem for PAs, as we will be stuck with at most 56bit
> PAs for quite a while, but VAs are a different story, and this sort of
> range check should be valid for VAs as well.
>
> Thanks,
>
> M.
>
> --
> Jazz isn't dead. It just smells funny.
On Mon, Sep 22, 2025 at 10:00:07PM +0100, Vincent Donnefort wrote:
> On Sun, Sep 21, 2025 at 12:29:08PM +0100, Marc Zyngier wrote:
> > On Fri, 19 Sep 2025 16:50:56 +0100,
> > Vincent Donnefort <vdonnefort@google.com> wrote:
> > >
> > > There's currently no verification for host issued ranges in most of the
> > > pKVM memory transitions. The subsequent end boundary might therefore be
> > > subject to overflow and could evade the later checks.
> > >
> > > Close this loophole with an additional check_range_args() check on a per
> > > public function basis.
> > >
> > > host_unshare_guest transition is already protected via
> > > __check_host_shared_guest(), while assert_host_shared_guest() callers
> > > are already ignoring host checks.
> > >
> > > Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> > >
> > > ---
> > >
> > > v1 -> v2:
> > > - Also check for (nr_pages * PAGE_SIZE) overflow. (Quentin)
> > > - Rename to check_range_args().
> > >
> > > diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > > index 8957734d6183..65fcd2148f59 100644
> > > --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > > +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > > @@ -712,6 +712,14 @@ static int __guest_check_page_state_range(struct pkvm_hyp_vm *vm, u64 addr,
> > > return check_page_state_range(&vm->pgt, addr, size, &d);
> > > }
> > >
> > > +static bool check_range_args(u64 start, u64 nr_pages, u64 *size)
> > > +{
> > > + if (check_mul_overflow(nr_pages, PAGE_SIZE, size))
> > > + return false;
> > > +
> > > + return start < (start + *size);
> >
> > I will echo Oliver's concern on v1: you probably want to convert the
> > boundary check to be inclusive of the end of the range. Otherwise, a
> > range that ends at the top of the 64bit range will be represented as
> > 0, and fail the check despite being perfectly valid.
>
> Do you mean allowing something like start == 0xfffffffffffff000 and size ==
> 4096?
Yes, this is what I was alluding to on v1.
> But I guess that would still put all the following checks using "addr + size" at
> risk. Also, I believe even the code in pgtable.c wouldn't support a such range
> as it is also using a u64 end boundary.
I'm not sure I follow. Ranges are pretty commonly expressed as a range
terminated by an exclusive value. This just hasn't been an issue yet as
the page table code is only ever dealing with TTBR0 or VTTBR
translations.
Anyway, I'd rather these range checks have as few assumptions of the
applied address space as possible.
Thanks,
Oliver
On Mon, Sep 22, 2025 at 04:33:24PM -0700, Oliver Upton wrote:
> On Mon, Sep 22, 2025 at 10:00:07PM +0100, Vincent Donnefort wrote:
> > On Sun, Sep 21, 2025 at 12:29:08PM +0100, Marc Zyngier wrote:
> > > On Fri, 19 Sep 2025 16:50:56 +0100,
> > > Vincent Donnefort <vdonnefort@google.com> wrote:
> > > >
> > > > There's currently no verification for host issued ranges in most of the
> > > > pKVM memory transitions. The subsequent end boundary might therefore be
> > > > subject to overflow and could evade the later checks.
> > > >
> > > > Close this loophole with an additional check_range_args() check on a per
> > > > public function basis.
> > > >
> > > > host_unshare_guest transition is already protected via
> > > > __check_host_shared_guest(), while assert_host_shared_guest() callers
> > > > are already ignoring host checks.
> > > >
> > > > Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> > > >
> > > > ---
> > > >
> > > > v1 -> v2:
> > > > - Also check for (nr_pages * PAGE_SIZE) overflow. (Quentin)
> > > > - Rename to check_range_args().
> > > >
> > > > diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > > > index 8957734d6183..65fcd2148f59 100644
> > > > --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > > > +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > > > @@ -712,6 +712,14 @@ static int __guest_check_page_state_range(struct pkvm_hyp_vm *vm, u64 addr,
> > > > return check_page_state_range(&vm->pgt, addr, size, &d);
> > > > }
> > > >
> > > > +static bool check_range_args(u64 start, u64 nr_pages, u64 *size)
> > > > +{
> > > > + if (check_mul_overflow(nr_pages, PAGE_SIZE, size))
> > > > + return false;
> > > > +
> > > > + return start < (start + *size);
> > >
> > > I will echo Oliver's concern on v1: you probably want to convert the
> > > boundary check to be inclusive of the end of the range. Otherwise, a
> > > range that ends at the top of the 64bit range will be represented as
> > > 0, and fail the check despite being perfectly valid.
> >
> > Do you mean allowing something like start == 0xfffffffffffff000 and size ==
> > 4096?
>
> Yes, this is what I was alluding to on v1.
>
> > But I guess that would still put all the following checks using "addr + size" at
> > risk. Also, I believe even the code in pgtable.c wouldn't support a such range
> > as it is also using a u64 end boundary.
>
> I'm not sure I follow. Ranges are pretty commonly expressed as a range
> terminated by an exclusive value. This just hasn't been an issue yet as
> the page table code is only ever dealing with TTBR0 or VTTBR
> translations.
If I do exclude the end boundary, evading checks would be as simple as making
sure we overflow the end boundary?
e.g. __pkvm_host_share_guest(phys = 0xfffffffffffff000, size = 4096)
check_range_allowed_memory(phys, phys + size) /* nop */
....
for_each_hyp_page(page, phys, size) { /* nop */
...
}
...
/* Install a valid mapping to phys */
kvm_pgtable_stage2_map(&vm->pgt, ipa, size, phys, ...)
>
> Anyway, I'd rather these range checks have as few assumptions of the
> applied address space as possible.
>
> Thanks,
> Oliver
>
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>
On Tue, 23 Sep 2025 10:18:59 +0100,
Vincent Donnefort <vdonnefort@google.com> wrote:
>
> On Mon, Sep 22, 2025 at 04:33:24PM -0700, Oliver Upton wrote:
> > On Mon, Sep 22, 2025 at 10:00:07PM +0100, Vincent Donnefort wrote:
> > > On Sun, Sep 21, 2025 at 12:29:08PM +0100, Marc Zyngier wrote:
> > > > On Fri, 19 Sep 2025 16:50:56 +0100,
> > > > Vincent Donnefort <vdonnefort@google.com> wrote:
> > > > >
> > > > > There's currently no verification for host issued ranges in most of the
> > > > > pKVM memory transitions. The subsequent end boundary might therefore be
> > > > > subject to overflow and could evade the later checks.
> > > > >
> > > > > Close this loophole with an additional check_range_args() check on a per
> > > > > public function basis.
> > > > >
> > > > > host_unshare_guest transition is already protected via
> > > > > __check_host_shared_guest(), while assert_host_shared_guest() callers
> > > > > are already ignoring host checks.
> > > > >
> > > > > Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> > > > >
> > > > > ---
> > > > >
> > > > > v1 -> v2:
> > > > > - Also check for (nr_pages * PAGE_SIZE) overflow. (Quentin)
> > > > > - Rename to check_range_args().
> > > > >
> > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > > > > index 8957734d6183..65fcd2148f59 100644
> > > > > --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > > > > +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > > > > @@ -712,6 +712,14 @@ static int __guest_check_page_state_range(struct pkvm_hyp_vm *vm, u64 addr,
> > > > > return check_page_state_range(&vm->pgt, addr, size, &d);
> > > > > }
> > > > >
> > > > > +static bool check_range_args(u64 start, u64 nr_pages, u64 *size)
> > > > > +{
> > > > > + if (check_mul_overflow(nr_pages, PAGE_SIZE, size))
> > > > > + return false;
> > > > > +
> > > > > + return start < (start + *size);
> > > >
> > > > I will echo Oliver's concern on v1: you probably want to convert the
> > > > boundary check to be inclusive of the end of the range. Otherwise, a
> > > > range that ends at the top of the 64bit range will be represented as
> > > > 0, and fail the check despite being perfectly valid.
> > >
> > > Do you mean allowing something like start == 0xfffffffffffff000 and size ==
> > > 4096?
> >
> > Yes, this is what I was alluding to on v1.
> >
> > > But I guess that would still put all the following checks using "addr + size" at
> > > risk. Also, I believe even the code in pgtable.c wouldn't support a such range
> > > as it is also using a u64 end boundary.
> >
> > I'm not sure I follow. Ranges are pretty commonly expressed as a range
> > terminated by an exclusive value. This just hasn't been an issue yet as
> > the page table code is only ever dealing with TTBR0 or VTTBR
> > translations.
>
> If I do exclude the end boundary, evading checks would be as simple as making
> sure we overflow the end boundary?
>
> e.g. __pkvm_host_share_guest(phys = 0xfffffffffffff000, size = 4096)
>
> check_range_allowed_memory(phys, phys + size) /* nop */
> ....
> for_each_hyp_page(page, phys, size) { /* nop */
> ...
> }
> ...
> /* Install a valid mapping to phys */
> kvm_pgtable_stage2_map(&vm->pgt, ipa, size, phys, ...)
Why shouldn't this be as simple as this:
static bool check_range_args(u64 start, u64 nr_pages, u64 *size)
{
if (check_mul_overflow(nr_pages, PAGE_SIZE, size))
return false;
return start < (start + *size - 1);
}
which correctly deals with the boundary issue?
M.
--
Without deviation from the norm, progress is not possible.
© 2016 - 2025 Red Hat, Inc.