Message ID | 20190827052047.31547-1-alastair@au1.ibm.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | powerpc: Perform a bounds check in arch_add_memory | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (0e4523c0b4f64eaf7abe59e143e6bdf8f972acff) |
snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: > From: Alastair D'Silva <alastair@d-silva.org> > > It is possible for firmware to allocate memory ranges outside > the range of physical memory that we support (MAX_PHYSMEM_BITS). Doesn't that count as a FW bug? Do you have any evidence of that in the field? Just wondering... > This patch adds a bounds check to ensure that any hotplugged > memory is addressable. > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org> > --- > arch/powerpc/mm/mem.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c > index 9191a66b3bc5..de18fb73de30 100644 > --- a/arch/powerpc/mm/mem.c > +++ b/arch/powerpc/mm/mem.c > @@ -111,6 +111,9 @@ int __ref arch_add_memory(int nid, u64 start, u64 size, > unsigned long nr_pages = size >> PAGE_SHIFT; > int rc; > > + if ((start + size - 1) >> MAX_PHYSMEM_BITS) > + return -EINVAL; > + > resize_hpt_for_hotplug(memblock_phys_mem_size()); > > start = (unsigned long)__va(start); > -- > 2.21.0
On Tue, 2019-08-27 at 08:28 +0200, Michal Hocko wrote: > On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: > > From: Alastair D'Silva <alastair@d-silva.org> > > > > It is possible for firmware to allocate memory ranges outside > > the range of physical memory that we support (MAX_PHYSMEM_BITS). > > Doesn't that count as a FW bug? Do you have any evidence of that in > the > field? Just wondering... > Not outside our lab, but OpenCAPI attached LPC memory is assigned addresses based on the slot/NPU it is connected to. These addresses prior to: 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to 2PB") were inaccessible and resulted in bogus sections - see our discussion on 'mm: Trigger bug on if a section is not found in __section_nr'. Doing this check here was your suggestion :) It's entirely possible that a similar problem will occur in the future, and it's cheap to guard against, which is why I've added this.
On 27.08.19 08:39, Alastair D'Silva wrote: > On Tue, 2019-08-27 at 08:28 +0200, Michal Hocko wrote: >> On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: >>> From: Alastair D'Silva <alastair@d-silva.org> >>> >>> It is possible for firmware to allocate memory ranges outside >>> the range of physical memory that we support (MAX_PHYSMEM_BITS). >> >> Doesn't that count as a FW bug? Do you have any evidence of that in >> the >> field? Just wondering... >> > > Not outside our lab, but OpenCAPI attached LPC memory is assigned > addresses based on the slot/NPU it is connected to. These addresses > prior to: > 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to 2PB") > were inaccessible and resulted in bogus sections - see our discussion > on 'mm: Trigger bug on if a section is not found in __section_nr'. > Doing this check here was your suggestion :) > > It's entirely possible that a similar problem will occur in the future, > and it's cheap to guard against, which is why I've added this. > If you keep it here, I guess this should be wrapped by a WARN_ON_ONCE(). If we move it to common code (e.g., __add_pages() or add_memory()), then probably not. I can see that s390x allows to configure MAX_PHYSMEM_BITS, so the check could actually make sense.
On Tue 27-08-19 16:39:56, Alastair D'Silva wrote: > On Tue, 2019-08-27 at 08:28 +0200, Michal Hocko wrote: > > On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: > > > From: Alastair D'Silva <alastair@d-silva.org> > > > > > > It is possible for firmware to allocate memory ranges outside > > > the range of physical memory that we support (MAX_PHYSMEM_BITS). > > > > Doesn't that count as a FW bug? Do you have any evidence of that in > > the > > field? Just wondering... > > > > Not outside our lab, but OpenCAPI attached LPC memory is assigned > addresses based on the slot/NPU it is connected to. These addresses > prior to: > 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to 2PB") > were inaccessible and resulted in bogus sections - see our discussion > on 'mm: Trigger bug on if a section is not found in __section_nr'. Please document this in the changelog
On Tue, 2019-08-27 at 09:13 +0200, David Hildenbrand wrote: > On 27.08.19 08:39, Alastair D'Silva wrote: > > On Tue, 2019-08-27 at 08:28 +0200, Michal Hocko wrote: > > > On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: > > > > From: Alastair D'Silva <alastair@d-silva.org> > > > > > > > > It is possible for firmware to allocate memory ranges outside > > > > the range of physical memory that we support > > > > (MAX_PHYSMEM_BITS). > > > > > > Doesn't that count as a FW bug? Do you have any evidence of that > > > in > > > the > > > field? Just wondering... > > > > > > > Not outside our lab, but OpenCAPI attached LPC memory is assigned > > addresses based on the slot/NPU it is connected to. These addresses > > prior to: > > 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to > > 2PB") > > were inaccessible and resulted in bogus sections - see our > > discussion > > on 'mm: Trigger bug on if a section is not found in __section_nr'. > > Doing this check here was your suggestion :) > > > > It's entirely possible that a similar problem will occur in the > > future, > > and it's cheap to guard against, which is why I've added this. > > > > If you keep it here, I guess this should be wrapped by a > WARN_ON_ONCE(). > > If we move it to common code (e.g., __add_pages() or add_memory()), > then > probably not. I can see that s390x allows to configure > MAX_PHYSMEM_BITS, > so the check could actually make sense. > I couldn't see a nice platform indepedent way to determine the allowable address range, but if there is, then I'll move this to the generic code instead.
On 02.09.19 01:54, Alastair D'Silva wrote: > On Tue, 2019-08-27 at 09:13 +0200, David Hildenbrand wrote: >> On 27.08.19 08:39, Alastair D'Silva wrote: >>> On Tue, 2019-08-27 at 08:28 +0200, Michal Hocko wrote: >>>> On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: >>>>> From: Alastair D'Silva <alastair@d-silva.org> >>>>> >>>>> It is possible for firmware to allocate memory ranges outside >>>>> the range of physical memory that we support >>>>> (MAX_PHYSMEM_BITS). >>>> >>>> Doesn't that count as a FW bug? Do you have any evidence of that >>>> in >>>> the >>>> field? Just wondering... >>>> >>> >>> Not outside our lab, but OpenCAPI attached LPC memory is assigned >>> addresses based on the slot/NPU it is connected to. These addresses >>> prior to: >>> 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to >>> 2PB") >>> were inaccessible and resulted in bogus sections - see our >>> discussion >>> on 'mm: Trigger bug on if a section is not found in __section_nr'. >>> Doing this check here was your suggestion :) >>> >>> It's entirely possible that a similar problem will occur in the >>> future, >>> and it's cheap to guard against, which is why I've added this. >>> >> >> If you keep it here, I guess this should be wrapped by a >> WARN_ON_ONCE(). >> >> If we move it to common code (e.g., __add_pages() or add_memory()), >> then >> probably not. I can see that s390x allows to configure >> MAX_PHYSMEM_BITS, >> so the check could actually make sense. >> > > I couldn't see a nice platform indepedent way to determine the > allowable address range, but if there is, then I'll move this to the > generic code instead. > At least on the !ZONE_DEVICE path we have __add_memory() -> register_memory_resource() ... return ERR_PTR(-E2BIG); I was thinking about something like int add_pages() { if ((start + size - 1) >> MAX_PHYSMEM_BITS) return -E2BIG; return arch_add_memory(...) } And switching users of arch_add_memory() to add_pages(). However, x86 already has an add_pages() function, so that would need some more thought. Maybe simply renaming the existing add_pages() to arch_add_pages(). add_pages(): Create virtual mapping __add_pages(): Don't create virtual mapping arch_add_memory(): Arch backend for add_pages() arch_add_pages(): Arch backend for __add_pages() It would be even more consistent if we would have arch_add_pages() vs. __arch_add_pages().
On Mon, 2019-09-02 at 09:28 +0200, David Hildenbrand wrote: > On 02.09.19 01:54, Alastair D'Silva wrote: > > On Tue, 2019-08-27 at 09:13 +0200, David Hildenbrand wrote: > > > On 27.08.19 08:39, Alastair D'Silva wrote: > > > > On Tue, 2019-08-27 at 08:28 +0200, Michal Hocko wrote: > > > > > On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: > > > > > > From: Alastair D'Silva <alastair@d-silva.org> > > > > > > > > > > > > It is possible for firmware to allocate memory ranges > > > > > > outside > > > > > > the range of physical memory that we support > > > > > > (MAX_PHYSMEM_BITS). > > > > > > > > > > Doesn't that count as a FW bug? Do you have any evidence of > > > > > that > > > > > in > > > > > the > > > > > field? Just wondering... > > > > > > > > > > > > > Not outside our lab, but OpenCAPI attached LPC memory is > > > > assigned > > > > addresses based on the slot/NPU it is connected to. These > > > > addresses > > > > prior to: > > > > 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory > > > > to > > > > 2PB") > > > > were inaccessible and resulted in bogus sections - see our > > > > discussion > > > > on 'mm: Trigger bug on if a section is not found in > > > > __section_nr'. > > > > Doing this check here was your suggestion :) > > > > > > > > It's entirely possible that a similar problem will occur in the > > > > future, > > > > and it's cheap to guard against, which is why I've added this. > > > > > > > > > > If you keep it here, I guess this should be wrapped by a > > > WARN_ON_ONCE(). > > > > > > If we move it to common code (e.g., __add_pages() or > > > add_memory()), > > > then > > > probably not. I can see that s390x allows to configure > > > MAX_PHYSMEM_BITS, > > > so the check could actually make sense. > > > > > > > I couldn't see a nice platform indepedent way to determine the > > allowable address range, but if there is, then I'll move this to > > the > > generic code instead. > > > > At least on the !ZONE_DEVICE path we have > > __add_memory() -> register_memory_resource() ... > > return ERR_PTR(-E2BIG); > > > I was thinking about something like > > int add_pages() > { > if ((start + size - 1) >> MAX_PHYSMEM_BITS) > return -E2BIG; > > return arch_add_memory(...) > } > > And switching users of arch_add_memory() to add_pages(). However, x86 > already has an add_pages() function, so that would need some more > thought. > > Maybe simply renaming the existing add_pages() to arch_add_pages(). > > add_pages(): Create virtual mapping > __add_pages(): Don't create virtual mapping > > arch_add_memory(): Arch backend for add_pages() > arch_add_pages(): Arch backend for __add_pages() > > It would be even more consistent if we would have arch_add_pages() > vs. > __arch_add_pages(). Looking a bit further, I think a good course of action would be to add the check to memory_hotplug.c:check_hotplug_memory_range(). This would be the least invasive, and could check both MAX_POSSIBLE_PHYSMEM_BITS and MAX_PHYSMEM_BITS. With that in mind, we can drop this patch.
On 04.09.19 07:25, Alastair D'Silva wrote: > On Mon, 2019-09-02 at 09:28 +0200, David Hildenbrand wrote: >> On 02.09.19 01:54, Alastair D'Silva wrote: >>> On Tue, 2019-08-27 at 09:13 +0200, David Hildenbrand wrote: >>>> On 27.08.19 08:39, Alastair D'Silva wrote: >>>>> On Tue, 2019-08-27 at 08:28 +0200, Michal Hocko wrote: >>>>>> On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: >>>>>>> From: Alastair D'Silva <alastair@d-silva.org> >>>>>>> >>>>>>> It is possible for firmware to allocate memory ranges >>>>>>> outside >>>>>>> the range of physical memory that we support >>>>>>> (MAX_PHYSMEM_BITS). >>>>>> >>>>>> Doesn't that count as a FW bug? Do you have any evidence of >>>>>> that >>>>>> in >>>>>> the >>>>>> field? Just wondering... >>>>>> >>>>> >>>>> Not outside our lab, but OpenCAPI attached LPC memory is >>>>> assigned >>>>> addresses based on the slot/NPU it is connected to. These >>>>> addresses >>>>> prior to: >>>>> 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory >>>>> to >>>>> 2PB") >>>>> were inaccessible and resulted in bogus sections - see our >>>>> discussion >>>>> on 'mm: Trigger bug on if a section is not found in >>>>> __section_nr'. >>>>> Doing this check here was your suggestion :) >>>>> >>>>> It's entirely possible that a similar problem will occur in the >>>>> future, >>>>> and it's cheap to guard against, which is why I've added this. >>>>> >>>> >>>> If you keep it here, I guess this should be wrapped by a >>>> WARN_ON_ONCE(). >>>> >>>> If we move it to common code (e.g., __add_pages() or >>>> add_memory()), >>>> then >>>> probably not. I can see that s390x allows to configure >>>> MAX_PHYSMEM_BITS, >>>> so the check could actually make sense. >>>> >>> >>> I couldn't see a nice platform indepedent way to determine the >>> allowable address range, but if there is, then I'll move this to >>> the >>> generic code instead. >>> >> >> At least on the !ZONE_DEVICE path we have >> >> __add_memory() -> register_memory_resource() ... >> >> return ERR_PTR(-E2BIG); >> >> >> I was thinking about something like >> >> int add_pages() >> { >> if ((start + size - 1) >> MAX_PHYSMEM_BITS) >> return -E2BIG; >> >> return arch_add_memory(...) >> } >> >> And switching users of arch_add_memory() to add_pages(). However, x86 >> already has an add_pages() function, so that would need some more >> thought. >> >> Maybe simply renaming the existing add_pages() to arch_add_pages(). >> >> add_pages(): Create virtual mapping >> __add_pages(): Don't create virtual mapping >> >> arch_add_memory(): Arch backend for add_pages() >> arch_add_pages(): Arch backend for __add_pages() >> >> It would be even more consistent if we would have arch_add_pages() >> vs. >> __arch_add_pages(). > > Looking a bit further, I think a good course of action would be to add > the check to memory_hotplug.c:check_hotplug_memory_range(). > > This would be the least invasive, and could check both > MAX_POSSIBLE_PHYSMEM_BITS and MAX_PHYSMEM_BITS. You won't be able to catch the memremap path that way, just saying. But at least it would be an easy change. > > With that in mind, we can drop this patch. >
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c index 9191a66b3bc5..de18fb73de30 100644 --- a/arch/powerpc/mm/mem.c +++ b/arch/powerpc/mm/mem.c @@ -111,6 +111,9 @@ int __ref arch_add_memory(int nid, u64 start, u64 size, unsigned long nr_pages = size >> PAGE_SHIFT; int rc; + if ((start + size - 1) >> MAX_PHYSMEM_BITS) + return -EINVAL; + resize_hpt_for_hotplug(memblock_phys_mem_size()); start = (unsigned long)__va(start);