[PATCH] powerpc: Perform a bounds check in arch_add_memory

[PATCH] powerpc: Perform a bounds check in arch_add_memory

[Alastair D'Silva]

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).

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

Re: [PATCH] powerpc: Perform a bounds check in arch_add_memory

[Michal Hocko]

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

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] powerpc: Perform a bounds check in arch_add_memory

[Alastair D'Silva]

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.

-- 
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819

Re: [PATCH] powerpc: Perform a bounds check in arch_add_memory

[David Hildenbrand]

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.

-- 

Thanks,

David / dhildenb

RE: [PATCH] powerpc: Perform a bounds check in arch_add_memory

[Alastair D'Silva]

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.

-- 
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819

Re: [PATCH] powerpc: Perform a bounds check in arch_add_memory

[David Hildenbrand]

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().

-- 

Thanks,

David / dhildenb

Re: [PATCH] powerpc: Perform a bounds check in arch_add_memory

[Alastair D'Silva]

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.

-- 
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819

Re: [PATCH] powerpc: Perform a bounds check in arch_add_memory

[David Hildenbrand]

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.
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH] powerpc: Perform a bounds check in arch_add_memory

[Michal Hocko]

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

-- 
Michal Hocko
SUSE Labs