linux-kernel - Re: [PATCH] prctl: fix PR_SET_MM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YE9hZtUSWovMDili@localhost.localdomain>
Date:   Mon, 15 Mar 2021 16:30:14 +0300
From:   Alexey Dobriyan <adobriyan@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
        gorcunov@...nvz.org, security@...nel.org,
        torvalds@...ux-foundation.org
Subject: Re: [PATCH] prctl: fix PR_SET_MM_AUXV kernel stack leak

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
>