[XEN v2] Xen: Ensure "xenheap_bits - PAGE_SHIFT" can be used to shift "unsigned long"

[XEN v2] Xen: Ensure "xenheap_bits - PAGE_SHIFT" can be used to shift "unsigned long"

[Ayan Kumar Halder]

Machine frame number (mfn) is used to represent the hardware page address.
This is an unsigned long variable. We need to check if it can hold the complete
range of hardware page addresses. To ensure this we check that the count of bits
represented by 'unsigned long' added to the bit index of page size, should be
less than the count of bits required to represent the maximum physical address.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

Currently this change will not have any impact on the existing architectures.
The following table illustrates PADDR_BITS vs BITS_PER_LONG of different archs

------------------------------------------------
| Arch      |   PADDR_BITS    |   BITS_PER_LONG |
------------------------------------------------
| Arm_64    |   48            |   64            |
| Arm_32    |   40            |   32            |
| RISCV_64  |   Don't know    |   64            |
| x86       |   52            |   64            |
-------------------------------------------------

However, this will change when we introduce a platform (For eg Cortex-R52) which
supports 32 bit physical address and BITS_PER_LONG. This platform does not follow
the same code path as Arm_32.
Thus, I have introduced this change as I don't see it causing a regression on
any of the supported platforms.

Changes from v1:-
1. Changed the check from "(PADDR_BITS > BITS_PER_LONG)" to "((PADDR_BITS - PAGE_SHIFT) >= BITS_PER_LONG)"
2. Updated the commit message to explain the reason for this.

 xen/common/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 62afb07bc6..c5b8c7444f 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2245,7 +2245,7 @@ void __init xenheap_max_mfn(unsigned long mfn)
 {
     ASSERT(!first_node_initialised);
     ASSERT(!xenheap_bits);
-    BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
+    BUILD_BUG_ON((PADDR_BITS - PAGE_SHIFT) >= BITS_PER_LONG);
     xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
     printk(XENLOG_INFO "Xen heap: %u bits\n", xenheap_bits);
 }
-- 
2.17.1

Re: [XEN v2] Xen: Ensure "xenheap_bits - PAGE_SHIFT" can be used to shift "unsigned long"

[Jan Beulich]

On 01.12.2022 19:11, Ayan Kumar Halder wrote:
> Machine frame number (mfn) is used to represent the hardware page address.
> This is an unsigned long variable. We need to check if it can hold the complete
> range of hardware page addresses. To ensure this we check that the count of bits
> represented by 'unsigned long' added to the bit index of page size, should be
> less than the count of bits required to represent the maximum physical address.

I'm afraid I can't connect the description with ...

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2245,7 +2245,7 @@ void __init xenheap_max_mfn(unsigned long mfn)
>  {
>      ASSERT(!first_node_initialised);
>      ASSERT(!xenheap_bits);
> -    BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
> +    BUILD_BUG_ON((PADDR_BITS - PAGE_SHIFT) >= BITS_PER_LONG);

... the actual change made. Julien, when replying to v1, already gave
a clear hint what is relevant: The use of (xenheap_bits - PAGE_SHIFT)
in right hand operands of shift operators. As relevant is of course
the absence of uses directly as shift counts, which otherwise could
still be UB (and which iirc is why the adjustment by PAGE_SHIFT was
left out in the original check).

Jan

Re: [XEN v2] Xen: Ensure "xenheap_bits - PAGE_SHIFT" can be used to shift "unsigned long"

[Ayan Kumar Halder]

Hi Jan,

On 02/12/2022 08:31, Jan Beulich wrote:
> On 01.12.2022 19:11, Ayan Kumar Halder wrote:
>> Machine frame number (mfn) is used to represent the hardware page address.
>> This is an unsigned long variable. We need to check if it can hold the complete
>> range of hardware page addresses. To ensure this we check that the count of bits
>> represented by 'unsigned long' added to the bit index of page size, should be
>> less than the count of bits required to represent the maximum physical address.
> I'm afraid I can't connect the description with ...
>
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -2245,7 +2245,7 @@ void __init xenheap_max_mfn(unsigned long mfn)
>>   {
>>       ASSERT(!first_node_initialised);
>>       ASSERT(!xenheap_bits);
>> -    BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
>> +    BUILD_BUG_ON((PADDR_BITS - PAGE_SHIFT) >= BITS_PER_LONG);
> ... the actual change made. Julien, when replying to v1, already gave
> a clear hint what is relevant: The use of (xenheap_bits - PAGE_SHIFT)
> in right hand operands of shift operators. As relevant is of course
> the absence of uses directly as shift counts, which otherwise could
> still be UB (and which iirc is why the adjustment by PAGE_SHIFT was
> left out in the original check).

I could see the following uses of xenheap_bits in page_alloc.c

1. init_node_heap()

               (!xenheap_bits ||
                !((mfn + nr - 1) >> (xenheap_bits - PAGE_SHIFT))) )

                (!xenheap_bits ||
                !((mfn + needed - 1) >> (xenheap_bits - PAGE_SHIFT))) )

