[patch 3/3] IA64: virt_to_page() can be called with NULL arg

[patch 3/3] IA64: virt_to_page() can be called with NULL arg

[akpm]

From: Kirill Korotaev <dev@openvz.org>

It does not return NULL when arg is NULL.

Signed-off-by: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Signed-off-by: Kirill Korotaev <dev@openvz.org>
Cc: "Luck, Tony" <tony.luck@intel.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 include/asm-ia64/pgalloc.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

diff -puN include/asm-ia64/pgalloc.h~ia64-virt_to_page-can-be-called-with-null-arg include/asm-ia64/pgalloc.h
--- a/include/asm-ia64/pgalloc.h~ia64-virt_to_page-can-be-called-with-null-arg
+++ a/include/asm-ia64/pgalloc.h
@@ -137,7 +137,8 @@ pmd_populate_kernel(struct mm_struct *mm
 static inline struct page *pte_alloc_one(struct mm_struct *mm,
 					 unsigned long addr)
 {
-	return virt_to_page(pgtable_quicklist_alloc());
+	void *pg = pgtable_quicklist_alloc();
+	return pg ? virt_to_page(pg) : NULL;
 }
 
 static inline pte_t *pte_alloc_one_kernel(struct mm_struct *mm,
_

Re: [patch 3/3] IA64: virt_to_page() can be called with NULL arg

[Jes Sorensen]

>>>>> "akpm" = akpm  <akpm@osdl.org> writes:

akpm> From: Kirill Korotaev <dev@openvz.org> It does not return NULL
akpm> when arg is NULL.

Shouldn't the real fix be to track down who calls virt_to_page() with
a NULL pointer? IMHO it is bogus to do so.

Cheers,
Jes

Re: [patch 3/3] IA64: virt_to_page() can be called with NULL arg

[Jes Sorensen]

Kirill Korotaev wrote:
>>>>>>> "akpm" = akpm  <akpm@osdl.org> writes:
>>
>> akpm> From: Kirill Korotaev <dev@openvz.org> It does not return NULL
>> akpm> when arg is NULL.
>>
>> Shouldn't the real fix be to track down who calls virt_to_page() with
>> a NULL pointer? IMHO it is bogus to do so.
> what do you propose? to insert BUG_ON(!kaddr) into virt_to_page()?
> in this case caller in question should be still fixed.

If you hit this, yes I'd insert the BUG_ON in your test kernel and fix
the code. Maybe add the BUG_ON in upstream for CONFIG_DEBUG or
something.

Which callers did you see cause this? If it was a common problem I would
expect a lot of data corruption or crashes on ia64 systems which I
haven't heard of.

Cheers,
Jes

Re: [patch 3/3] IA64: virt_to_page() can be called with NULL arg

[Kirill Korotaev]

>>>>>>"akpm" = akpm  <akpm@osdl.org> writes:
> 
> 
> akpm> From: Kirill Korotaev <dev@openvz.org> It does not return NULL
> akpm> when arg is NULL.
> 
> Shouldn't the real fix be to track down who calls virt_to_page() with
> a NULL pointer? IMHO it is bogus to do so.
what do you propose? to insert BUG_ON(!kaddr) into virt_to_page()?
in this case caller in question should be still fixed.

Thanks,
Kirill

Re: [patch 3/3] IA64: virt_to_page() can be called with NULL arg

[Jes Sorensen]

Kirill Korotaev wrote:
> Jes Sorensen wrote:
>> If you hit this, yes I'd insert the BUG_ON in your test kernel and fix
>> the code. Maybe add the BUG_ON in upstream for CONFIG_DEBUG or
>> something.
> I guess then all the platforms should be analyzed/patched carefully
> or all the callers of virt_to_page().
> Care to create debug patch?

Well you suggested a patch which just hides the problem. I suggest you
change it to have the BUG_ON().

>> Which callers did you see cause this? If it was a common problem I would
>> expect a lot of data corruption or crashes on ia64 systems which I
>> haven't heard of.
> from the patch:
> pte_alloc_one() calls pgtable_quicklist_alloc() which can return NULL in
> case of allocation failure.
> 
> It was hit on OpenVZ where kernel memory is accounted and limited on
> per-container basis (it is possible to DoS using page tables allocations).
> In mainstream the bug can be hit if OOM killer
> kills the process and __get_free_page() returns NULL which is rare, but still possible.

I see, since you have it tracked down, it would be good to fix it
and push a patch upstream. Unless of course Andrew or Linus thinks this
is the wrong approach.

Cheers,
Jes

Re: [patch 3/3] IA64: virt_to_page() can be called with NULL arg

[Kirill Korotaev]

Jes Sorensen wrote:
> Kirill Korotaev wrote:
> 
>>>>>>>>"akpm" = akpm  <akpm@osdl.org> writes:
>>>
>>>akpm> From: Kirill Korotaev <dev@openvz.org> It does not return NULL
>>>akpm> when arg is NULL.
>>>
>>>Shouldn't the real fix be to track down who calls virt_to_page() with
>>>a NULL pointer? IMHO it is bogus to do so.
>>
>>what do you propose? to insert BUG_ON(!kaddr) into virt_to_page()?
>>in this case caller in question should be still fixed.
> 
> 
> If you hit this, yes I'd insert the BUG_ON in your test kernel and fix
> the code. Maybe add the BUG_ON in upstream for CONFIG_DEBUG or
> something.
I guess then all the platforms should be analyzed/patched carefully
or all the callers of virt_to_page().
Care to create debug patch?

> Which callers did you see cause this? If it was a common problem I would
> expect a lot of data corruption or crashes on ia64 systems which I
> haven't heard of.
from the patch:
pte_alloc_one() calls pgtable_quicklist_alloc() which can return NULL in
case of allocation failure.

It was hit on OpenVZ where kernel memory is accounted and limited on
per-container basis (it is possible to DoS using page tables allocations).
In mainstream the bug can be hit if OOM killer
kills the process and __get_free_page() returns NULL which is rare, but still possible.

Thanks,
Kirill

Re: [patch 3/3] IA64: virt_to_page() can be called with NULL arg

[Andrew Morton]

On Wed, 20 Dec 2006 11:14:53 +0100
Jes Sorensen <jes@sgi.com> wrote:

> Kirill Korotaev wrote:
> > Jes Sorensen wrote:
> >> If you hit this, yes I'd insert the BUG_ON in your test kernel and fix
> >> the code. Maybe add the BUG_ON in upstream for CONFIG_DEBUG or
> >> something.
> > I guess then all the platforms should be analyzed/patched carefully
> > or all the callers of virt_to_page().
> > Care to create debug patch?
> 
> Well you suggested a patch which just hides the problem. I suggest you
> change it to have the BUG_ON().
> 

The patch doesn't hide any problems:

--- a/include/asm-ia64/pgalloc.h~ia64-virt_to_page-can-be-called-with-null-arg
+++ a/include/asm-ia64/pgalloc.h
@@ -137,7 +137,8 @@ pmd_populate_kernel(struct mm_struct *mm
 static inline struct page *pte_alloc_one(struct mm_struct *mm,
 					 unsigned long addr)
 {
-	return virt_to_page(pgtable_quicklist_alloc());
+	void *pg = pgtable_quicklist_alloc();
+	return pg ? virt_to_page(pg) : NULL;
 }
 


if the memory allocation function fails, we propagate that failure back to
callers.  All very good.

From a quick look it seems that the pte_alloc() callers are all nicely
handling the failures.

The patch looks good to me?

Re: [patch 3/3] IA64: virt_to_page() can be called with NULL arg

[Jes Sorensen]

Andrew Morton wrote:
> The patch doesn't hide any problems:
[snip]
> if the memory allocation function fails, we propagate that failure back to
> callers.  All very good.

ARGH!

I somehow put it in my head that the patch was modifying virt_to_page()
rather than the pte function.

My apologies, this was written by a stupid person who can't read :-(

Cheers,
Jes

Re: [patch 3/3] IA64: virt_to_page() can be called with NULL arg

[Kirill Korotaev]

Jes,

> Well you suggested a patch which just hides the problem. I suggest you
> change it to have the BUG_ON().
IMHO you are wrong.
the suggested patch *fixes* one particular place, which can be triggered on
mainstream IA64 by a standard user and is actually a *SECURITY* bug which
can be potentially exploited (when OOM killer is enabled).
It doesn't hide anything, It just doesn't help to catch other places.

>>>Which callers did you see cause this? If it was a common problem I would
>>>expect a lot of data corruption or crashes on ia64 systems which I
>>>haven't heard of.
>>
>>from the patch:
>>pte_alloc_one() calls pgtable_quicklist_alloc() which can return NULL in
>>case of allocation failure.
>>
>>It was hit on OpenVZ where kernel memory is accounted and limited on
>>per-container basis (it is possible to DoS using page tables allocations).
>>In mainstream the bug can be hit if OOM killer
>>kills the process and __get_free_page() returns NULL which is rare, but still possible.
> 
> 
> I see, since you have it tracked down, it would be good to fix it
> and push a patch upstream. Unless of course Andrew or Linus thinks this
> is the wrong approach.
Maybe the fact that I came without an exploit to crash IA64
makes you think it should not be commited, ok, you can leave it as is then.
NOTE: I don't mind against the debug you proposed. It is quite a good idea.

Thanks,
Kirill

Re: [patch 3/3] IA64: virt_to_page() can be called with NULL arg

[Jes Sorensen]

Kirill Korotaev wrote:
> Jes,
> 
>> Well you suggested a patch which just hides the problem. I suggest you
>> change it to have the BUG_ON().
> IMHO you are wrong.
> the suggested patch *fixes* one particular place, which can be triggered on
> mainstream IA64 by a standard user and is actually a *SECURITY* bug which
> can be potentially exploited (when OOM killer is enabled).
> It doesn't hide anything, It just doesn't help to catch other places.

Hi Kirill,

See my response to Andrew, you are indeed right.

I'll go back and hide in my corner in shame :-(

Cheers,
Jes