Re: [PATCH] xen/arm: Harden setup_frametable_mappings

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 17/01/2023 10:29, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> On 16/01/2023 14:41, Michal Orzel wrote:
>> The amount of supported physical memory depends on the frametable size
>> and the number of struct page_info entries that can fit into it. Define
>> a macro PAGE_INFO_SIZE to store the current size of the struct page_info
>> (i.e. 56B for arm64 and 32B for arm32) and add a sanity check in
>> setup_frametable_mappings to be notified whenever the size of the
>> structure changes. Also call a panic if the calculated frametable_size
>> exceeds the limit defined by FRAMETABLE_SIZE macro.
>>
>> Update the comments regarding the frametable in asm/config.h and take
>> the opportunity to remove unused macro FRAMETABLE_VIRT_END on arm32.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>>   xen/arch/arm/include/asm/config.h |  5 ++---
>>   xen/arch/arm/include/asm/mm.h     | 11 +++++++++++
>>   xen/arch/arm/mm.c                 |  5 +++++
>>   3 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/config.h 
>> b/xen/arch/arm/include/asm/config.h
>> index 16213c8b677f..d8f99776986f 100644
>> --- a/xen/arch/arm/include/asm/config.h
>> +++ b/xen/arch/arm/include/asm/config.h
>> @@ -82,7 +82,7 @@
>>    * ARM32 layout:
>>    *   0  -  12M   <COMMON>
>>    *
>> - *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
>> + *  32M - 128M   Frametable: 32 bytes per page for 12GB of RAM
>>    * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
>>    *                    space
>>    *
>> @@ -95,7 +95,7 @@
>>    *
>>    *   1G -   2G   VMAP: ioremap and early_ioremap
>>    *
>> - *  32G -  64G   Frametable: 24 bytes per page for 5.3TB of RAM
>> + *  32G -  64G   Frametable: 56 bytes per page for 2TB of RAM
>>    *
>>    * 0x0000008000000000 - 0x00007fffffffffff (127.5TB, L0 slots [1..255])
>>    *  Unused
>> @@ -127,7 +127,6 @@
>>   #define FRAMETABLE_VIRT_START  _AT(vaddr_t,0x02000000)
>>   #define FRAMETABLE_SIZE        MB(128-32)
>>   #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
>> -#define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
> 
> This is somewhat unrelated to the goal of the patch. We do clean-up in
> the same patch, but they tend to be in the same of already modified hunk
> (which is not the case here).
> 
> So I would prefer if this is split. This would make this patch a
> potential candidate for backport.
Just for clarity. Do you mean to separate all the config.h changes or only
the FRAMETABLE_VIRT_END removal? I guess the former.

> 
>>
>>   #define VMAP_VIRT_START        _AT(vaddr_t,0x10000000)
>>   #define VMAP_VIRT_SIZE         _AT(vaddr_t, GB(1) - MB(256))
>> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
>> index 68adcac9fa8d..23dec574eb31 100644
>> --- a/xen/arch/arm/include/asm/mm.h
>> +++ b/xen/arch/arm/include/asm/mm.h
>> @@ -26,6 +26,17 @@
>>    */
>>   #define PFN_ORDER(_pfn) ((_pfn)->v.free.order)
>>
>> +/*
>> + * The size of struct page_info impacts the number of entries that can fit
>> + * into the frametable area and thus it affects the amount of physical 
>> memory
>> + * we claim to support. Define PAGE_INFO_SIZE to be used for sanity 
>> checking.
>> +*/
>> +#ifdef CONFIG_ARM_64
>> +#define PAGE_INFO_SIZE 56
>> +#else
>> +#define PAGE_INFO_SIZE 32
>> +#endif
>> +
>>   struct page_info
>>   {
>>       /* Each frame can be threaded onto a doubly-linked list. */
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 0fc6f2992dd1..a8c28fa5b768 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -676,6 +676,11 @@ void __init setup_frametable_mappings(paddr_t ps, 
>> paddr_t pe)
>>       const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : 
>> MB(32);
>>       int rc;
>>
>> +    BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE);
>> +
>> +    if ( frametable_size > FRAMETABLE_SIZE )
>> +        panic("RAM size is too big to fit in a frametable area\n");
> 
> This is not correct. Depending on the PDX compression, the frametable
> may end up to cover non-RAM. So I would write:
> 
> "The cannot frametable cannot cover the physical region 0x%PRIpaddr -
> 0x%PRIpaddr".
Yes, you're right.

> 
>> +
>>       frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps));
>>       /* Round up to 2M or 32M boundary, as appropriate. */
>>       frametable_size = ROUNDUP(frametable_size, mapping_size);
> 
> Cheers,
> 
> --
> Julien Grall

~Michal