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

[Jan Beulich <jbeulich@xxxxxxxx>]

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