| Message ID | 20221201181145.45279-1-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN,v2] Xen: Ensure "xenheap_bits - PAGE_SHIFT" can be used to shift "unsigned long" | expand |
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
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
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
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); }
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(-)