[PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak

[PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak

[Alexey Dobriyan]

	prctl(PR_SET_MM, PR_SET_MM_AUXV, addr, 1);

will copy 1 byte from userspace to (quite big) on-stack array
and then stash everything to mm->saved_auxv.
AT_NULL terminator will be inserted at the very end.

/proc/*/auxv handler will find that AT_NULL terminator
and copy original stack contents to userspace.

This devious scheme requires CAP_SYS_RESOURCE.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

	apply to >=3.5

 kernel/sys.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2079,7 +2079,7 @@ static int prctl_set_auxv(struct mm_struct *mm, unsigned long addr,
 	 * up to the caller to provide sane values here, otherwise userspace
 	 * tools which use this vector might be unhappy.
 	 */
-	unsigned long user_auxv[AT_VECTOR_SIZE];
+	unsigned long user_auxv[AT_VECTOR_SIZE] = {};
 
 	if (len > sizeof(user_auxv))
 		return -EINVAL;

Re: [PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak

[Linus Torvalds]

Applied directly, since I'm just about to tag rc3 and was just looking
that there were no last-minute pull requests.

Andrew, no need to pick it up into your queue.

Side note: I think we should return -EINVAL more aggressively: right
now we fill up potentially all of user_auxv[] and return success, but
we will have always cleared that last auxv pointer pair.

So we actually return "success" even when the user supplies us with
more data than we then really accept.

IOW, tightening that up might be worth it (maybe actually check that
they are valid user pointers at the same time).

That's a separate issue, and I can't find it in myself to care (and
nobody has ever complained), but I thought I'd mention it.

                 Linus

Re: [PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak

[Cyrill Gorcunov]

On Sun, Mar 14, 2021 at 11:51:14PM +0300, Alexey Dobriyan wrote:
> 	prctl(PR_SET_MM, PR_SET_MM_AUXV, addr, 1);
> 
> will copy 1 byte from userspace to (quite big) on-stack array
> and then stash everything to mm->saved_auxv.
> AT_NULL terminator will be inserted at the very end.
> 
> /proc/*/auxv handler will find that AT_NULL terminator
> and copy original stack contents to userspace.
> 
> This devious scheme requires CAP_SYS_RESOURCE.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---

Thanks for catching up, Alexey!

Re: [PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak

[Cyrill Gorcunov]

On Sun, Mar 14, 2021 at 02:40:05PM -0700, Linus Torvalds wrote:
> Applied directly, since I'm just about to tag rc3 and was just looking
> that there were no last-minute pull requests.
> 
> Andrew, no need to pick it up into your queue.
> 
> Side note: I think we should return -EINVAL more aggressively: right
> now we fill up potentially all of user_auxv[] and return success, but
> we will have always cleared that last auxv pointer pair.
> 
> So we actually return "success" even when the user supplies us with
> more data than we then really accept.

Yes, this is somehow weird and probably we should start complaining
if last two elements in the user array is not AT_NULL but I fear
this might break backward compatibility? Dunno if someone relies
on kernel to setup last two entries unconditionally.

> 
> IOW, tightening that up might be worth it (maybe actually check that
> they are valid user pointers at the same time).
> 
> That's a separate issue, and I can't find it in myself to care (and
> nobody has ever complained), but I thought I'd mention it.

auxv stuff (Re: [PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak)

[Alexey Dobriyan]

On Sun, Mar 14, 2021 at 02:40:05PM -0700, Linus Torvalds wrote:
> [mm->saved_auxv]
> 
> That's a separate issue, and I can't find it in myself to care (and
> nobody has ever complained), but I thought I'd mention it.

There is another (non-security) one. Compat 32-bit process will report
2 longs too many:

00000000  20 00 00 00 40 85 f5 f7  21 00 00 00 00 80 f5 f7  | ...@...!.......|
00000010  10 00 00 00 ff fb 8b 0f  06 00 00 00 00 10 00 00  |................|
00000020  11 00 00 00 64 00 00 00  03 00 00 00 34 80 04 08  |....d.......4...|
00000030  04 00 00 00 20 00 00 00  05 00 00 00 08 00 00 00  |.... ...........|
00000040  07 00 00 00 00 90 f5 f7  08 00 00 00 00 00 00 00  |................|
00000050  09 00 00 00 25 83 04 08  0b 00 00 00 00 00 00 00  |....%...........|
00000060  0c 00 00 00 00 00 00 00  0d 00 00 00 00 00 00 00  |................|
00000070  0e 00 00 00 00 00 00 00  17 00 00 00 00 00 00 00  |................|
00000080  19 00 00 00 8b 27 99 ff  1a 00 00 00 02 00 00 00  |.....'..........|
00000090  1f 00 00 00 f0 2f 99 ff  0f 00 00 00 9b 27 99 ff  |...../.......'..|
000000a0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
	  AT_NULL     AT_NULL	   ^^^^^^^^^^^^^^^^^^^^^^^
000000b0

Re: auxv stuff (Re: [PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak)

[Cyrill Gorcunov]

On Mon, Mar 15, 2021 at 09:00:00AM +0300, Alexey Dobriyan wrote:
> On Sun, Mar 14, 2021 at 02:40:05PM -0700, Linus Torvalds wrote:
> > [mm->saved_auxv]
> > 
> > That's a separate issue, and I can't find it in myself to care (and
> > nobody has ever complained), but I thought I'd mention it.
> 
> There is another (non-security) one. Compat 32-bit process will report
> 2 longs too many:

Good catch! Alexey, should I address it? Or you have fixed it already?

Re: [PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak

[Dan Carpenter]

On Sun, Mar 14, 2021 at 11:51:14PM +0300, Alexey Dobriyan wrote:
> 	prctl(PR_SET_MM, PR_SET_MM_AUXV, addr, 1);
> 
> will copy 1 byte from userspace to (quite big) on-stack array
> and then stash everything to mm->saved_auxv.

What?  It won't save everything, only the 1 byte.  What am I not seeing?

kernel/sys.c
  2073  static int prctl_set_auxv(struct mm_struct *mm, unsigned long addr,
  2074                            unsigned long len)
  2075  {
  2076          /*
  2077           * This doesn't move the auxiliary vector itself since it's pinned to
  2078           * mm_struct, but it permits filling the vector with new values.  It's
  2079           * up to the caller to provide sane values here, otherwise userspace
  2080           * tools which use this vector might be unhappy.
  2081           */
  2082          unsigned long user_auxv[AT_VECTOR_SIZE] = {};
  2083  
  2084          if (len > sizeof(user_auxv))
  2085                  return -EINVAL;
  2086  
  2087          if (copy_from_user(user_auxv, (const void __user *)addr, len))
                                   ^^^^^^^^^                             ^^^
Copies one byte from user space.

  2088                  return -EFAULT;
  2089  
  2090          /* Make sure the last entry is always AT_NULL */
  2091          user_auxv[AT_VECTOR_SIZE - 2] = 0;
  2092          user_auxv[AT_VECTOR_SIZE - 1] = 0;
  2093  
  2094          BUILD_BUG_ON(sizeof(user_auxv) != sizeof(mm->saved_auxv));
  2095  
  2096          task_lock(current);
  2097          memcpy(mm->saved_auxv, user_auxv, len);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Saves one byte to mm->saved_auxv.

  2098          task_unlock(current);
  2099  
  2100          return 0;
  2101  }

regards,
dan carpenter

Re: [PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak

[Oleg Nesterov]

On 03/14, Alexey Dobriyan wrote:
>
> 	prctl(PR_SET_MM, PR_SET_MM_AUXV, addr, 1);
>
> will copy 1 byte from userspace to (quite big) on-stack array
> and then stash everything to mm->saved_auxv.

I too don't understand, memcpy(mm->saved_auxv, user_auxv, len) will
copy 1 byte...

And why task_lock(current) ? What does it try to protect?

Oleg.

Re: [PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak

[Cyrill Gorcunov]

On Mon, Mar 15, 2021 at 01:08:03PM +0100, Oleg Nesterov wrote:
> On 03/14, Alexey Dobriyan wrote:
> >
> > 	prctl(PR_SET_MM, PR_SET_MM_AUXV, addr, 1);
> >
> > will copy 1 byte from userspace to (quite big) on-stack array
> > and then stash everything to mm->saved_auxv.
> 
> I too don't understand, memcpy(mm->saved_auxv, user_auxv, len) will
> copy 1 byte...

Indeed. I overlooked that we pass @len when copying. I should
not reply at night :(

> 
> And why task_lock(current) ? What does it try to protect?

As far as I remember this was related to reading from procfs
at time the patch was written for first time. Looks like this
not relevant anymore and could be dropped.

Re: [PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak

[Oleg Nesterov]

On 03/15, Cyrill Gorcunov wrote:
>
> On Mon, Mar 15, 2021 at 01:08:03PM +0100, Oleg Nesterov wrote:
>
> >
> > And why task_lock(current) ? What does it try to protect?
>
> As far as I remember this was related to reading from procfs
> at time the patch was written for first time. Looks like this
> not relevant anymore and could be dropped.

I think this was never relevant, ->alloc_lock is per-thread, not per mm.

Oleg.

Re: [PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak

[Alexey Dobriyan]

On Mon, Mar 15, 2021 at 01:29:02PM +0300, Dan Carpenter wrote:
> On Sun, Mar 14, 2021 at 11:51:14PM +0300, Alexey Dobriyan wrote:
> > 	prctl(PR_SET_MM, PR_SET_MM_AUXV, addr, 1);
> > 
> > will copy 1 byte from userspace to (quite big) on-stack array
> > and then stash everything to mm->saved_auxv.
> 
> What?  It won't save everything, only the 1 byte.  What am I not seeing?

It does copy 1 byte. How embarassing of me.

I was confused by another way of setting auxv data:

	        if (prctl_map.auxv_size)
	                memcpy(mm->saved_auxv, user_auxv, sizeof(user_auxv));

This does full array copy but the array is fully initialised so there is
no problem.

Stop the presses!

> kernel/sys.c
>   2073  static int prctl_set_auxv(struct mm_struct *mm, unsigned long addr,
>   2074                            unsigned long len)
>   2075  {
>   2076          /*
>   2077           * This doesn't move the auxiliary vector itself since it's pinned to
>   2078           * mm_struct, but it permits filling the vector with new values.  It's
>   2079           * up to the caller to provide sane values here, otherwise userspace
>   2080           * tools which use this vector might be unhappy.
>   2081           */
>   2082          unsigned long user_auxv[AT_VECTOR_SIZE] = {};
>   2083  
>   2084          if (len > sizeof(user_auxv))
>   2085                  return -EINVAL;
>   2086  
>   2087          if (copy_from_user(user_auxv, (const void __user *)addr, len))
>                                    ^^^^^^^^^                             ^^^
> Copies one byte from user space.
> 
>   2088                  return -EFAULT;
>   2089  
>   2090          /* Make sure the last entry is always AT_NULL */
>   2091          user_auxv[AT_VECTOR_SIZE - 2] = 0;
>   2092          user_auxv[AT_VECTOR_SIZE - 1] = 0;
>   2093  
>   2094          BUILD_BUG_ON(sizeof(user_auxv) != sizeof(mm->saved_auxv));
>   2095  
>   2096          task_lock(current);
>   2097          memcpy(mm->saved_auxv, user_auxv, len);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Saves one byte to mm->saved_auxv.
> 
>   2098          task_unlock(current);
>   2099  
>   2100          return 0;
>   2101  }
> 
> regards,
> dan carpenter
> 

Re: [PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak

[Cyrill Gorcunov]

On Mon, Mar 15, 2021 at 02:19:12PM +0100, Oleg Nesterov wrote:
> > >
> > > And why task_lock(current) ? What does it try to protect?
> >
> > As far as I remember this was related to reading from procfs
> > at time the patch was written for first time. Looks like this
> > not relevant anymore and could be dropped.
> 
> I think this was never relevant, ->alloc_lock is per-thread, not per mm.

Then we can safely drop it. I'll take one more look once time permit
and prepare a patch.

Re: auxv stuff (Re: [PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak)

[Alexey Dobriyan]

On Mon, Mar 15, 2021 at 09:42:47AM +0300, Cyrill Gorcunov wrote:
> On Mon, Mar 15, 2021 at 09:00:00AM +0300, Alexey Dobriyan wrote:
> > On Sun, Mar 14, 2021 at 02:40:05PM -0700, Linus Torvalds wrote:
> > > [mm->saved_auxv]
> > > 
> > > That's a separate issue, and I can't find it in myself to care (and
> > > nobody has ever complained), but I thought I'd mention it.
> > 
> > There is another (non-security) one. Compat 32-bit process will report
> > 2 longs too many:
> 
> Good catch! Alexey, should I address it? Or you have fixed it already?

I didn't and I don't know how frankly.
Something I've noticed during more important auxv rewrite.

Re: auxv stuff (Re: [PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak)

[Cyrill Gorcunov]

On Tue, Mar 16, 2021 at 09:50:35PM +0300, Alexey Dobriyan wrote:
> > > 
> > > There is another (non-security) one. Compat 32-bit process will report
> > > 2 longs too many:
> > 
> > Good catch! Alexey, should I address it? Or you have fixed it already?
> 
> I didn't and I don't know how frankly.
> Something I've noticed during more important auxv rewrite.

OK. I will then. Thanks!