pdx: cast PAGE_SIZE value ahead of shifting

[PATCH] pdx: cast PAGE_SIZE value ahead of shifting

Posted by Roger Pau Monne 2 months, 2 weeks ago

Coverity complains that:

277    ma_va_bottom_mask   = (PAGE_SIZE << bottom_shift) - 1;
 In expression 0x1000 << bottom_shift, left shifting by more than 31 bits
 has undefined behavior. The shift amount, bottom_shift, is as much as 63.

Cast PAGE_SIZE to paddr_t so it has the right width.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Coverity ID: 1662707
Fixes: bac2000063ba ('x86-64: reduce range spanned by 1:1 mapping and frame table indexes')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/pdx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/pdx.c b/xen/common/pdx.c
index 7e070ff962e8..5f54dd18f90d 100644
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -288,7 +288,7 @@ bool __init pfn_pdx_compression_setup(paddr_t base)
 
     pfn_pdx_hole_shift  = hole_shift;
     pfn_pdx_bottom_mask = (1UL << bottom_shift) - 1;
-    ma_va_bottom_mask   = (PAGE_SIZE << bottom_shift) - 1;
+    ma_va_bottom_mask   = ((paddr_t)PAGE_SIZE << bottom_shift) - 1;
     pfn_hole_mask       = ((1UL << hole_shift) - 1) << bottom_shift;
     pfn_top_mask        = ~(pfn_pdx_bottom_mask | pfn_hole_mask);
     ma_top_mask         = pfn_top_mask << PAGE_SHIFT;
-- 
2.49.0

Re: [PATCH] pdx: cast PAGE_SIZE value ahead of shifting

Posted by Jan Beulich 2 months, 2 weeks ago

On 13.08.2025 14:55, Roger Pau Monne wrote:
> --- a/xen/common/pdx.c
> +++ b/xen/common/pdx.c
> @@ -288,7 +288,7 @@ bool __init pfn_pdx_compression_setup(paddr_t base)
>  
>      pfn_pdx_hole_shift  = hole_shift;
>      pfn_pdx_bottom_mask = (1UL << bottom_shift) - 1;
> -    ma_va_bottom_mask   = (PAGE_SIZE << bottom_shift) - 1;
> +    ma_va_bottom_mask   = ((paddr_t)PAGE_SIZE << bottom_shift) - 1;

Given

#define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)

this shouldn't be needed, except maybe for Arm32. There, however, ...

>      pfn_hole_mask       = ((1UL << hole_shift) - 1) << bottom_shift;

... this and the shift immediately ahead would also be a problem afaict,
which makes me conclude this isn't what Coverity has looked at. I expect
the problem is with the toolstack side definition of PAGE_SIZE, which imo
would rather be addressed there. (And yes, I'm pretty averse to arbitrary
casts like this being introduced.)

Jan

Re: [PATCH] pdx: cast PAGE_SIZE value ahead of shifting

Posted by Roger Pau Monné 2 months, 2 weeks ago

On Thu, Aug 14, 2025 at 09:18:45AM +0200, Jan Beulich wrote:
> On 13.08.2025 14:55, Roger Pau Monne wrote:
> > --- a/xen/common/pdx.c
> > +++ b/xen/common/pdx.c
> > @@ -288,7 +288,7 @@ bool __init pfn_pdx_compression_setup(paddr_t base)
> >  
> >      pfn_pdx_hole_shift  = hole_shift;
> >      pfn_pdx_bottom_mask = (1UL << bottom_shift) - 1;
> > -    ma_va_bottom_mask   = (PAGE_SIZE << bottom_shift) - 1;
> > +    ma_va_bottom_mask   = ((paddr_t)PAGE_SIZE << bottom_shift) - 1;
> 
> Given
> 
> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
> 
> this shouldn't be needed, except maybe for Arm32. There, however, ...
> 
> >      pfn_hole_mask       = ((1UL << hole_shift) - 1) << bottom_shift;
> 
> ... this and the shift immediately ahead would also be a problem afaict,
> which makes me conclude this isn't what Coverity has looked at. I expect
> the problem is with the toolstack side definition of PAGE_SIZE, which imo
> would rather be addressed there. (And yes, I'm pretty averse to arbitrary
> casts like this being introduced.)

