diff mbox series

sys: don't hold uts_sem while accessing userspace memory

Message ID 20180625163410.216168-1-jannh@google.com
State Not Applicable
Delegated to: David Miller
Headers show
Series sys: don't hold uts_sem while accessing userspace memory | expand

Commit Message

Jann Horn June 25, 2018, 4:34 p.m. UTC 
Holding uts_sem as a writer while accessing userspace memory allows a
namespace admin to stall all processes that attempt to take uts_sem.
Instead, move data through stack buffers and don't access userspace memory
while uts_sem is held.

Cc: stable@vger.kernel.org
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Jann Horn <jannh@google.com>
---
 arch/alpha/kernel/osf_sys.c      | 51 ++++++++---------
 arch/sparc/kernel/sys_sparc_32.c | 22 +++++---
 arch/sparc/kernel/sys_sparc_64.c | 20 ++++---
 kernel/sys.c                     | 95 +++++++++++++++-----------------
 kernel/utsname_sysctl.c          | 41 ++++++++------
 5 files changed, 119 insertions(+), 110 deletions(-)

Comments

Al Viro June 25, 2018, 4:41 p.m. UTC | #1 
On Mon, Jun 25, 2018 at 06:34:10PM +0200, Jann Horn wrote:

> +	char tmp[32];
>  
> -	if (namelen > 32)
> +	if (namelen < 0 || namelen > 32)
>  		namelen = 32;
>  
>  	down_read(&uts_sem);
>  	kname = utsname()->domainname;
>  	len = strnlen(kname, namelen);
> -	if (copy_to_user(name, kname, min(len + 1, namelen)))
> -		err = -EFAULT;
> +	len = min(len + 1, namelen);
> +	memcpy(tmp, kname, len);
>  	up_read(&uts_sem);
>  
> -	return err;
> +	if (copy_to_user(name, tmp, len))
> +		return -EFAULT;

Infoleak, and similar in a lot of other places.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jann Horn June 25, 2018, 6:16 p.m. UTC | #2 
On Mon, Jun 25, 2018 at 6:41 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Jun 25, 2018 at 06:34:10PM +0200, Jann Horn wrote:
>
> > +     char tmp[32];
> >
> > -     if (namelen > 32)
> > +     if (namelen < 0 || namelen > 32)
> >               namelen = 32;
> >
> >       down_read(&uts_sem);
> >       kname = utsname()->domainname;
> >       len = strnlen(kname, namelen);
> > -     if (copy_to_user(name, kname, min(len + 1, namelen)))
> > -             err = -EFAULT;
> > +     len = min(len + 1, namelen);
> > +     memcpy(tmp, kname, len);
> >       up_read(&uts_sem);
> >
> > -     return err;
> > +     if (copy_to_user(name, tmp, len))
> > +             return -EFAULT;
>
> Infoleak, and similar in a lot of other places.

I don't see a problem. copy_to_user() copies "len" bytes from "tmp".
The preceding memcpy() filled exactly those bytes, so I'm not leaking
stack memory. And "len" is bounded to "namelen", which is bounded to
32, which is smaller than __NEW_UTS_LEN, so I'm not leaking memory
from outside the bounds of utsname()->domainname.
And the contents of the "struct new_utsname" are completely
user-accessible (including bytes behind null terminators); you can see
that e.g. sys_newuname() copies the whole struct to userspace; this
seems to be intentional, if you e.g. look at how sys_sethostname() is
implemented.
I went over this function with a fine comb and didn't spot anything wrong:

