[PATCH 4.4 33/47] sys: dont hold uts_sem while accessing userspace memory

From: Greg Kroah-Hartman
Date: Fri Sep 07 2018 - 17:41:10 EST

Next message: Greg Kroah-Hartman: "[PATCH 4.4 34/47] userns: move user access out of the mutex"
Previous message: Greg Kroah-Hartman: "[PATCH 4.4 32/47] osf_getdomainname(): use copy_to_user()"
In reply to: Greg Kroah-Hartman: "[PATCH 4.4 32/47] osf_getdomainname(): use copy_to_user()"
Next in thread: Greg Kroah-Hartman: "[PATCH 4.4 34/47] userns: move user access out of the mutex"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

4.4-stable review patch. If anyone has any objections, please let me know.

------------------

From: Jann Horn <jannh@xxxxxxxxxx>

commit 42a0cc3478584d4d63f68f2f5af021ddbea771fa upstream.

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@xxxxxxxxxxxxxxx
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
Signed-off-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>

---
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(-)

--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c
@@ -526,24 +526,19 @@ SYSCALL_DEFINE4(osf_mount, unsigned long
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)
@@ -563,18 +558,21 @@ SYSCALL_DEFINE2(osf_getdomainname, char
{
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;
}

/*
@@ -736,13 +734,14 @@ SYSCALL_DEFINE3(osf_sysinfo, int, comman
};
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);
@@ -750,13 +749,11 @@ SYSCALL_DEFINE3(osf_sysinfo, int, comman
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,
--- a/arch/sparc/kernel/sys_sparc_32.c
+++ b/arch/sparc/kernel/sys_sparc_32.c
@@ -201,23 +201,27 @@ SYSCALL_DEFINE5(rt_sigaction, int, sig,

asmlinkage long sys_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;
}
--- a/arch/sparc/kernel/sys_sparc_64.c
+++ b/arch/sparc/kernel/sys_sparc_64.c
@@ -524,23 +524,27 @@ extern void check_pending(int signum);

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;
}
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1142,18 +1142,19 @@ static int override_release(char __user

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
@@ -1162,55 +1163,46 @@ SYSCALL_DEFINE1(newuname, struct new_uts
*/
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

@@ -1224,17 +1216,18 @@ SYSCALL_DEFINE2(sethostname, char __user

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;
}

@@ -1242,8 +1235,9 @@ SYSCALL_DEFINE2(sethostname, char __user

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;
@@ -1252,11 +1246,11 @@ SYSCALL_DEFINE2(gethostname, char __user
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
@@ -1275,17 +1269,18 @@ SYSCALL_DEFINE2(setdomainname, char __us
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;
}

--- a/kernel/utsname_sysctl.c
+++ b/kernel/utsname_sysctl.c
@@ -17,7 +17,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;
@@ -25,21 +25,9 @@ static void *get_uts(struct ctl_table *t
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 ????
@@ -49,13 +37,34 @@ static int proc_do_uts_string(struct ctl
{
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;
}

Next message: Greg Kroah-Hartman: "[PATCH 4.4 34/47] userns: move user access out of the mutex"
Previous message: Greg Kroah-Hartman: "[PATCH 4.4 32/47] osf_getdomainname(): use copy_to_user()"
In reply to: Greg Kroah-Hartman: "[PATCH 4.4 32/47] osf_getdomainname(): use copy_to_user()"
Next in thread: Greg Kroah-Hartman: "[PATCH 4.4 34/47] userns: move user access out of the mutex"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]