Re: [PATCH] mm: Don't warn if memdup_user fails

Re: [PATCH] mm: Don't warn if memdup_user fails

[Andrew Morton]

On Wed, 11 Jan 2012 18:50:36 +0200
Sasha Levin <levinsasha928@gmail.com> wrote:

> memdup_user() is called when we need to copy data from userspace. This
> means that a user is able to trigger warnings if the kmalloc() inside
> memdup_user() fails.
> 
> For example, this is one caused by writing to much data to ecryptdev:
> 
> [  912.739685] ------------[ cut here ]------------
> [  912.745080] WARNING: at mm/page_alloc.c:2217 __alloc_pages_nodemask+0x22c/0x910()
> [  912.746525] Pid: 19977, comm: trinity Not tainted 3.2.0-next-20120110-sasha #120
> [  912.747915] Call Trace:
> [  912.748415]  [<ffffffff8115ec5c>] ? __alloc_pages_nodemask+0x22c/0x910
> [  912.749651]  [<ffffffff8109a2d5>] warn_slowpath_common+0x75/0xb0
> [  912.750756]  [<ffffffff8109a3d5>] warn_slowpath_null+0x15/0x20
> [  912.751831]  [<ffffffff8115ec5c>] __alloc_pages_nodemask+0x22c/0x910
> [  912.754230]  [<ffffffff81070fd5>] ? pvclock_clocksource_read+0x55/0xd0
> [  912.755484]  [<ffffffff8106ff56>] ? kvm_clock_read+0x46/0x80
> [  912.756565]  [<ffffffff810d1548>] ? sched_clock_cpu+0xc8/0x140
> [  912.757667]  [<ffffffff810cc731>] ? get_parent_ip+0x11/0x50
> [  912.758731]  [<ffffffff810cc731>] ? get_parent_ip+0x11/0x50
> [  912.759890]  [<ffffffff81341a4b>] ? ecryptfs_miscdev_write+0x6b/0x240
> [  912.761119]  [<ffffffff81196c80>] alloc_pages_current+0xa0/0x110
> [  912.762269]  [<ffffffff8115ba1f>] __get_free_pages+0xf/0x40
> [  912.763347]  [<ffffffff811a6082>] __kmalloc_track_caller+0x172/0x190
> [  912.764561]  [<ffffffff8116f0ab>] memdup_user+0x2b/0x90
> [  912.765526]  [<ffffffff81341a4b>] ecryptfs_miscdev_write+0x6b/0x240
> [  912.766669]  [<ffffffff813419e0>] ? ecryptfs_miscdev_open+0x190/0x190
> [  912.767832]  [<ffffffff811ba360>] do_loop_readv_writev+0x50/0x80
> [  912.770735]  [<ffffffff811ba69e>] do_readv_writev+0x1ce/0x1e0
> [  912.773059]  [<ffffffff8251bbbc>] ? __mutex_unlock_slowpath+0x10c/0x200
> [  912.774634]  [<ffffffff810cc731>] ? get_parent_ip+0x11/0x50
> [  912.775699]  [<ffffffff810cc8dd>] ? sub_preempt_count+0x9d/0xd0
> [  912.776827]  [<ffffffff8251f09d>] ? retint_swapgs+0x13/0x1b
> [  912.777887]  [<ffffffff811ba758>] vfs_writev+0x48/0x60
> [  912.779162]  [<ffffffff811ba86f>] sys_writev+0x4f/0xb0
> [  912.780152]  [<ffffffff8251f979>] system_call_fastpath+0x16/0x1b
> [  912.793046] ---[ end trace 50c38c9cdee53379 ]---
> [  912.793906] ecryptfs_miscdev_write: memdup_user returned error [-12]
> 
> Failing memdup_user() shouldn't be generating warnings, instead it should
> be notifying userspace about the error.
> 
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
>  mm/util.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/util.c b/mm/util.c
> index 136ac4f..88bb4d4 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -91,7 +91,7 @@ void *memdup_user(const void __user *src, size_t len)
>  	 * cause pagefault, which makes it pointless to use GFP_NOFS
>  	 * or GFP_ATOMIC.
>  	 */
> -	p = kmalloc_track_caller(len, GFP_KERNEL);
> +	p = kmalloc_track_caller(len, GFP_KERNEL | __GFP_NOWARN);
>  	if (!p)
>  		return ERR_PTR(-ENOMEM);

