RE: [PATCH v7 1/1] memory_hotplug: Add a bounds check to __add_pages

["Alastair D'Silva" <alastair@au1.ibm.com>]

On Wed, 2019-10-02 at 15:14 -0700, Andrew Morton wrote:
> On Tue, 1 Oct 2019 11:49:47 +0200 David Hildenbrand <david@redhat.com
> > wrote:
> 
> > > @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn,
> > > unsigned long nr_pages,
> > >  	return 0;
> > >  }
> > >  
> > > +static int check_hotplug_memory_addressable(unsigned long pfn,
> > > +					    unsigned long nr_pages)
> > > +{
> > > +	const u64 max_addr = PFN_PHYS(pfn + nr_pages) - 1;
> > > +
> > > +	if (max_addr >> MAX_PHYSMEM_BITS) {
> > > +		const u64 max_allowed = (1ull << (MAX_PHYSMEM_BITS +
> > > 1)) - 1;
> > > +		WARN(1,
> > > +		     "Hotplugged memory exceeds maximum addressable
> > > address, range=%#llx-%#llx, maximum=%#llx\n",
> > > +		     (u64)PFN_PHYS(pfn), max_addr, max_allowed);
> > > +		return -E2BIG;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /*
> > >   * Reasonably generic function for adding memory.  It is
> > >   * expected that archs that support memory hotplug will
> > > @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long
> > > pfn, unsigned long nr_pages,
> > >  	unsigned long nr, start_sec, end_sec;
> > >  	struct vmem_altmap *altmap = restrictions->altmap;
> > >  
> > > +	err = check_hotplug_memory_addressable(pfn, nr_pages);
> > > +	if (err)
> > > +		return err;
> > > +
> > >  	if (altmap) {
> > >  		/*
> > >  		 * Validate altmap is within bounds of the total
> > > request
> > > 
> > 
> > I actually wanted to give my RB to v7, not v6 :)
> > 
> 
> Given that check_hotplug_memory_addressable() is now static, I'll
> assume that the old [2/2]
> mm-add-a-bounds-check-in-devm_memremap_pages.patch is now obsolete.
> 

Yes, please ignore that whole series.

> From: Alastair D'Silva <alastair@d-silva.org>
> Subject: mm/memremap.c: add a bounds check in devm_memremap_pages()
> 
> The call to check_hotplug_memory_addressable() validates that the
> memory
> is fully addressable.
> 
> Without this call, it is possible that we may remap pages that is not
> physically addressable, resulting in bogus section numbers being
> returned
> from __section_nr().
> 
> Link: 
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lkml.kernel.org_r_20190917010752.28395-2D3-2Dalastair-40au1.ibm.com&d=DwICAg&c=jf_iaSHvJObTbx-siA1ZOg&r=cT4tgeEQ0Ll3SIlZDHE5AEXyKy6uKADMtf9_Eb7-vec&m=pVid6q3tQNfU2PQborLw8oYmNm9naF133dZ8AJ5lW9A&s=51ZuQa-kwRu8vW9vt5OgxjaIMWm4_n-aqp5xMSdkI4k&e=
>  
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> Acked-by: David Hildenbrand <david@redhat.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Logan Gunthorpe <logang@deltatee.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Oscar Salvador <osalvador@suse.com>
> Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
> Cc: Qian Cai <cai@lca.pw>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  mm/memremap.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> --- a/mm/memremap.c~mm-add-a-bounds-check-in-devm_memremap_pages
> +++ a/mm/memremap.c
> @@ -185,6 +185,11 @@ void *memremap_pages(struct dev_pagemap
>  	int error, is_ram;
>  	bool need_devmap_managed = true;
>  
> +	error = check_hotplug_memory_addressable(res->start,
> +						 resource_size(res));
> +	if (error)
> +		return ERR_PTR(error);
> +
>  	switch (pgmap->type) {
>  	case MEMORY_DEVICE_PRIVATE:
>  		if (!IS_ENABLED(CONFIG_DEVICE_PRIVATE)) {
> _
> 
-- 
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819