[PATCH] mm: __nr_to_section - make it safe against overflow v2

[PATCH] mm: __nr_to_section - make it safe against overflow v2

[Cyrill Gorcunov]

__nr_to_section should check for array bound overflow.
We should better get NULL dereference then silently
pass some memory snippet out of bounds to a caller.

Also add a comment about mem_section structure.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 include/linux/mmzone.h |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Index: linux-2.6.git/include/linux/mmzone.h
===================================================================
--- linux-2.6.git.orig/include/linux/mmzone.h
+++ linux-2.6.git/include/linux/mmzone.h
@@ -935,6 +935,12 @@ static inline unsigned long early_pfn_to
 
 struct page;
 struct page_cgroup;
+
+/*
+ * NOTE: sizeof(struct mem_section) _must_ be power of 2
+ * otherwise SECTION_ROOT_MASK will be broken so be
+ * really cautious while modifying this structure
+ */
 struct mem_section {
 	/*
 	 * This is, logically, a pointer to an array of struct
@@ -980,9 +986,14 @@ extern struct mem_section mem_section[NR
 
 static inline struct mem_section *__nr_to_section(unsigned long nr)
 {
-	if (!mem_section[SECTION_NR_TO_ROOT(nr)])
+	unsigned long idx = SECTION_NR_TO_ROOT(nr);
+
+	if (WARN_ON(idx >= NR_SECTION_ROOTS))
+		return NULL;
+
+	if (!mem_section[idx])
 		return NULL;
-	return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK];
+	return &mem_section[idx][nr & SECTION_ROOT_MASK];
 }
 extern int __section_nr(struct mem_section* ms);
 extern unsigned long usemap_size(void);

Re: [PATCH] mm: __nr_to_section - make it safe against overflow v2

[Pekka Enberg]

On Mon, 2009-01-05 at 13:31 +0300, Cyrill Gorcunov wrote:
> __nr_to_section should check for array bound overflow.
> We should better get NULL dereference then silently
> pass some memory snippet out of bounds to a caller.
> 
> Also add a comment about mem_section structure.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>

Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>

> ---
>  include/linux/mmzone.h |   15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6.git/include/linux/mmzone.h
> ===================================================================
> --- linux-2.6.git.orig/include/linux/mmzone.h
> +++ linux-2.6.git/include/linux/mmzone.h
> @@ -935,6 +935,12 @@ static inline unsigned long early_pfn_to
>  
>  struct page;
>  struct page_cgroup;
> +
> +/*
> + * NOTE: sizeof(struct mem_section) _must_ be power of 2
> + * otherwise SECTION_ROOT_MASK will be broken so be
> + * really cautious while modifying this structure
> + */
>  struct mem_section {
>  	/*
>  	 * This is, logically, a pointer to an array of struct
> @@ -980,9 +986,14 @@ extern struct mem_section mem_section[NR
>  
>  static inline struct mem_section *__nr_to_section(unsigned long nr)
>  {
> -	if (!mem_section[SECTION_NR_TO_ROOT(nr)])
> +	unsigned long idx = SECTION_NR_TO_ROOT(nr);
> +
> +	if (WARN_ON(idx >= NR_SECTION_ROOTS))
> +		return NULL;
> +
> +	if (!mem_section[idx])
>  		return NULL;
> -	return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK];
> +	return &mem_section[idx][nr & SECTION_ROOT_MASK];
>  }
>  extern int __section_nr(struct mem_section* ms);
>  extern unsigned long usemap_size(void);

Re: [PATCH] mm: __nr_to_section - make it safe against overflow v2

[Andrew Morton]

On Mon, 5 Jan 2009 13:31:32 +0300
Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> __nr_to_section should check for array bound overflow.
> We should better get NULL dereference then silently
> pass some memory snippet out of bounds to a caller.
> 

Are there actually any known problems here?

> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---
>  include/linux/mmzone.h |   15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6.git/include/linux/mmzone.h
> ===================================================================
> --- linux-2.6.git.orig/include/linux/mmzone.h
> +++ linux-2.6.git/include/linux/mmzone.h
> @@ -935,6 +935,12 @@ static inline unsigned long early_pfn_to
>  
>  struct page;
>  struct page_cgroup;
> +
> +/*
> + * NOTE: sizeof(struct mem_section) _must_ be power of 2
> + * otherwise SECTION_ROOT_MASK will be broken so be
> + * really cautious while modifying this structure
> + */
>  struct mem_section {
>  	/*
>  	 * This is, logically, a pointer to an array of struct
> @@ -980,9 +986,14 @@ extern struct mem_section mem_section[NR
>  
>  static inline struct mem_section *__nr_to_section(unsigned long nr)
>  {
> -	if (!mem_section[SECTION_NR_TO_ROOT(nr)])
> +	unsigned long idx = SECTION_NR_TO_ROOT(nr);
> +
> +	if (WARN_ON(idx >= NR_SECTION_ROOTS))
> +		return NULL;
> +
> +	if (!mem_section[idx])
>  		return NULL;
> -	return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK];
> +	return &mem_section[idx][nr & SECTION_ROOT_MASK];
>  }

The patch adds nearly 300 bytes of stuff to mm/sparse.o, and for what??

Re: [PATCH] mm: __nr_to_section - make it safe against overflow v2

[KAMEZAWA Hiroyuki]

On Mon, 5 Jan 2009 16:37:42 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 5 Jan 2009 13:31:32 +0300
> Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> 
> > __nr_to_section should check for array bound overflow.
> > We should better get NULL dereference then silently
> > pass some memory snippet out of bounds to a caller.
> > 
> 
> Are there actually any known problems here?
> 

IIRC, I never saw any problem. (But I may see in memory-hotplug development.)
I don't like adding any check here.

This __nr_to_section[] is very internal function and will not be accessed directly
by codes other than VM(of sparsemem and memory hotplug).

For general users,  this function will be called via pfn_to_page(). So, this means
what this patch should really do is checking "pfn" argugments to pfn_to_page().
Of course, I don't want any check in pfn_to_page()'s "pfn" range.But, pfn_to_page()
is usually called with pfn_valid() if necessary. pfn_valid() and pfn_present(),
other callers of this, check this in explicit way.


Cyrill, if you really want to do check "idx", please add

==
__nr_to_section_direct(unsigned long nr) (or some better name)
{
	BUG_ON(nr >NR_SECTION_ROOTS);
	return __nr_to_section(nr);
}
==
to, mm/internal.h or somewhere. and use this in sparse.c and memhotplug.c


So, Nack from me for now.


Thanks,
-Kame

Re: [PATCH] mm: __nr_to_section - make it safe against overflow v2

[KAMEZAWA Hiroyuki]

On Tue, 6 Jan 2009 14:57:41 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Mon, 5 Jan 2009 16:37:42 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Mon, 5 Jan 2009 13:31:32 +0300
> > Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> > 
> > > __nr_to_section should check for array bound overflow.
> > > We should better get NULL dereference then silently
> > > pass some memory snippet out of bounds to a caller.
> > > 
> > 
> > Are there actually any known problems here?
> > 
> 
> IIRC, I never saw any problem. (But I may see in memory-hotplug development.)

I digged mails and seems this patch is from this thread.

=
http://lkml.org/lkml/2009/1/4/61
=

He seems to want to hunt boot time failure.
(Finally, BIOS-update was necessary for original problem of this thread.)

Cyrill, could you modify this WARN_ON() works works when __nr_to_section()
is called directly by sparse.c and memhotplug.c ?

Adding WARN_ON() in pfn_to_page() is overkill.

Thanks,
-Kame

Re: [PATCH] mm: __nr_to_section - make it safe against overflow v2

[Cyrill Gorcunov]

[KAMEZAWA Hiroyuki - Tue, Jan 06, 2009 at 03:30:36PM +0900]
| On Tue, 6 Jan 2009 14:57:41 +0900
| KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
| 
| > On Mon, 5 Jan 2009 16:37:42 -0800
| > Andrew Morton <akpm@linux-foundation.org> wrote:
| > 
| > > On Mon, 5 Jan 2009 13:31:32 +0300
| > > Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > > 
| > > > __nr_to_section should check for array bound overflow.
| > > > We should better get NULL dereference then silently
| > > > pass some memory snippet out of bounds to a caller.
| > > > 
| > > 
| > > Are there actually any known problems here?
| > > 
| > 
| > IIRC, I never saw any problem. (But I may see in memory-hotplug development.)
| 
| I digged mails and seems this patch is from this thread.
| 
| =
| http://lkml.org/lkml/2009/1/4/61
| =
| 
| He seems to want to hunt boot time failure.
| (Finally, BIOS-update was necessary for original problem of this thread.)
| 
| Cyrill, could you modify this WARN_ON() works works when __nr_to_section()
| is called directly by sparse.c and memhotplug.c ?
| 
| Adding WARN_ON() in pfn_to_page() is overkill.
| 
| Thanks,
| -Kame
| 

Thanks for review Kame, this patch should be just dropped.

Actually the only code snippet which make me nervious is
sizeof (mem_section) part. Which is to remain power of
two and even doesn't have 'packed' attribute neither any
comments above. And if it happens that it will be modified
or say gcc decide to add some bytes here (bugs happens) we
would silently address wrong mem_section. So I think at least
the 'comment' part of my patch is deserve to be applied :)

		- Cyrill -

Re: [PATCH] mm: __nr_to_section - make it safe against overflow v2

[Cyrill Gorcunov]

[Andrew Morton - Mon, Jan 05, 2009 at 04:37:42PM -0800]
...
| The patch adds nearly 300 bytes of stuff to mm/sparse.o, and for what??
| 
...

No need to apply it. Sorry for noise.

		- Cyrill -

Re: [PATCH] mm: __nr_to_section - make it safe against overflow v2

[Pekka Enberg]

Hi Andrew,

On Tue, Jan 6, 2009 at 2:37 AM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Mon, 5 Jan 2009 13:31:32 +0300
> Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
>> __nr_to_section should check for array bound overflow.
>> We should better get NULL dereference then silently
>> pass some memory snippet out of bounds to a caller.
>>
>
> Are there actually any known problems here?
>
>>
>> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
>> ---
>>  include/linux/mmzone.h |   15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> Index: linux-2.6.git/include/linux/mmzone.h
>> ===================================================================
>> --- linux-2.6.git.orig/include/linux/mmzone.h
>> +++ linux-2.6.git/include/linux/mmzone.h
>> @@ -935,6 +935,12 @@ static inline unsigned long early_pfn_to
>>
>>  struct page;
>>  struct page_cgroup;
>> +
>> +/*
>> + * NOTE: sizeof(struct mem_section) _must_ be power of 2
>> + * otherwise SECTION_ROOT_MASK will be broken so be
>> + * really cautious while modifying this structure
>> + */
>>  struct mem_section {
>>       /*
>>        * This is, logically, a pointer to an array of struct
>> @@ -980,9 +986,14 @@ extern struct mem_section mem_section[NR
>>
>>  static inline struct mem_section *__nr_to_section(unsigned long nr)
>>  {
>> -     if (!mem_section[SECTION_NR_TO_ROOT(nr)])
>> +     unsigned long idx = SECTION_NR_TO_ROOT(nr);
>> +
>> +     if (WARN_ON(idx >= NR_SECTION_ROOTS))
>> +             return NULL;
>> +
>> +     if (!mem_section[idx])
>>               return NULL;
>> -     return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK];
>> +     return &mem_section[idx][nr & SECTION_ROOT_MASK];
>>  }
>
> The patch adds nearly 300 bytes of stuff to mm/sparse.o, and for what??

I was thinking of things like commit
bead9a3abd15710b0bdfd418daef606722d86282 ("mm: sparsemem
memory_present() fix") and such but missed the fact that
__nr_to_section() inlined all over the place. So yeah, it's not worth
it.

                                            Pekka