As I've realized while looking at this, wouldn't ma_va_bottom_mask
also better be of type paddr_t, since it's not operating on pfns, but
physical addresses.  I didn't adjust the type of ma_va_bottom_mask,
but I would be happy to do it if you agree.

Thanks, roger.

Re: [PATCH] pdx: cast PAGE_SIZE value ahead of shifting

Posted by Jan Beulich 2 months, 2 weeks ago

On 14.08.2025 12:28, Roger Pau Monné wrote:
> On Thu, Aug 14, 2025 at 09:18:45AM +0200, Jan Beulich wrote:
>> On 13.08.2025 14:55, Roger Pau Monne wrote:
>>> --- a/xen/common/pdx.c
>>> +++ b/xen/common/pdx.c
>>> @@ -288,7 +288,7 @@ bool __init pfn_pdx_compression_setup(paddr_t base)
>>>  
>>>      pfn_pdx_hole_shift  = hole_shift;
>>>      pfn_pdx_bottom_mask = (1UL << bottom_shift) - 1;
>>> -    ma_va_bottom_mask   = (PAGE_SIZE << bottom_shift) - 1;
>>> +    ma_va_bottom_mask   = ((paddr_t)PAGE_SIZE << bottom_shift) - 1;
>>
>> Given
>>
>> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
>>
>> this shouldn't be needed, except maybe for Arm32. There, however, ...
>>
>>>      pfn_hole_mask       = ((1UL << hole_shift) - 1) << bottom_shift;
>>
>> ... this and the shift immediately ahead would also be a problem afaict,
>> which makes me conclude this isn't what Coverity has looked at. I expect
>> the problem is with the toolstack side definition of PAGE_SIZE, which imo
>> would rather be addressed there. (And yes, I'm pretty averse to arbitrary
>> casts like this being introduced.)
> 
> As I've realized while looking at this, wouldn't ma_va_bottom_mask
> also better be of type paddr_t, since it's not operating on pfns, but
> physical addresses.  I didn't adjust the type of ma_va_bottom_mask,
> but I would be happy to do it if you agree.

No, as its name says it's also used on virtual addresses (really: offsets
into the direct map). It hence would better not have any bits set outside
of the range that VAs can cover. With that, imo the cast (if any) also
should have been to unsigned long, not paddr_t. Yet as said, im the cast
would better not be there in the first place. Just that meanwhile I've
learned that this was committed already.

Jan

Re: [PATCH] pdx: cast PAGE_SIZE value ahead of shifting

Posted by Roger Pau Monné 2 months, 2 weeks ago

On Thu, Aug 14, 2025 at 12:45:40PM +0200, Jan Beulich wrote:
> On 14.08.2025 12:28, Roger Pau Monné wrote:
> > On Thu, Aug 14, 2025 at 09:18:45AM +0200, Jan Beulich wrote:
> >> On 13.08.2025 14:55, Roger Pau Monne wrote:
> >>> --- a/xen/common/pdx.c
> >>> +++ b/xen/common/pdx.c
> >>> @@ -288,7 +288,7 @@ bool __init pfn_pdx_compression_setup(paddr_t base)
> >>>  
> >>>      pfn_pdx_hole_shift  = hole_shift;
> >>>      pfn_pdx_bottom_mask = (1UL << bottom_shift) - 1;
> >>> -    ma_va_bottom_mask   = (PAGE_SIZE << bottom_shift) - 1;
> >>> +    ma_va_bottom_mask   = ((paddr_t)PAGE_SIZE << bottom_shift) - 1;
> >>
> >> Given
> >>
> >> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
> >>
> >> this shouldn't be needed, except maybe for Arm32. There, however, ...
> >>
> >>>      pfn_hole_mask       = ((1UL << hole_shift) - 1) << bottom_shift;
> >>
> >> ... this and the shift immediately ahead would also be a problem afaict,
> >> which makes me conclude this isn't what Coverity has looked at. I expect
> >> the problem is with the toolstack side definition of PAGE_SIZE, which imo
> >> would rather be addressed there. (And yes, I'm pretty averse to arbitrary
> >> casts like this being introduced.)
> > 
> > As I've realized while looking at this, wouldn't ma_va_bottom_mask
> > also better be of type paddr_t, since it's not operating on pfns, but
> > physical addresses.  I didn't adjust the type of ma_va_bottom_mask,
> > but I would be happy to do it if you agree.
> 
> No, as its name says it's also used on virtual addresses (really: offsets
> into the direct map). It hence would better not have any bits set outside
> of the range that VAs can cover.

