Re: [PATCH] modify acquire_domstatic_pages to take an unsigned int size parameter

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Stefano,

> On 16 Sep 2021, at 21:47, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> acquire_domstatic_pages currently takes an unsigned long nr_mfns
> parameter, but actually it cannot handle anything larger than an
> unsigned int nr_mfns. That's because acquire_domstatic_pages is based on
> assign_pages which also takes an unsigned int nr parameter.
> 
> So modify the nr_mfns parameter of acquire_domstatic_pages to be
> unsigned int.
> 
> There is only one caller in
> xen/arch/arm/domain_build.c:allocate_static_memory. Check that the value
> to be passed to acquire_domstatic_pages is no larger than UINT_MAX. If
> it is, print an error and goto fail.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

Cheers
Bertrand

> ---
> 
> Jan, I took your suggestion of moving the check closer to where the
> value is read from DT. At that point I also took the opportunity to
> change acquire_domstatic_pages to take an unsigned int parameter
> instead of unsigned long.
> 
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 62ab7d0ead..d233d634c1 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -554,6 +554,12 @@ static void __init allocate_static_memory(struct domain 
> *d,
>         device_tree_get_reg(&cell, addr_cells, size_cells, &pbase, &psize);
>         ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE));
> 
> +        if ( PFN_DOWN(psize) > UINT_MAX )
> +        {
> +            printk(XENLOG_ERR "%pd: static memory size too large: 
> %#"PRIpaddr,
> +                   d, psize);
> +            goto fail;
> +        }
>         smfn = maddr_to_mfn(pbase);
>         res = acquire_domstatic_pages(d, smfn, PFN_DOWN(psize), 0);
>         if ( res )
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index b9441cb06f..b64c07ae92 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2714,7 +2714,7 @@ static struct page_info * __init 
> acquire_staticmem_pages(mfn_t smfn,
>  * then assign them to one specific domain #d.
>  */
> int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
> -                                   unsigned long nr_mfns, unsigned int 
> memflags)
> +                                   unsigned int nr_mfns, unsigned int 
> memflags)
> {
>     struct page_info *pg;
> 
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index dd49237e86..5db26ed477 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -89,7 +89,7 @@ bool scrub_free_pages(void);
> /* These functions are for static memory */
> void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
>                           bool need_scrub);
> -int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned long 
> nr_mfns,
> +int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int 
> nr_mfns,
>                             unsigned int memflags);
> #endif
> 
>