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 <[email protected]>
---
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;
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
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 <[email protected]>
> ---
Thanks for catching up, Alexey!
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.
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
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?
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
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.
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.
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.
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
>
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.
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.
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!