Re: [RFC][PATCH] utsname: completely overwrite prior information

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 12 Sep 2008 22:36:24 +0200
Vegard Nossum <vegard.nossum@gmail.com> wrote:

> >From 25c69de4760e20cf7562cf92a55b65a71546093e Mon Sep 17 00:00:00 2001
> From: Vegard Nossum <vegard.nossum@gmail.com>
> Date: Thu, 11 Sep 2008 19:59:50 +0200
> Subject: [PATCH] utsname: completely overwrite prior information
> 
> On sethostname() and setdomainname(), previous information may be
> retained if it was longer than than the new hostname/domainname.
> 
> This can be demonstrated trivially by calling sethostname() first
> with a long name, then with a short name, and then calling uname()
> to retrieve the full buffer that contains the hostname (and
> possibly parts of the old hostname), one just has to look past the
> terminating zero.
> 
> I don't know if we should really care that much (hence the RFC);
> the only scenarios I can possibly think of is administrator
> putting something sensitive in the hostname (or domain name) by
> accident, and changing it back will not undo the mistake entirely,
> though it's not like we can recover gracefully from "rm -rf /"
> either... The other scenario is namespaces (CLONE_NEWUTS) where
> some information may be unintentionally "inherited" from the
> previous namespace (a program wants to hide the original name and
> does clone + sethostname, but some information is still left).
> 
> I think the patch may be defended on grounds of the principle of
> least surprise. But I am not adamant :-)
> 
> (I guess the question now is whether userspace should be able to
> write embedded NULs into the buffer or not...)
> 
> At least the observation has been made and the patch has been
> presented.
> 

fair enuf.  Did you check allthe other fields in 'struct new_utsname'?

> index 038a7bc..78b4515 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1352,7 +1352,8 @@ asmlinkage long sys_sethostname(char __user *name, int len)
>  	errno = -EFAULT;
>  	if (!copy_from_user(tmp, name, len)) {
>  		memcpy(utsname()->nodename, tmp, len);
> -		utsname()->nodename[len] = 0;
> +		memset(utsname()->nodename + len, 0,
> +			sizeof(utsname()->nodename) - len);

We could do the memset before the memcpy.  It's more work, but less
text.  Whatever.


While we're there, the code generation in there is a bit sloppy.  How's
this look?


From: Andrew Morton <akpm@linux-foundation.org>

utsname() is quite expensive to calculate.  Cache it in a local.

          text    data     bss     dec     hex filename
before:  11136     720      16   11872    2e60 kernel/sys.o
after:   11096     720      16   11832    2e38 kernel/sys.o

Cc: Vegard Nossum <vegard.nossum@gmail.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: "Serge E. Hallyn" <serue@us.ibm.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/sys.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff -puN kernel/sys.c~kernel-sysc-improve-code-generation kernel/sys.c
--- a/kernel/sys.c~kernel-sysc-improve-code-generation
+++ a/kernel/sys.c
@@ -1415,9 +1415,10 @@ asmlinkage long sys_sethostname(char __u
 	down_write(&uts_sem);
 	errno = -EFAULT;
 	if (!copy_from_user(tmp, name, len)) {
-		memcpy(utsname()->nodename, tmp, len);
-		memset(utsname()->nodename + len, 0,
-			sizeof(utsname()->nodename) - len);
+		struct new_utsname *u = utsname();
+
+		memcpy(u->nodename, tmp, len);
+		memset(u->nodename + len, 0, sizeof(u->nodename) - len);
 		errno = 0;
 	}
 	up_write(&uts_sem);
@@ -1429,15 +1430,17 @@ asmlinkage long sys_sethostname(char __u
 asmlinkage long sys_gethostname(char __user *name, int len)
 {
 	int i, errno;
+	struct new_utsname *u;
 
 	if (len < 0)
 		return -EINVAL;
 	down_read(&uts_sem);
-	i = 1 + strlen(utsname()->nodename);
+	u = utsname();
+	i = 1 + strlen(u->nodename);
 	if (i > len)
 		i = len;
 	errno = 0;
-	if (copy_to_user(name, utsname()->nodename, i))
+	if (copy_to_user(name, u->nodename, i))
 		errno = -EFAULT;
 	up_read(&uts_sem);
 	return errno;
@@ -1462,9 +1465,10 @@ asmlinkage long sys_setdomainname(char _
 	down_write(&uts_sem);
 	errno = -EFAULT;
 	if (!copy_from_user(tmp, name, len)) {
-		memcpy(utsname()->domainname, tmp, len);
-		memset(utsname()->domainname + len, 0,
-			sizeof(utsname()->domainname) - len);
+		struct new_utsname *u = utsname();
+
+		memcpy(u->domainname, tmp, len);
+		memset(u->domainname + len, 0, sizeof(u->domainname) - len);
 		errno = 0;
 	}
 	up_write(&uts_sem);
_