It's confusing that it's sometimes used against a paddr_t or an
unsigned long type.  The logic itself already limits the shift so it's
below the width of unsigned long AFAICT.

> With that, imo the cast (if any) also
> should have been to unsigned long, not paddr_t. Yet as said, im the cast
> would better not be there in the first place. Just that meanwhile I've
> learned that this was committed already.

Sorry, I should have waited for your opinion.

I think you would prefer the patch below.  I can send this formally,
not sure whether you would prefer a formal revert of the previous
patch, plus the new fix applied, or doing the revert in the new patc
(like below) is fine.

Thanks, Roger.
---
diff --git a/tools/tests/pdx/harness.h b/tools/tests/pdx/harness.h
index 5bef7df650d2..a0fe33b4f1e0 100644
--- a/tools/tests/pdx/harness.h
+++ b/tools/tests/pdx/harness.h
@@ -33,7 +33,7 @@
 #define PAGE_SHIFT    12
 /* Some libcs define PAGE_SIZE in limits.h. */
 #undef  PAGE_SIZE
-#define PAGE_SIZE     (1 << PAGE_SHIFT)
+#define PAGE_SIZE     (1UL << PAGE_SHIFT)
 #define MAX_ORDER     18 /* 2 * PAGETABLE_ORDER (9) */
 
 #define PFN_DOWN(x)   ((x) >> PAGE_SHIFT)
diff --git a/xen/common/pdx.c b/xen/common/pdx.c
index 06536cc639f3..9e6b36086fbd 100644
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -274,7 +274,7 @@ bool __init pfn_pdx_compression_setup(paddr_t base)
 
     pfn_pdx_hole_shift  = hole_shift;
     pfn_pdx_bottom_mask = (1UL << bottom_shift) - 1;
-    ma_va_bottom_mask   = ((paddr_t)PAGE_SIZE << bottom_shift) - 1;
+    ma_va_bottom_mask   = (PAGE_SIZE << bottom_shift) - 1;
     pfn_hole_mask       = ((1UL << hole_shift) - 1) << bottom_shift;
     pfn_top_mask        = ~(pfn_pdx_bottom_mask | pfn_hole_mask);
     ma_top_mask         = pfn_top_mask << PAGE_SHIFT;

Re: [PATCH] pdx: cast PAGE_SIZE value ahead of shifting

Posted by Andrew Cooper 2 months, 2 weeks ago

On 14/08/2025 2:01 pm, Roger Pau Monné wrote:
> On Thu, Aug 14, 2025 at 12:45:40PM +0200, Jan Beulich wrote:
>> On 14.08.2025 12:28, Roger Pau Monné wrote:
>>> On Thu, Aug 14, 2025 at 09:18:45AM +0200, Jan Beulich wrote:
> diff --git a/tools/tests/pdx/harness.h b/tools/tests/pdx/harness.h
> index 5bef7df650d2..a0fe33b4f1e0 100644
> --- a/tools/tests/pdx/harness.h
> +++ b/tools/tests/pdx/harness.h
> @@ -33,7 +33,7 @@
>  #define PAGE_SHIFT    12
>  /* Some libcs define PAGE_SIZE in limits.h. */
>  #undef  PAGE_SIZE
> -#define PAGE_SIZE     (1 << PAGE_SHIFT)
> +#define PAGE_SIZE     (1UL << PAGE_SHIFT)

It should be 1L to match Xen.

~Andrew

Re: [PATCH] pdx: cast PAGE_SIZE value ahead of shifting

Posted by Jan Beulich 2 months, 2 weeks ago