There's nothing particularly special about memdup_user(): there are
many ways in which userspace can trigger GFP_KERNEL allocations.

The problem here (one which your patch carefully covers up) is that
ecryptfs_miscdev_write() is passing an unchecked userspace-provided
`count' direct into kmalloc().  This is a bit problematic for other
reasons: it gives userspace a way to trigger heavy reclaim activity and
perhaps even to trigger the oom-killer.

A better fix here would be to validate the incoming arg before using
it.  Preferably by running ecryptfs_parse_packet_length() before taking
a copy of the data.  That would require adding a small copy_from_user()
to peek at the message header.

Re: [PATCH] mm: Don't warn if memdup_user fails

[Pekka Enberg]

On Wed, 11 Jan 2012, Andrew Morton wrote:
>> diff --git a/mm/util.c b/mm/util.c
>> index 136ac4f..88bb4d4 100644
>> --- a/mm/util.c
>> +++ b/mm/util.c
>> @@ -91,7 +91,7 @@ void *memdup_user(const void __user *src, size_t len)
>>  	 * cause pagefault, which makes it pointless to use GFP_NOFS
>>  	 * or GFP_ATOMIC.
>>  	 */
>> -	p = kmalloc_track_caller(len, GFP_KERNEL);
>> +	p = kmalloc_track_caller(len, GFP_KERNEL | __GFP_NOWARN);
>>  	if (!p)
>>  		return ERR_PTR(-ENOMEM);
>
> There's nothing particularly special about memdup_user(): there are
> many ways in which userspace can trigger GFP_KERNEL allocations.
>
> The problem here (one which your patch carefully covers up) is that
> ecryptfs_miscdev_write() is passing an unchecked userspace-provided
> `count' direct into kmalloc().  This is a bit problematic for other
> reasons: it gives userspace a way to trigger heavy reclaim activity and
> perhaps even to trigger the oom-killer.
>
> A better fix here would be to validate the incoming arg before using
> it.  Preferably by running ecryptfs_parse_packet_length() before taking
> a copy of the data.  That would require adding a small copy_from_user()
> to peek at the message header.

Yup, right you are. I didn't think about the reclaim and oom issue. We 
should add a big fat warning on top of memdup_user() to tell users to 
check 'len' for sanity themselves. I think they're now fooled into 
thinking memdup_user() automagically does the right thing.

 			Pekka

Re: [PATCH] mm: Don't warn if memdup_user fails

[Sasha Levin]