SYSCALL_DEFINE2(osf_getdomainname, char __user *, name, int, namelen)
{
        int len, err = 0;
        char *kname;
        char tmp[32];

        if (namelen < 0 || namelen > 32)
                namelen = 32;
        // namelen in range 0..32

        down_read(&uts_sem);
        kname = utsname()->domainname;
        // kname points to a 65-byte buffer that userspace is permitted to read
        len = strnlen(kname, namelen); // strnlen() is bounded to
first 32 bytes of 65-byte buffer
        // len is in range 0..32
        len = min(len + 1, namelen);
        // min([0..32] + 1, [0..32]) = min([1..33], [0..32]) is in range 0..32
        memcpy(tmp, kname, len); // writes up to `len` bytes into tmp;
len<=32, sizeof(tmp)==32; len<=32, sizeof(utsname()->domainname)>32
        // userspace is permitted to read first `len` bytes of `tmp`
        up_read(&uts_sem);

        if (copy_to_user(name, tmp, len)) // first `len` bytes of
`tmp` are exposed to userspace
                return -EFAULT;
        return 0;
}

Can you please explain why there is an infoleak here?
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings June 26, 2018, 5:43 p.m. UTC | #3 
On Mon, 2018-06-25 at 20:16 +0200, Jann Horn wrote:
> On Mon, Jun 25, 2018 at 6:41 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > 
> > On Mon, Jun 25, 2018 at 06:34:10PM +0200, Jann Horn wrote:
> > 
> > > +     char tmp[32];
> > > 
> > > -     if (namelen > 32)
> > > +     if (namelen < 0 || namelen > 32)
> > >               namelen = 32;
> > > 
> > >       down_read(&uts_sem);
> > >       kname = utsname()->domainname;
> > >       len = strnlen(kname, namelen);
> > > -     if (copy_to_user(name, kname, min(len + 1, namelen)))
> > > -             err = -EFAULT;
> > > +     len = min(len + 1, namelen);
> > > +     memcpy(tmp, kname, len);
> > >       up_read(&uts_sem);
> > > 
> > > -     return err;
> > > +     if (copy_to_user(name, tmp, len))
> > > +             return -EFAULT;
> > 
> > Infoleak, and similar in a lot of other places.
> 
> I don't see a problem. copy_to_user() copies "len" bytes from "tmp".
[...]
> Can you please explain why there is an infoleak here?

I think you're *fixing* information leaks in the Alpha syscalls,
because a negative value of namelen used to result in a huge length
argument to copy_to_user().

Ben.
Jann Horn June 27, 2018, 12:29 p.m. UTC | #4 
On Tue, Jun 26, 2018 at 7:43 PM Ben Hutchings <ben@decadent.org.uk> wrote:
>
> On Mon, 2018-06-25 at 20:16 +0200, Jann Horn wrote:
> > On Mon, Jun 25, 2018 at 6:41 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > >
> > > On Mon, Jun 25, 2018 at 06:34:10PM +0200, Jann Horn wrote:
> > >
> > > > +     char tmp[32];
> > > >
> > > > -     if (namelen > 32)
> > > > +     if (namelen < 0 || namelen > 32)
> > > >               namelen = 32;
> > > >
> > > >       down_read(&uts_sem);
> > > >       kname = utsname()->domainname;
> > > >       len = strnlen(kname, namelen);
> > > > -     if (copy_to_user(name, kname, min(len + 1, namelen)))
> > > > -             err = -EFAULT;
> > > > +     len = min(len + 1, namelen);
> > > > +     memcpy(tmp, kname, len);
> > > >       up_read(&uts_sem);
> > > >
> > > > -     return err;
> > > > +     if (copy_to_user(name, tmp, len))
> > > > +             return -EFAULT;
> > >
> > > Infoleak, and similar in a lot of other places.
> >
> > I don't see a problem. copy_to_user() copies "len" bytes from "tmp".
> [...]
> > Can you please explain why there is an infoleak here?
>
> I think you're *fixing* information leaks in the Alpha syscalls,
> because a negative value of namelen used to result in a huge length
> argument to copy_to_user().