2. In alloc_xenheap_pages()

     if ( xenheap_bits && (memflags >> _MEMF_bits) > xenheap_bits )
         memflags &= ~MEMF_bits(~0U);
     if ( !(memflags >> _MEMF_bits) )
         memflags |= MEMF_bits(xenheap_bits);

 From what I see, whenever "xenheap_bits" is used as a right hand 
operand of shift operator, it is always used as "(xenheap_bits - 
PAGE_SHIFT)".

So, is it correct to say this :-

Ensure (xenheap_bits - PAGE_SHIFT) can be used as a rhs operand of a 
shift operator

We want to ensure that "xenheap_bits - PAGE_SHIFT" is strictly less than 
the number of bits to represent unsigned long as it is used a rhs 
operand to shift mfn.

- Ayan

>
> Jan

Re: [XEN v2] Xen: Ensure "xenheap_bits - PAGE_SHIFT" can be used to shift "unsigned long"

[Jan Beulich]

On 02.12.2022 10:30, Ayan Kumar Halder wrote:
> Hi Jan,
> 
> On 02/12/2022 08:31, Jan Beulich wrote:
>> On 01.12.2022 19:11, Ayan Kumar Halder wrote:
>>> Machine frame number (mfn) is used to represent the hardware page address.
>>> This is an unsigned long variable. We need to check if it can hold the complete
>>> range of hardware page addresses. To ensure this we check that the count of bits
>>> represented by 'unsigned long' added to the bit index of page size, should be
>>> less than the count of bits required to represent the maximum physical address.
>> I'm afraid I can't connect the description with ...
>>
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -2245,7 +2245,7 @@ void __init xenheap_max_mfn(unsigned long mfn)
>>>   {
>>>       ASSERT(!first_node_initialised);
>>>       ASSERT(!xenheap_bits);
>>> -    BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
>>> +    BUILD_BUG_ON((PADDR_BITS - PAGE_SHIFT) >= BITS_PER_LONG);
>> ... the actual change made. Julien, when replying to v1, already gave
>> a clear hint what is relevant: The use of (xenheap_bits - PAGE_SHIFT)
>> in right hand operands of shift operators. As relevant is of course
>> the absence of uses directly as shift counts, which otherwise could
>> still be UB (and which iirc is why the adjustment by PAGE_SHIFT was
>> left out in the original check).
> 
> I could see the following uses of xenheap_bits in page_alloc.c
> 
> 1. init_node_heap()
> 
>                (!xenheap_bits ||
>                 !((mfn + nr - 1) >> (xenheap_bits - PAGE_SHIFT))) )
> 
>                 (!xenheap_bits ||
>                 !((mfn + needed - 1) >> (xenheap_bits - PAGE_SHIFT))) )
> 
> 2. In alloc_xenheap_pages()
> 
>      if ( xenheap_bits && (memflags >> _MEMF_bits) > xenheap_bits )
>          memflags &= ~MEMF_bits(~0U);
>      if ( !(memflags >> _MEMF_bits) )
>          memflags |= MEMF_bits(xenheap_bits);
> 
>  From what I see, whenever "xenheap_bits" is used as a right hand 
> operand of shift operator, it is always used as "(xenheap_bits - 
> PAGE_SHIFT)".
> 
> So, is it correct to say this :-
> 
> Ensure (xenheap_bits - PAGE_SHIFT) can be used as a rhs operand of a 
> shift operator
> 
> We want to ensure that "xenheap_bits - PAGE_SHIFT" is strictly less than 
> the number of bits to represent unsigned long as it is used a rhs 
> operand to shift mfn.

Yes. Plus, as said, it is also important to note that the value is never
used to shift an address (rather than a frame number), and going forward
then also shouldn't be (perhaps unless further precautions are taken).

Jan