| Message ID | alpine.DEB.2.21.2109161341270.21985@sstabellini-ThinkPad-T480s (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | modify acquire_domstatic_pages to take an unsigned int size parameter | expand |
On 16.09.2021 22:47, Stefano Stabellini 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@xilinx.com> > --- > > 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. Fine by me; fwiw Acked-by: Jan Beulich <jbeulich@suse.com> Jan
Hi Stefano, > On 16 Sep 2021, at 21:47, Stefano Stabellini <sstabellini@kernel.org> 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@xilinx.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> 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 > >
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
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@xilinx.com> --- 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.