Ah, you're right. Looks like this was previously fixed in commit
21c5977a836e ("alpha: fix several security issues", first in v3.0),
and then un-fixed in commit 9ba3eb5103cf ("osf_getdomainname(): use
copy_to_user()", first in v4.13).
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
index 6e921754c8fc..2a5c768df335 100644
--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c
@@ -530,24 +530,19 @@  SYSCALL_DEFINE4(osf_mount, unsigned long, typenr, const char __user *, path,
 SYSCALL_DEFINE1(osf_utsname, char __user *, name)
 {
 	int error;
+	char tmp[5 * 32];
 
 	down_read(&uts_sem);
-	error = -EFAULT;
-	if (copy_to_user(name + 0, utsname()->sysname, 32))
-		goto out;
-	if (copy_to_user(name + 32, utsname()->nodename, 32))
-		goto out;
-	if (copy_to_user(name + 64, utsname()->release, 32))
-		goto out;
-	if (copy_to_user(name + 96, utsname()->version, 32))
-		goto out;
-	if (copy_to_user(name + 128, utsname()->machine, 32))
-		goto out;
+	memcpy(tmp + 0 * 32, utsname()->sysname, 32);
+	memcpy(tmp + 1 * 32, utsname()->nodename, 32);
+	memcpy(tmp + 2 * 32, utsname()->release, 32);
+	memcpy(tmp + 3 * 32, utsname()->version, 32);
+	memcpy(tmp + 4 * 32, utsname()->machine, 32);
+	up_read(&uts_sem);
 
-	error = 0;
- out:
-	up_read(&uts_sem);	
-	return error;
+	if (copy_to_user(name, tmp, sizeof(tmp)))
+		return -EFAULT;
+	return 0;
 }
 
 SYSCALL_DEFINE0(getpagesize)
@@ -567,18 +562,21 @@  SYSCALL_DEFINE2(osf_getdomainname, char __user *, name, int, namelen)
 {
 	int len, err = 0;
 	char *kname;
+	char tmp[32];
 
-	if (namelen > 32)
+	if (namelen < 0 || namelen > 32)
 		namelen = 32;
 
 	down_read(&uts_sem);
 	kname = utsname()->domainname;
 	len = strnlen(kname, namelen);
-	if (copy_to_user(name, kname, min(len + 1, namelen)))
-		err = -EFAULT;
+	len = min(len + 1, namelen);
+	memcpy(tmp, kname, len);
 	up_read(&uts_sem);
 
-	return err;
+	if (copy_to_user(name, tmp, len))
+		return -EFAULT;
+	return 0;
 }
 
 /*
@@ -739,13 +737,14 @@  SYSCALL_DEFINE3(osf_sysinfo, int, command, char __user *, buf, long, count)
 	};
 	unsigned long offset;
 	const char *res;
-	long len, err = -EINVAL;
+	long len;
+	char tmp[__NEW_UTS_LEN + 1];
 
 	offset = command-1;
 	if (offset >= ARRAY_SIZE(sysinfo_table)) {
 		/* Digital UNIX has a few unpublished interfaces here */
 		printk("sysinfo(%d)", command);
-		goto out;
+		return -EINVAL;
 	}
 
 	down_read(&uts_sem);
@@ -753,13 +752,11 @@  SYSCALL_DEFINE3(osf_sysinfo, int, command, char __user *, buf, long, count)
 	len = strlen(res)+1;
 	if ((unsigned long)len > (unsigned long)count)
 		len = count;
-	if (copy_to_user(buf, res, len))
-		err = -EFAULT;
-	else
-		err = 0;
+	memcpy(tmp, res, len);
 	up_read(&uts_sem);
- out:
-	return err;
+	if (copy_to_user(buf, tmp, len))
+		return -EFAULT;
+	return 0;
 }
 
 SYSCALL_DEFINE5(osf_getsysinfo, unsigned long, op, void __user *, buffer,
diff --git a/arch/sparc/kernel/sys_sparc_32.c b/arch/sparc/kernel/sys_sparc_32.c
index 7f3d9c59719a..452e4d080855 100644
--- a/arch/sparc/kernel/sys_sparc_32.c
+++ b/arch/sparc/kernel/sys_sparc_32.c
@@ -197,23 +197,27 @@  SYSCALL_DEFINE5(rt_sigaction, int, sig,
 
 SYSCALL_DEFINE2(getdomainname, char __user *, name, int, len)
 {
- 	int nlen, err;
- 	
+	int nlen, err;
+	char tmp[__NEW_UTS_LEN + 1];
+
 	if (len < 0)
 		return -EINVAL;
 
- 	down_read(&uts_sem);
- 	
+	down_read(&uts_sem);
+
 	nlen = strlen(utsname()->domainname) + 1;
 	err = -EINVAL;
 	if (nlen > len)
-		goto out;
+		goto out_unlock;
+	memcpy(tmp, utsname()->domainname, nlen);
 
-	err = -EFAULT;
-	if (!copy_to_user(name, utsname()->domainname, nlen))
-		err = 0;
+	up_read(&uts_sem);
 
-out:
+	if (copy_to_user(name, tmp, nlen))
+		return -EFAULT;
+	return 0;
+
+out_unlock:
 	up_read(&uts_sem);
 	return err;
 }
diff --git a/arch/sparc/kernel/sys_sparc_64.c b/arch/sparc/kernel/sys_sparc_64.c
index 63baa8aa9414..274ed0b9b3e0 100644
--- a/arch/sparc/kernel/sys_sparc_64.c
+++ b/arch/sparc/kernel/sys_sparc_64.c
@@ -519,23 +519,27 @@  asmlinkage void sparc_breakpoint(struct pt_regs *regs)
 
 SYSCALL_DEFINE2(getdomainname, char __user *, name, int, len)
 {
-        int nlen, err;
+	int nlen, err;
+	char tmp[__NEW_UTS_LEN + 1];
 
 	if (len < 0)
 		return -EINVAL;
 
- 	down_read(&uts_sem);
- 	
+	down_read(&uts_sem);
+
 	nlen = strlen(utsname()->domainname) + 1;
 	err = -EINVAL;
 	if (nlen > len)
-		goto out;
+		goto out_unlock;
+	memcpy(tmp, utsname()->domainname, nlen);
+
+	up_read(&uts_sem);
 
-	err = -EFAULT;
-	if (!copy_to_user(name, utsname()->domainname, nlen))
-		err = 0;
+	if (copy_to_user(name, tmp, nlen))
+		return -EFAULT;
+	return 0;
 
-out:
+out_unlock:
 	up_read(&uts_sem);
 	return err;
 }
diff --git a/kernel/sys.c b/kernel/sys.c
index 38509dc1f77b..69b9a37ecf0d 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1237,18 +1237,19 @@  static int override_release(char __user *release, size_t len)
 
 SYSCALL_DEFINE1(newuname, struct new_utsname __user *, name)
 {
-	int errno = 0;
+	struct new_utsname tmp;
 
 	down_read(&uts_sem);
-	if (copy_to_user(name, utsname(), sizeof *name))
-		errno = -EFAULT;
+	memcpy(&tmp, utsname(), sizeof(tmp));
 	up_read(&uts_sem);
+	if (copy_to_user(name, &tmp, sizeof(tmp)))
+		return -EFAULT;
 
-	if (!errno && override_release(name->release, sizeof(name->release)))
-		errno = -EFAULT;
-	if (!errno && override_architecture(name))
-		errno = -EFAULT;
-	return errno;
+	if (override_release(name->release, sizeof(name->release)))
+		return -EFAULT;
+	if (override_architecture(name))
+		return -EFAULT;
+	return 0;
 }
 
 #ifdef __ARCH_WANT_SYS_OLD_UNAME
@@ -1257,55 +1258,46 @@  SYSCALL_DEFINE1(newuname, struct new_utsname __user *, name)
  */
 SYSCALL_DEFINE1(uname, struct old_utsname __user *, name)
 {
-	int error = 0;
+	struct old_utsname tmp;
 
 	if (!name)
 		return -EFAULT;
 
 	down_read(&uts_sem);
-	if (copy_to_user(name, utsname(), sizeof(*name)))
-		error = -EFAULT;
+	memcpy(&tmp, utsname(), sizeof(tmp));
 	up_read(&uts_sem);
+	if (copy_to_user(name, &tmp, sizeof(tmp)))
+		return -EFAULT;
 
-	if (!error && override_release(name->release, sizeof(name->release)))
-		error = -EFAULT;
-	if (!error && override_architecture(name))
-		error = -EFAULT;
-	return error;
+	if (override_release(name->release, sizeof(name->release)))
+		return -EFAULT;
+	if (override_architecture(name))
+		return -EFAULT;
+	return 0;
 }
 
 SYSCALL_DEFINE1(olduname, struct oldold_utsname __user *, name)
 {
-	int error;
+	struct oldold_utsname tmp = {};
 
 	if (!name)
 		return -EFAULT;
-	if (!access_ok(VERIFY_WRITE, name, sizeof(struct oldold_utsname)))
-		return -EFAULT;
 
 	down_read(&uts_sem);
-	error = __copy_to_user(&name->sysname, &utsname()->sysname,
-			       __OLD_UTS_LEN);
-	error |= __put_user(0, name->sysname + __OLD_UTS_LEN);
-	error |= __copy_to_user(&name->nodename, &utsname()->nodename,
-				__OLD_UTS_LEN);
-	error |= __put_user(0, name->nodename + __OLD_UTS_LEN);
-	error |= __copy_to_user(&name->release, &utsname()->release,
-				__OLD_UTS_LEN);
-	error |= __put_user(0, name->release + __OLD_UTS_LEN);
-	error |= __copy_to_user(&name->version, &utsname()->version,
-				__OLD_UTS_LEN);
-	error |= __put_user(0, name->version + __OLD_UTS_LEN);
-	error |= __copy_to_user(&name->machine, &utsname()->machine,
-				__OLD_UTS_LEN);
-	error |= __put_user(0, name->machine + __OLD_UTS_LEN);
+	memcpy(&tmp.sysname, &utsname()->sysname, __OLD_UTS_LEN);
+	memcpy(&tmp.nodename, &utsname()->nodename, __OLD_UTS_LEN);
+	memcpy(&tmp.release, &utsname()->release, __OLD_UTS_LEN);
+	memcpy(&tmp.version, &utsname()->version, __OLD_UTS_LEN);
+	memcpy(&tmp.machine, &utsname()->machine, __OLD_UTS_LEN);
 	up_read(&uts_sem);
+	if (copy_to_user(name, &tmp, sizeof(tmp)))
+		return -EFAULT;
 
-	if (!error && override_architecture(name))
-		error = -EFAULT;
-	if (!error && override_release(name->release, sizeof(name->release)))
-		error = -EFAULT;
-	return error ? -EFAULT : 0;
+	if (override_architecture(name))
+		return -EFAULT;
+	if (override_release(name->release, sizeof(name->release)))
+		return -EFAULT;
+	return 0;
 }
 #endif
 
@@ -1319,17 +1311,18 @@  SYSCALL_DEFINE2(sethostname, char __user *, name, int, len)
 
 	if (len < 0 || len > __NEW_UTS_LEN)
 		return -EINVAL;
-	down_write(&uts_sem);
 	errno = -EFAULT;
 	if (!copy_from_user(tmp, name, len)) {
-		struct new_utsname *u = utsname();
+		struct new_utsname *u;
 
+		down_write(&uts_sem);
+		u = utsname();
 		memcpy(u->nodename, tmp, len);
 		memset(u->nodename + len, 0, sizeof(u->nodename) - len);
 		errno = 0;
 		uts_proc_notify(UTS_PROC_HOSTNAME);
+		up_write(&uts_sem);
 	}
-	up_write(&uts_sem);
 	return errno;
 }
 
@@ -1337,8 +1330,9 @@  SYSCALL_DEFINE2(sethostname, char __user *, name, int, len)
 
 SYSCALL_DEFINE2(gethostname, char __user *, name, int, len)
 {
-	int i, errno;
+	int i;
 	struct new_utsname *u;
+	char tmp[__NEW_UTS_LEN + 1];
 
 	if (len < 0)
 		return -EINVAL;
@@ -1347,11 +1341,11 @@  SYSCALL_DEFINE2(gethostname, char __user *, name, int, len)
 	i = 1 + strlen(u->nodename);
 	if (i > len)
 		i = len;
-	errno = 0;
-	if (copy_to_user(name, u->nodename, i))
-		errno = -EFAULT;
+	memcpy(tmp, u->nodename, i);
 	up_read(&uts_sem);
-	return errno;
+	if (copy_to_user(name, tmp, i))
+		return -EFAULT;
+	return 0;
 }
 
 #endif
@@ -1370,17 +1364,18 @@  SYSCALL_DEFINE2(setdomainname, char __user *, name, int, len)
 	if (len < 0 || len > __NEW_UTS_LEN)
 		return -EINVAL;
 
-	down_write(&uts_sem);
 	errno = -EFAULT;
 	if (!copy_from_user(tmp, name, len)) {
-		struct new_utsname *u = utsname();
+		struct new_utsname *u;
 
+		down_write(&uts_sem);
+		u = utsname();
 		memcpy(u->domainname, tmp, len);
 		memset(u->domainname + len, 0, sizeof(u->domainname) - len);
 		errno = 0;
 		uts_proc_notify(UTS_PROC_DOMAINNAME);
+		up_write(&uts_sem);
 	}
-	up_write(&uts_sem);
 	return errno;
 }
 