On 14.08.2025 15:01, Roger Pau Monné wrote:
> On Thu, Aug 14, 2025 at 12:45:40PM +0200, Jan Beulich wrote:
>> On 14.08.2025 12:28, Roger Pau Monné wrote:
>>> On Thu, Aug 14, 2025 at 09:18:45AM +0200, Jan Beulich wrote:
>>>> On 13.08.2025 14:55, Roger Pau Monne wrote:
>>>>> --- a/xen/common/pdx.c
>>>>> +++ b/xen/common/pdx.c
>>>>> @@ -288,7 +288,7 @@ bool __init pfn_pdx_compression_setup(paddr_t base)
>>>>>  
>>>>>      pfn_pdx_hole_shift  = hole_shift;
>>>>>      pfn_pdx_bottom_mask = (1UL << bottom_shift) - 1;
>>>>> -    ma_va_bottom_mask   = (PAGE_SIZE << bottom_shift) - 1;
>>>>> +    ma_va_bottom_mask   = ((paddr_t)PAGE_SIZE << bottom_shift) - 1;
>>>>
>>>> Given
>>>>
>>>> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
>>>>
>>>> this shouldn't be needed, except maybe for Arm32. There, however, ...
>>>>
>>>>>      pfn_hole_mask       = ((1UL << hole_shift) - 1) << bottom_shift;
>>>>
>>>> ... this and the shift immediately ahead would also be a problem afaict,
>>>> which makes me conclude this isn't what Coverity has looked at. I expect
>>>> the problem is with the toolstack side definition of PAGE_SIZE, which imo
>>>> would rather be addressed there. (And yes, I'm pretty averse to arbitrary
>>>> casts like this being introduced.)
>>>
>>> As I've realized while looking at this, wouldn't ma_va_bottom_mask
>>> also better be of type paddr_t, since it's not operating on pfns, but
>>> physical addresses.  I didn't adjust the type of ma_va_bottom_mask,
>>> but I would be happy to do it if you agree.
>>
>> No, as its name says it's also used on virtual addresses (really: offsets
>> into the direct map). It hence would better not have any bits set outside
>> of the range that VAs can cover.
> 
> It's confusing that it's sometimes used against a paddr_t or an
> unsigned long type.  The logic itself already limits the shift so it's
> below the width of unsigned long AFAICT.

Well, the variable simply doesn't need to be wider than the narrowest type
it's used with.

>> With that, imo the cast (if any) also
>> should have been to unsigned long, not paddr_t. Yet as said, im the cast
>> would better not be there in the first place. Just that meanwhile I've
>> learned that this was committed already.
> 
> Sorry, I should have waited for your opinion.
> 
> I think you would prefer the patch below.

Yes.

>  I can send this formally,
> not sure whether you would prefer a formal revert of the previous
> patch, plus the new fix applied, or doing the revert in the new patc
> (like below) is fine.

Acked-by: Jan Beulich <jbeulich@suse.com>

I don't see a strong need for an outright revert.

Jan

Re: [PATCH] pdx: cast PAGE_SIZE value ahead of shifting

Posted by Roger Pau Monné 2 months, 2 weeks ago

