mfn_valid() granularity is (currently) 256Mb. Therefore the start of a
1Gb page passing the test doesn't necessarily mean all parts of such a
range would also pass. Yet using the result of mfn_to_page() on an MFN
which doesn't pass mfn_valid() checking is liable to result in a crash
(the invocation of mfn_to_page() alone is presumably "just" UB in such a
case).

Fixes: ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Of course we could leverage mfn_valid() granularity here to do an
increment by more than 1 if mfn_valid() returned false. Yet doing so
likely would want a suitable helper to be introduced first, rather than
open-coding such logic here.
---
v2: New.

--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -519,8 +519,12 @@ int epte_get_entry_emt(struct domain *d,
     }
 
     for ( special_pgs = i = 0; i < (1ul << order); i++ )
-        if ( is_special_page(mfn_to_page(mfn_add(mfn, i))) )
+    {
+        mfn_t cur = mfn_add(mfn, i);
+
+        if ( mfn_valid(cur) && is_special_page(mfn_to_page(cur)) )
             special_pgs++;
+    }
 
     if ( special_pgs )
     {

On Wed, 2024-06-12 at 15:16 +0200, Jan Beulich wrote:
> mfn_valid() granularity is (currently) 256Mb. Therefore the start of
> a
> 1Gb page passing the test doesn't necessarily mean all parts of such
> a
> range would also pass. Yet using the result of mfn_to_page() on an
> MFN
> which doesn't pass mfn_valid() checking is liable to result in a
> crash
> (the invocation of mfn_to_page() alone is presumably "just" UB in
> such a
> case).
> 
> Fixes: ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~ Oleksii
> ---
> Of course we could leverage mfn_valid() granularity here to do an
> increment by more than 1 if mfn_valid() returned false. Yet doing so
> likely would want a suitable helper to be introduced first, rather
> than
> open-coding such logic here.
> ---
> v2: New.
> 
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -519,8 +519,12 @@ int epte_get_entry_emt(struct domain *d,
>      }
>  
>      for ( special_pgs = i = 0; i < (1ul << order); i++ )
> -        if ( is_special_page(mfn_to_page(mfn_add(mfn, i))) )
> +    {
> +        mfn_t cur = mfn_add(mfn, i);
> +
> +        if ( mfn_valid(cur) && is_special_page(mfn_to_page(cur)) )
>              special_pgs++;
> +    }
>  
>      if ( special_pgs )
>      {
> 

On Wed, Jun 12, 2024 at 03:16:37PM +0200, Jan Beulich wrote:
> mfn_valid() granularity is (currently) 256Mb. Therefore the start of a
> 1Gb page passing the test doesn't necessarily mean all parts of such a
> range would also pass.

How would such a superpage end up in the EPT?

I would assume this can only happen when adding a superpage MMIO that
has part of it return success from mfn_valid()?

> Yet using the result of mfn_to_page() on an MFN
> which doesn't pass mfn_valid() checking is liable to result in a crash
> (the invocation of mfn_to_page() alone is presumably "just" UB in such a
> case).
> 
> Fixes: ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> Of course we could leverage mfn_valid() granularity here to do an
> increment by more than 1 if mfn_valid() returned false. Yet doing so
> likely would want a suitable helper to be introduced first, rather than
> open-coding such logic here.

We would still need to call is_special_page() on each 4K chunk, at
which point taking advantage of the mfn_valid() granularity is likely
to make the code more complicated to follow IMO.

Thanks, Roger.

On 12.06.2024 16:11, Roger Pau Monné wrote:
> On Wed, Jun 12, 2024 at 03:16:37PM +0200, Jan Beulich wrote:
>> mfn_valid() granularity is (currently) 256Mb. Therefore the start of a
>> 1Gb page passing the test doesn't necessarily mean all parts of such a
>> range would also pass.
> 
> How would such a superpage end up in the EPT?
> 
> I would assume this can only happen when adding a superpage MMIO that
> has part of it return success from mfn_valid()?

Yes, that's the only way I can think of.

>> Yet using the result of mfn_to_page() on an MFN
>> which doesn't pass mfn_valid() checking is liable to result in a crash
>> (the invocation of mfn_to_page() alone is presumably "just" UB in such a
>> case).
>>
>> Fixes: ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> Of course we could leverage mfn_valid() granularity here to do an
>> increment by more than 1 if mfn_valid() returned false. Yet doing so
>> likely would want a suitable helper to be introduced first, rather than
>> open-coding such logic here.
> 
> We would still need to call is_special_page() on each 4K chunk,

Why? Within any block for which mfn_valid() returns false, there can be
no RAM pages and hence also no special ones. It's only blocks where
mfn_valid() returns true that we'd need to iterate through page-by-page.

> at
> which point taking advantage of the mfn_valid() granularity is likely
> to make the code more complicated to follow IMO.

Right, this making it more complicated is the main counter argument. Hence
why I think that if to go such a route at all, it would need some new
helper(s) such that at the use sites things still would remain reasonably
clear.

Jan

On Wed, Jun 12, 2024 at 04:47:12PM +0200, Jan Beulich wrote:
> On 12.06.2024 16:11, Roger Pau Monné wrote:
> > On Wed, Jun 12, 2024 at 03:16:37PM +0200, Jan Beulich wrote:
> >> mfn_valid() granularity is (currently) 256Mb. Therefore the start of a
> >> 1Gb page passing the test doesn't necessarily mean all parts of such a
> >> range would also pass.
> > 
> > How would such a superpage end up in the EPT?
> > 
> > I would assume this can only happen when adding a superpage MMIO that
> > has part of it return success from mfn_valid()?
> 
> Yes, that's the only way I can think of.
> 
> >> Yet using the result of mfn_to_page() on an MFN
> >> which doesn't pass mfn_valid() checking is liable to result in a crash
> >> (the invocation of mfn_to_page() alone is presumably "just" UB in such a
> >> case).
> >>
> >> Fixes: ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> >> ---
> >> Of course we could leverage mfn_valid() granularity here to do an
> >> increment by more than 1 if mfn_valid() returned false. Yet doing so
> >> likely would want a suitable helper to be introduced first, rather than
> >> open-coding such logic here.
> > 
> > We would still need to call is_special_page() on each 4K chunk,
> 
> Why? Within any block for which mfn_valid() returns false, there can be
> no RAM pages and hence also no special ones. It's only blocks where
> mfn_valid() returns true that we'd need to iterate through page-by-page.

Oh right, I was thinking the other way around (mfn_valid() returning
true), never mind.

> > at
> > which point taking advantage of the mfn_valid() granularity is likely
> > to make the code more complicated to follow IMO.
> 
> Right, this making it more complicated is the main counter argument. Hence
> why I think that if to go such a route at all, it would need some new
> helper(s) such that at the use sites things still would remain reasonably
> clear.

We could also add an extra check to exit the loop early if special
pages have been found but don't match the current loop index, as it's
all special pages or none.

Thanks, Roger.

On 12.06.2024 17:02, Roger Pau Monné wrote:
> We could also add an extra check to exit the loop early if special
> pages have been found but don't match the current loop index, as it's
> all special pages or none.

I was actually considering to make such a change, but then concluded
that in the common case there'll be no special pages anyway, and hence
we need to run the loop to completion anyway.

Jan