diff --git a/kernel/utsname_sysctl.c b/kernel/utsname_sysctl.c
index 233cd8fc6910..258033d62cb3 100644
--- a/kernel/utsname_sysctl.c
+++ b/kernel/utsname_sysctl.c
@@ -18,7 +18,7 @@ 
 
 #ifdef CONFIG_PROC_SYSCTL
 
-static void *get_uts(struct ctl_table *table, int write)
+static void *get_uts(struct ctl_table *table)
 {
 	char *which = table->data;
 	struct uts_namespace *uts_ns;
@@ -26,21 +26,9 @@  static void *get_uts(struct ctl_table *table, int write)
 	uts_ns = current->nsproxy->uts_ns;
 	which = (which - (char *)&init_uts_ns) + (char *)uts_ns;
 
-	if (!write)
-		down_read(&uts_sem);
-	else
-		down_write(&uts_sem);
 	return which;
 }
 
-static void put_uts(struct ctl_table *table, int write, void *which)
-{
-	if (!write)
-		up_read(&uts_sem);
-	else
-		up_write(&uts_sem);
-}
-
 /*
  *	Special case of dostring for the UTS structure. This has locks
  *	to observe. Should this be in kernel/sys.c ????
@@ -50,13 +38,34 @@  static int proc_do_uts_string(struct ctl_table *table, int write,
 {
 	struct ctl_table uts_table;
 	int r;
+	char tmp_data[__NEW_UTS_LEN + 1];
+
 	memcpy(&uts_table, table, sizeof(uts_table));
-	uts_table.data = get_uts(table, write);
+	uts_table.data = tmp_data;
+
+	/*
+	 * Buffer the value in tmp_data so that proc_dostring() can be called
+	 * without holding any locks.
+	 * We also need to read the original value in the write==1 case to
+	 * support partial writes.
+	 */
+	down_read(&uts_sem);
+	memcpy(tmp_data, get_uts(table), sizeof(tmp_data));
+	up_read(&uts_sem);
 	r = proc_dostring(&uts_table, write, buffer, lenp, ppos);
-	put_uts(table, write, uts_table.data);
 
-	if (write)
+	if (write) {
+		/*
+		 * Write back the new value.
+		 * Note that, since we dropped uts_sem, the result can
+		 * theoretically be incorrect if there are two parallel writes
+		 * at non-zero offsets to the same sysctl.
+		 */
+		down_write(&uts_sem);
+		memcpy(get_uts(table), tmp_data, sizeof(tmp_data));
+		up_write(&uts_sem);
 		proc_sys_poll_notify(table->poll);
+	}
 
 	return r;
 }