IA64: virt_to_page() can be called with NULL arg

Kirill Korotaev wrote:
>>>>>>> "akpm" == akpm 
<akpmosdl.org> writes:
>>
>> akpm> From: Kirill Korotaev <devopenvz.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
-
To unsubscribe from this list: send the line
"unsubscribe linux-ia64" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html

Jes Sorensen wrote:
> Kirill Korotaev wrote:
> 
>>>>>>>>"akpm" == akpm 
<akpmosdl.org> writes:
>>>
>>>akpm> From: Kirill Korotaev <devopenvz.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

-
To unsubscribe from this list: send the line
"unsubscribe linux-ia64" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html

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
-
To unsubscribe from this list: send the line
"unsubscribe linux-ia64" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html

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

-
To unsubscribe from this list: send the line
"unsubscribe linux-ia64" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html

On Wed, 20 Dec 2006 11:14:53 +0100
Jes Sorensen <jessgi.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?
-
To unsubscribe from this list: send the line
"unsubscribe linux-ia64" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html

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
-
To unsubscribe from this list: send the line
"unsubscribe linux-ia64" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html

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
-
To unsubscribe from this list: send the line
"unsubscribe linux-ia64" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html