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

From: Alastair D'Silva
Date: Wed Oct 02 2019 - 19:28:03 EST


On Wed, 2019-10-02 at 15:14 -0700, Andrew Morton wrote:
> On Tue, 1 Oct 2019 11:49:47 +0200 David Hildenbrand <david@xxxxxxxxxx
> > 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@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
> Acked-by: David Hildenbrand <david@xxxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
> Cc: Jason Gunthorpe <jgg@xxxxxxxx>
> Cc: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxxx>
> Cc: Oscar Salvador <osalvador@xxxxxxxx>
> Cc: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
> Cc: Qian Cai <cai@xxxxxx>
> Cc: Wei Yang <richard.weiyang@xxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> 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