On Thu, Aug 14, 2025 at 03:15:26PM +0200, Jan Beulich wrote:
> On 14.08.2025 15:01, Roger Pau Monné wrote:
> > On Thu, Aug 14, 2025 at 12:45:40PM +0200, Jan Beulich wrote:
> >> On 14.08.2025 12:28, Roger Pau Monné wrote:
> >>> On Thu, Aug 14, 2025 at 09:18:45AM +0200, Jan Beulich wrote:
> >>>> On 13.08.2025 14:55, Roger Pau Monne wrote:
> >>>>> --- a/xen/common/pdx.c
> >>>>> +++ b/xen/common/pdx.c
> >>>>> @@ -288,7 +288,7 @@ bool __init pfn_pdx_compression_setup(paddr_t base)
> >>>>>  
> >>>>>      pfn_pdx_hole_shift  = hole_shift;
> >>>>>      pfn_pdx_bottom_mask = (1UL << bottom_shift) - 1;
> >>>>> -    ma_va_bottom_mask   = (PAGE_SIZE << bottom_shift) - 1;
> >>>>> +    ma_va_bottom_mask   = ((paddr_t)PAGE_SIZE << bottom_shift) - 1;
> >>>>
> >>>> Given
> >>>>
> >>>> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
> >>>>
> >>>> this shouldn't be needed, except maybe for Arm32. There, however, ...
> >>>>
> >>>>>      pfn_hole_mask       = ((1UL << hole_shift) - 1) << bottom_shift;
> >>>>
> >>>> ... this and the shift immediately ahead would also be a problem afaict,
> >>>> which makes me conclude this isn't what Coverity has looked at. I expect
> >>>> the problem is with the toolstack side definition of PAGE_SIZE, which imo
> >>>> would rather be addressed there. (And yes, I'm pretty averse to arbitrary
> >>>> casts like this being introduced.)
> >>>
> >>> As I've realized while looking at this, wouldn't ma_va_bottom_mask
> >>> also better be of type paddr_t, since it's not operating on pfns, but
> >>> physical addresses.  I didn't adjust the type of ma_va_bottom_mask,
> >>> but I would be happy to do it if you agree.
> >>
> >> No, as its name says it's also used on virtual addresses (really: offsets
> >> into the direct map). It hence would better not have any bits set outside
> >> of the range that VAs can cover.
> > 
> > It's confusing that it's sometimes used against a paddr_t or an
> > unsigned long type.  The logic itself already limits the shift so it's
> > below the width of unsigned long AFAICT.
> 
> Well, the variable simply doesn't need to be wider than the narrowest type
> it's used with.
> 
> >> With that, imo the cast (if any) also
> >> should have been to unsigned long, not paddr_t. Yet as said, im the cast
> >> would better not be there in the first place. Just that meanwhile I've
> >> learned that this was committed already.
> > 
> > Sorry, I should have waited for your opinion.
> > 
> > I think you would prefer the patch below.
> 
> Yes.
> 
> >  I can send this formally,
> > not sure whether you would prefer a formal revert of the previous
> > patch, plus the new fix applied, or doing the revert in the new patc
> > (like below) is fine.
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> I don't see a strong need for an outright revert.

I've adjusted UL -> L as requested by Andrew, and added the following
commit message:

tests/pdx: define PAGE_SIZE as long

Otherwise Coverity complains about possibly shifting an integer more than
31 bits.

This also reverts the previous attempt to fix this Coverity reported
issue, commit 4dd323029094d93dbc8d174fe744fd7f54f0a7a4.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Coverity ID: 1662707
Fixes: cb50e4033717 ('test/pdx: add PDX compression unit tests')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

Let me know if you are OK with the adjustment and commit message.

Thanks, Roger.

Re: [PATCH] pdx: cast PAGE_SIZE value ahead of shifting

Posted by Jan Beulich 2 months, 2 weeks ago

On 14.08.2025 16:57, Roger Pau Monné wrote:
> I've adjusted UL -> L as requested by Andrew, and added the following
> commit message:
> 
> tests/pdx: define PAGE_SIZE as long
> 
> Otherwise Coverity complains about possibly shifting an integer more than
> 31 bits.
> 
> This also reverts the previous attempt to fix this Coverity reported
> issue, commit 4dd323029094d93dbc8d174fe744fd7f54f0a7a4.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Coverity ID: 1662707
> Fixes: cb50e4033717 ('test/pdx: add PDX compression unit tests')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Let me know if you are OK with the adjustment and commit message.

Fine with me.

Jan

Re: [PATCH] pdx: cast PAGE_SIZE value ahead of shifting

Posted by Andrew Cooper 2 months, 2 weeks ago

On 13/08/2025 1:55 pm, Roger Pau Monne wrote:
> Coverity complains that:
>
> 277    ma_va_bottom_mask   = (PAGE_SIZE << bottom_shift) - 1;

I'd perhaps drop this line from the commit message.  The sentence below
is fine, and there's only a single hunk so it's obvious what is being
referred to.

>  In expression 0x1000 << bottom_shift, left shifting by more than 31 bits
>  has undefined behavior. The shift amount, bottom_shift, is as much as 63.
>
> Cast PAGE_SIZE to paddr_t so it has the right width.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Coverity ID: 1662707
> Fixes: bac2000063ba ('x86-64: reduce range spanned by 1:1 mapping and frame table indexes')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Formatting aside, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>