On Wed, 2012-01-11 at 14:12 -0800, Andrew Morton wrote:
> There's nothing particularly special about memdup_user(): there are
> many ways in which userspace can trigger GFP_KERNEL allocations.
> 
> The problem here (one which your patch carefully covers up) is that
> ecryptfs_miscdev_write() is passing an unchecked userspace-provided
> `count' direct into kmalloc().  This is a bit problematic for other
> reasons: it gives userspace a way to trigger heavy reclaim activity and
> perhaps even to trigger the oom-killer.
> 
> A better fix here would be to validate the incoming arg before using
> it.  Preferably by running ecryptfs_parse_packet_length() before taking
> a copy of the data.  That would require adding a small copy_from_user()
> to peek at the message header. 

Let's split it to two parts: the specific ecryptfs issue I've given as
an example here, and a general view about memdup_user().

I fully agree that in the case of ecryptfs there's a missing validity
check, and just calling memdup_user() with whatever the user has passed
to it is wrong and dangerous. This should be fixed in the ecryptfs code
and I'll send a patch to do that.

The other part, is memdup_user() itself. Kernel warnings are usually
reserved (AFAIK) to cases where it would be difficult to notify the user
since it happens in a flow which the user isn't directly responsible
for.

memdup_user() is always located in path which the user has triggered,
and is usually almost the first thing we try doing in response to the
trigger. In those code flows it doesn't make sense to print a kernel
warnings and taint the kernel, instead we can simply notify the user
about that error and let him deal with it any way he wants.

There are more reasons kalloc() can show warnings besides just trying to
allocate too much, and theres no reason to dump kernel warnings when
it's easier to notify the user.

-- 

Sasha.

Re: [PATCH] mm: Don't warn if memdup_user fails

[Pekka Enberg]

On Thu, Jan 12, 2012 at 10:06 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> Let's split it to two parts: the specific ecryptfs issue I've given as
> an example here, and a general view about memdup_user().
>
> I fully agree that in the case of ecryptfs there's a missing validity
> check, and just calling memdup_user() with whatever the user has passed
> to it is wrong and dangerous. This should be fixed in the ecryptfs code
> and I'll send a patch to do that.
>
> The other part, is memdup_user() itself. Kernel warnings are usually
> reserved (AFAIK) to cases where it would be difficult to notify the user
> since it happens in a flow which the user isn't directly responsible
> for.
>
> memdup_user() is always located in path which the user has triggered,
> and is usually almost the first thing we try doing in response to the
> trigger. In those code flows it doesn't make sense to print a kernel
> warnings and taint the kernel, instead we can simply notify the user
> about that error and let him deal with it any way he wants.
>
> There are more reasons kalloc() can show warnings besides just trying to
> allocate too much, and theres no reason to dump kernel warnings when
> it's easier to notify the user.

I think you missed Andrew's point. We absolutely want to issue a
kernel warning here because ecryptfs is misusing the memdup_user()
API. We must not let userspace processes allocate large amounts of
memory arbitrarily.

                        Pekka

Re: [PATCH] mm: Don't warn if memdup_user fails

[David Rientjes]

On Thu, 12 Jan 2012, Pekka Enberg wrote:

> I think you missed Andrew's point. We absolutely want to issue a
> kernel warning here because ecryptfs is misusing the memdup_user()
> API. We must not let userspace processes allocate large amounts of
> memory arbitrarily.
> 

I think it's good to fix ecryptfs like Tyler is doing and, at the same 
time, ensure that the len passed to memdup_user() makes sense prior to 
kmallocing memory with GFP_KERNEL.  Perhaps something like

	if (WARN_ON(len > PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER))
		return ERR_PTR(-ENOMEM);

in which case __GFP_NOWARN is irrelevant.  I think memdup_user() should 
definitely be taking gfp flags, though, so the caller can specify things 
like __GFP_NORETRY on its own to avoid infinitely looping in the page 
allocator trying reclaim and possibly calling the oom killer.

Re: [PATCH] mm: Don't warn if memdup_user fails

[Andrew Morton]

On Thu, 12 Jan 2012 13:19:54 -0800 (PST)
David Rientjes <rientjes@google.com> wrote:

> On Thu, 12 Jan 2012, Pekka Enberg wrote:
> 
> > I think you missed Andrew's point. We absolutely want to issue a
> > kernel warning here because ecryptfs is misusing the memdup_user()
> > API. We must not let userspace processes allocate large amounts of
> > memory arbitrarily.
> > 
> 
> I think it's good to fix ecryptfs like Tyler is doing and, at the same 
> time, ensure that the len passed to memdup_user() makes sense prior to 
> kmallocing memory with GFP_KERNEL.  Perhaps something like
> 
> 	if (WARN_ON(len > PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER))
> 		return ERR_PTR(-ENOMEM);
> 
> in which case __GFP_NOWARN is irrelevant.

If someone is passing huge size_t's into kmalloc() and getting failures
then that's probably a bug.  So perhaps we should add a warning to
kmalloc itself if the size_t is out of bounds, and !__GFP_NOWARN.

That might cause problems with those callers who like to call kmalloc()
in a probing loop with decreasing size_t.


But none of this will be very effective.  If someone is passing an
unchecked size_t into kmalloc then normal testing will not reveal the
problem because the testers won't pass stupid numbers into their
syscalls.

Re: [PATCH] mm: Don't warn if memdup_user fails

[David Rientjes]

On Thu, 12 Jan 2012, Andrew Morton wrote:

> > I think it's good to fix ecryptfs like Tyler is doing and, at the same 
> > time, ensure that the len passed to memdup_user() makes sense prior to 
> > kmallocing memory with GFP_KERNEL.  Perhaps something like
> > 
> > 	if (WARN_ON(len > PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER))
> > 		return ERR_PTR(-ENOMEM);
> > 
> > in which case __GFP_NOWARN is irrelevant.
> 
> If someone is passing huge size_t's into kmalloc() and getting failures
> then that's probably a bug.  So perhaps we should add a warning to
> kmalloc itself if the size_t is out of bounds, and !__GFP_NOWARN.
> 

That's already done.  For slub, for example, the largest object size 
handled by the allocator itself is an order-1 page; everything else gets 
passed through to the page allocator and its limitation is MAX_ORDER, 
which is the warning that we're seeing in Sasha's changelog when 
!__GFP_NOWARN.

> But none of this will be very effective.  If someone is passing an
> unchecked size_t into kmalloc then normal testing will not reveal the
> problem because the testers won't pass stupid numbers into their
> syscalls.
> 

They'll get the same warning that Sasha got, which is because the page 
allocator can't handle larger than MAX_ORDER orders.  The intention in my 
WARN_ON() above specifically for memdup_user() is to avoid the infinite 
loop in the page allocator for GFP_KERNEL allocations where the order 
is less than PAGE_ALLOC_COSTLY_ORDER and avoid the oom killer.  It returns 
immediately rather than passing __GFP_NORETRY since we don't want to incur 
the side-effects of direct reclaim or compaction as well.

The real fix would be to convert all callers to pass gfp flags into 
memdup_user() to determine the behavior they want, though.

Re: [PATCH] mm: Don't warn if memdup_user fails

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]

On Thu, Jan 12, 2012 at 01:58:03PM -0800, Andrew Morton wrote:
> On Thu, 12 Jan 2012 13:19:54 -0800 (PST)
> David Rientjes <rientjes@google.com> wrote:
> 
> > On Thu, 12 Jan 2012, Pekka Enberg wrote:
> > 
> > > I think you missed Andrew's point. We absolutely want to issue a
> > > kernel warning here because ecryptfs is misusing the memdup_user()
> > > API. We must not let userspace processes allocate large amounts of
> > > memory arbitrarily.
> > > 
> > 
> > I think it's good to fix ecryptfs like Tyler is doing and, at the same 
> > time, ensure that the len passed to memdup_user() makes sense prior to 
> > kmallocing memory with GFP_KERNEL.  Perhaps something like
> > 
> > 	if (WARN_ON(len > PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER))
> > 		return ERR_PTR(-ENOMEM);
> > 
> > in which case __GFP_NOWARN is irrelevant.
> 
> If someone is passing huge size_t's into kmalloc() and getting failures
> then that's probably a bug.

It's pretty common to pass high values to kmalloc().  We've added
a bunch of integer overflow checks recently where we do:

	if (n > ULONG_MAX / size)
		return -EINVAL;

The problem is that we didn't set a maximum bound before and we
can't know which maximum will break compatibility.

Probably we shouldn't do that, I guess.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mm: Don't warn if memdup_user fails

[Andrew Morton]

On Fri, 13 Jan 2012 10:17:52 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Thu, Jan 12, 2012 at 01:58:03PM -0800, Andrew Morton wrote:
> > On Thu, 12 Jan 2012 13:19:54 -0800 (PST)
> > David Rientjes <rientjes@google.com> wrote:
> > 
> > > On Thu, 12 Jan 2012, Pekka Enberg wrote:
> > > 
> > > > I think you missed Andrew's point. We absolutely want to issue a
> > > > kernel warning here because ecryptfs is misusing the memdup_user()
> > > > API. We must not let userspace processes allocate large amounts of
> > > > memory arbitrarily.
> > > > 
> > > 
> > > I think it's good to fix ecryptfs like Tyler is doing and, at the same 
> > > time, ensure that the len passed to memdup_user() makes sense prior to 
> > > kmallocing memory with GFP_KERNEL.  Perhaps something like
> > > 
> > > 	if (WARN_ON(len > PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER))
> > > 		return ERR_PTR(-ENOMEM);
> > > 
> > > in which case __GFP_NOWARN is irrelevant.
> > 
> > If someone is passing huge size_t's into kmalloc() and getting failures
> > then that's probably a bug.
> 
> It's pretty common to pass high values to kmalloc().  We've added
> a bunch of integer overflow checks recently where we do:
> 
> 	if (n > ULONG_MAX / size)
> 		return -EINVAL;

It would be cleaner to use kcalloc().  Except kcalloc() zeroes the memory
and we still don't have a non-zeroing kcalloc().

> The problem is that we didn't set a maximum bound before and we
> can't know which maximum will break compatibility.

Except for special cases (what are they?), code shouldn't be checking
for maximum kmalloc() size.  It should be checking the size against the
upper value which makes sense in the context of whatever it is doing at
the time.  This ecryptfs callsite is an example.

wrt any compatibility issues: the maximum amount of memory which can be
allocated by kmalloc() depends on the kernel config (see
kmalloc_sizes.h) so any code which is relying on any particular upper
bound is already busted.

Re: [PATCH] mm: Don't warn if memdup_user fails

[Tyler Hicks]

[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]

On 2012-01-12 10:06:34, Sasha Levin wrote:
> On Wed, 2012-01-11 at 14:12 -0800, Andrew Morton wrote:
> > There's nothing particularly special about memdup_user(): there are
> > many ways in which userspace can trigger GFP_KERNEL allocations.
> > 
> > The problem here (one which your patch carefully covers up) is that
> > ecryptfs_miscdev_write() is passing an unchecked userspace-provided
> > `count' direct into kmalloc().  This is a bit problematic for other
> > reasons: it gives userspace a way to trigger heavy reclaim activity and
> > perhaps even to trigger the oom-killer.
> > 
> > A better fix here would be to validate the incoming arg before using
> > it.  Preferably by running ecryptfs_parse_packet_length() before taking
> > a copy of the data.  That would require adding a small copy_from_user()
> > to peek at the message header. 
> 
> Let's split it to two parts: the specific ecryptfs issue I've given as
> an example here, and a general view about memdup_user().
> 
> I fully agree that in the case of ecryptfs there's a missing validity
> check, and just calling memdup_user() with whatever the user has passed
> to it is wrong and dangerous. This should be fixed in the ecryptfs code
> and I'll send a patch to do that.

I just wrote up a patch for the eCryptfs portion. I'll send it out a
little later after I get a chance to test it.

Tyler

> 
> The other part, is memdup_user() itself. Kernel warnings are usually
> reserved (AFAIK) to cases where it would be difficult to notify the user
> since it happens in a flow which the user isn't directly responsible
> for.
> 
> memdup_user() is always located in path which the user has triggered,
> and is usually almost the first thing we try doing in response to the
> trigger. In those code flows it doesn't make sense to print a kernel
> warnings and taint the kernel, instead we can simply notify the user
> about that error and let him deal with it any way he wants.
> 
> There are more reasons kalloc() can show warnings besides just trying to
> allocate too much, and theres no reason to dump kernel warnings when
> it's easier to notify the user.
> 
> -- 
> 
> Sasha.
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]