Re: [PATCH 1/3] sys_personality: validate personality before set_personality()

[Linus Torvalds <torvalds@linux-foundation.org>]

On Thu, 27 May 2010, Oleg Nesterov wrote:
> 
> --- 34-rc1/kernel/exec_domain.c~1_CK_OVERFLOW_EARLIER	2009-04-06 00:03:42.000000000 +0200
> +++ 34-rc1/kernel/exec_domain.c	2010-05-27 15:15:12.000000000 +0200
> @@ -193,9 +193,9 @@ SYSCALL_DEFINE1(personality, u_long, per
>  	u_long old = current->personality;
>  
>  	if (personality != 0xffffffff) {
> -		set_personality(personality);
> -		if (current->personality != personality)
> +		if ((unsigned int)personality != personality)
>  			return -EINVAL;
> +		set_personality(personality);
>  	}

I think this is total random noise. The whole type system is crazy - don't 
just paper over it.

The thing shouldn't use "u_long" in the first place. That's a totally 
bogus type to start with (hint: do a "git grep u_long" and see that most 
of them are in badly done drivers, and the _only_ hits inside kernel/ are 
in that broken personality handling)

And if we decide that the field must fit in an unsigned int (reasonable), 
then let's just ignore the top bits, and make it work right even if 
somebody passes in an unsigned int!

IOW, we should just clean the mess up entirely. Like the appended, I 
think.

NOTE! Untested, of course. But our whole system call infrastructure should 
mean that on architectures like 64-bit powerpc that needs wrappers to make 
sure that 32-bit arguments are properly sign-extended, this should all 
work fine. We'll only look at the low bits.

(That "ident_map" should be "unsigned int" or even "unsigned char", but 
that's a separate thing. I just got rid of the bogus "u_long" crap).

		Linus

---
 include/linux/personality.h |    2 +-
 include/linux/syscalls.h    |    2 +-
 kernel/exec_domain.c        |   18 +++++++++---------
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/linux/personality.h b/include/linux/personality.h
index 1261208..eec3bae 100644
--- a/include/linux/personality.h
+++ b/include/linux/personality.h
@@ -12,7 +12,7 @@ struct pt_regs;
 
 extern int		register_exec_domain(struct exec_domain *);
 extern int		unregister_exec_domain(struct exec_domain *);
-extern int		__set_personality(unsigned long);
+extern int		__set_personality(unsigned int);
 
 #endif /* __KERNEL__ */
 
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 057929b..7a858d6 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -312,7 +312,7 @@ asmlinkage long sys_capget(cap_user_header_t header,
 				cap_user_data_t dataptr);
 asmlinkage long sys_capset(cap_user_header_t header,
 				const cap_user_data_t data);
-asmlinkage long sys_personality(u_long personality);
+asmlinkage long sys_personality(unsigned int personality);
 
 asmlinkage long sys_sigpending(old_sigset_t __user *set);
 asmlinkage long sys_sigprocmask(int how, old_sigset_t __user *set,
diff --git a/kernel/exec_domain.c b/kernel/exec_domain.c
index c35452c..e016d3c 100644
--- a/kernel/exec_domain.c
+++ b/kernel/exec_domain.c
@@ -27,7 +27,7 @@ static struct exec_domain *exec_domains = &default_exec_domain;
 static DEFINE_RWLOCK(exec_domains_lock);
 
 
-static u_long ident_map[32] = {
+static unsigned long ident_map[32] = {
 	0,	1,	2,	3,	4,	5,	6,	7,
 	8,	9,	10,	11,	12,	13,	14,	15,
 	16,	17,	18,	19,	20,	21,	22,	23,
@@ -56,10 +56,10 @@ default_handler(int segment, struct pt_regs *regp)
 }
 
 static struct exec_domain *
-lookup_exec_domain(u_long personality)
+lookup_exec_domain(unsigned int personality)
 {
-	struct exec_domain *	ep;
-	u_long			pers = personality(personality);
+	struct exec_domain *ep;
+	unsigned int pers = personality(personality);
 
 	read_lock(&exec_domains_lock);
 	for (ep = exec_domains; ep; ep = ep->next) {
@@ -70,7 +70,7 @@ lookup_exec_domain(u_long personality)
 
 #ifdef CONFIG_MODULES
 	read_unlock(&exec_domains_lock);
-	request_module("personality-%ld", pers);
+	request_module("personality-%d", pers);
 	read_lock(&exec_domains_lock);
 
 	for (ep = exec_domains; ep; ep = ep->next) {
@@ -135,7 +135,7 @@ unregister:
 }
 
 int
-__set_personality(u_long personality)
+__set_personality(unsigned int personality)
 {
 	struct exec_domain	*ep, *oep;
 
@@ -188,9 +188,9 @@ static int __init proc_execdomains_init(void)
 module_init(proc_execdomains_init);
 #endif
 
-SYSCALL_DEFINE1(personality, u_long, personality)
+SYSCALL_DEFINE1(personality, unsigned int, personality)
 {
-	u_long old = current->personality;
+	unsigned int old = current->personality;
 
 	if (personality != 0xffffffff) {
 		set_personality(personality);
@@ -198,7 +198,7 @@ SYSCALL_DEFINE1(personality, u_long, personality)
 			return -EINVAL;
 	}
 
-	return (long)old;
+	return old;
 }