Q: sys_personality() && misc oddities

Q: sys_personality() && misc oddities

[Oleg Nesterov]

Hello.

This code is really old, and I do not know whom should I ask. And,
despite the fact it is really trivial, I have no idea how to fix it.
And even more, I am not sure it actually needs fixes. I'd better ask
the questions. Please help ;)



First of all, task_struct->personality is "int", but sys_personality()
takes "long". This means that every comparison or assignment inside of
sys_personality() paths is not right.

Probably we need something like this trivial patch

	--- x/kernel/exec_domain.c
	+++ x/kernel/exec_domain.c
	@@ -192,7 +192,9 @@ SYSCALL_DEFINE1(personality, u_long, per
	 {
		u_long old = current->personality;
	
	-	if (personality != 0xffffffff) {
	+	if (personality > 0xffffffff)
	+		return -EINVAL;
	+	else if (personality != 0xffffffff) {
			set_personality(personality);
			if (current->personality != personality)
				return -EINVAL;

?

Or, perhaps we shouldn't allow personality >= int32_max = 0x7ffffff ?
Otherwise, on 32bit machine the value returned to the user-space can
look like -ESOMETHING.

Even on x86_64, in user-space it is declared as

	int personality(unsigned long persona);

if the kernel returns the "large" old it looks negative to the user-space,
and the test-case thinks that the syscall failed but errno is not set.

This is the actual reason for the question. I am really surprized I
do not know how to close the redhat-internal bugzilla entry, the
problem is very trivial.

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

But there are other oddities I can't understand. Let's forget about
the sizeof(task_struct->personality), let's suppose it is "long" too.
And note that is was long before 97dc32cdb1b53832801159d5f634b41aad9d0a23
which did s/long/int/ to reduce the sizeof task_struct.

__set_personality(). What is the point to check
ep == current_thread_info()->exec_domain ? This buys nothing afaics.
IOW, it could be simplified:

	int __set_personality(u_long personality)
	{
		struct exec_domain *oep = current_thread_info()->exec_domain;

		current_thread_info()->exec_domain = lookup_exec_domain(personality);
		current->personality = personality;
		module_put(oep->module);

		return 0;
	}

Now let's look at the caller, sys_personality()

	set_personality(personality);
	if (current->personality != personality)
		return -EINVAL;

but __set_personality() always sets current->personality = personality,
what is the point to check equality?

IOW, when we should return -EINVAL? Perhaps, lookup_exec_domain() should
return NULL instead of default_exec_domain when the search in exec_domains
fails? And probably we shouldn't change task->personality/exec_domain in
this case? It is really strange that sys_personality() can return -EINVAL
but change ->personality.

But this can probably break exec. alpha does set_personality(PER_OSF4)
but I do not see the corresponding register_exec_domain(). On the other
hand, I do not understand why it puts PER_OSF4 into PER_MASK. PER_OSF4
is only used by sys_osf_readv/sys_osf_writev.

Thanks,

Oleg.

Re: Q: sys_personality() && misc oddities

[Roland McGrath]

I don't have any special insight either, just poking around as you are.

I suspect that most of the logic and code in the kernel for this just
predates 64-bit stuff, and so was written with long==int and then maybe
tweaked slightly later.

I don't think we're ever going to need or want a 64-bit personality word.
(There are still 10 bits unused in the middle for more flags.)

Though the high bit might be set on 32-bit, there still should not really
be a danger of misinterpreting a value as an error code--as long as we
haven't used up all 10 of those middle bits.  The test userland (glibc)
uses is not "long < 0" but "u_long > -4095UL".  So as long as at least
one bit in 0xff00 remains clear, it won't match.

For 64-bit you want to avoid sign-extension of the old value just so it
looks valid (even though it won't look like an error code).  I think the
most sensible thing is to change the task_struct field to 'unsigned int'.
Then:

		u_long old = current->personality;

will do what:

		u_long old = (unsigned int) current->personality;

would do now, i.e. zero-extend rather than sign-extend.

I think the 0xffffffff check is intended specifically for personality(-1)
to be a no-op that returns the old value, and nothing more.  So I'd just
make that check be "((int) personality != -1)" instead.

OTOH, this:

		set_personality(personality);
		if (current->personality != personality)
			return -EINVAL;

will then both do the job in set_personality() and return -EINVAL, when
some high bits are set.  So, perhaps you are right about checking high
bits.  Then I'd make it:

	if ((int) personality != -1) {
		if (unlikely((unsigned int) personality != personality))
			return -EINVAL;
		set_personality(personality);
		if (current->personality != personality)
			return -EINVAL;
	}

> __set_personality(). What is the point to check
> ep == current_thread_info()->exec_domain ? This buys nothing afaics.
> IOW, it could be simplified:

Agreed.

> Now let's look at the caller, sys_personality()
> 
> 	set_personality(personality);
> 	if (current->personality != personality)
> 		return -EINVAL;
> 
> but __set_personality() always sets current->personality = personality,
> what is the point to check equality?

Good point.  I suspect that at some point in the past set_personality()
would sometimes refuse to make the change.

> IOW, when we should return -EINVAL? Perhaps, lookup_exec_domain() should
> return NULL instead of default_exec_domain when the search in exec_domains
> fails? And probably we shouldn't change task->personality/exec_domain in
> this case? It is really strange that sys_personality() can return -EINVAL
> but change ->personality.

Agreed.

> But this can probably break exec. alpha does set_personality(PER_OSF4)
> but I do not see the corresponding register_exec_domain(). On the other
> hand, I do not understand why it puts PER_OSF4 into PER_MASK. PER_OSF4
> is only used by sys_osf_readv/sys_osf_writev.

No idea about that.


Thanks,
Roland

Re: Q: sys_personality() && misc oddities

[Oleg Nesterov]

On 05/25, Roland McGrath wrote:
>
> I don't think we're ever going to need or want a 64-bit personality word.
> (There are still 10 bits unused in the middle for more flags.)

OK,

> Though the high bit might be set on 32-bit, there still should not really
> be a danger of misinterpreting a value as an error code--as long as we
> haven't used up all 10 of those middle bits.  The test userland (glibc)
> uses is not "long < 0" but "u_long > -4095UL".  So as long as at least
> one bit in 0xff00 remains clear, it won't match.

Yes, libc itself is fine. But from the application's pov, personality()
returns int, not long.

> For 64-bit you want to avoid sign-extension of the old value just so it
> looks valid (even though it won't look like an error code).  I think the
> most sensible thing is to change the task_struct field to 'unsigned int'.

it is already 'unsigned int' ;)

> I think the 0xffffffff check is intended specifically for personality(-1)
> to be a no-op that returns the old value, and nothing more.

I think the same.

> OTOH, this:
>
> 		set_personality(personality);
> 		if (current->personality != personality)
> 			return -EINVAL;
>
> will then both do the job in set_personality() and return -EINVAL, when
> some high bits are set.

Yes! and despite the fact it returns -EINVAL, current->personality was
changed. This can't be right.

> So, perhaps you are right about checking high
> bits.  Then I'd make it:
>
> 	if ((int) personality != -1) {
> 		if (unlikely((unsigned int) personality != personality))
> 			return -EINVAL;

Well. Think about personality(0xffffffff - 1). It passes both checks
and we change current->personality. Then the application calls
personality() again, we return the old value, and since the user-space
expects "int" it gets -2.

How about

	if (personality != 0xffffffff) {
		if (personality >= 0x7fffffff)
			return -EINVAL;
		set_personality(personality);
	}

? Now that personality always fits into "insigned int" we don't need
to recheck current->personality == personality, and "< 0x7fffffff"
gurantees that "int old_personality = personality(whatever)" in user
space can be never misinterpeted as error.

As for the other oddities, they need the separate patches. Or we can
just leave this code alone ;)

Oleg.

Re: Q: sys_personality() && misc oddities

[Roland McGrath]

> > Though the high bit might be set on 32-bit, there still should not really
> > be a danger of misinterpreting a value as an error code--as long as we
> > haven't used up all 10 of those middle bits.  The test userland (glibc)
> > uses is not "long < 0" but "u_long > -4095UL".  So as long as at least
> > one bit in 0xff00 remains clear, it won't match.
> 
> Yes, libc itself is fine. But from the application's pov, personality()
> returns int, not long.

That doesn't really matter to error/success ambiguity.  Since what I said
is true, it won't ever return exactly -1 for a non-error.  But even if it
did, the application can use errno=0;personality(x);errno!=0 checking.

> > For 64-bit you want to avoid sign-extension of the old value just so it
> > looks valid (even though it won't look like an error code).  I think the
> > most sensible thing is to change the task_struct field to 'unsigned int'.
> 
> it is already 'unsigned int' ;)

Ok, then there is no bug right now, is there?

> Yes! and despite the fact it returns -EINVAL, current->personality was
> changed. This can't be right.

Agreed.

> > So, perhaps you are right about checking high
> > bits.  Then I'd make it:
> >
> > 	if ((int) personality != -1) {
> > 		if (unlikely((unsigned int) personality != personality))
> > 			return -EINVAL;
> 
> Well. Think about personality(0xffffffff - 1). It passes both checks
> and we change current->personality. Then the application calls
> personality() again, we return the old value, and since the user-space
> expects "int" it gets -2.

Yes, it never really made any sense to me that it doesn't validate any of
the flag bits.

> How about
> 
> 	if (personality != 0xffffffff) {
> 		if (personality >= 0x7fffffff)
> 			return -EINVAL;
> 		set_personality(personality);
> 	}
> 
> ? Now that personality always fits into "insigned int" we don't need
> to recheck current->personality == personality, and "< 0x7fffffff"
> gurantees that "int old_personality = personality(whatever)" in user
> space can be never misinterpeted as error.

Sure.

> As for the other oddities, they need the separate patches. Or we can
> just leave this code alone ;)

I can't see any sign that anybody cares.


Thanks,
Roland

Re: Q: sys_personality() && misc oddities

[H. Peter Anvin]

On 05/26/2010 01:31 PM, Roland McGrath wrote:
>> As for the other oddities, they need the separate patches. Or we can
>> just leave this code alone ;)
> 
> I can't see any sign that anybody cares.

I certainly care, because we have had some very strange behavior with
weird personality bits in the past.  So cleaning this up would be good.

	-hpa

[PATCH 0/3] (Was: Q: sys_personality() && misc oddities)

[Oleg Nesterov]

On 05/26, Roland McGrath wrote:
>
> > Yes, libc itself is fine. But from the application's pov, personality()
> > returns int, not long.
>
> That doesn't really matter to error/success ambiguity.  Since what I said
> is true, it won't ever return exactly -1 for a non-error.  But even if it
> did, the application can use errno=0;personality(x);errno!=0 checking.

Agreed! to me this looks like the user-space bug, but there are people
who disagree.

Probably my initial email wasn't clear, I'll try to explain this better
in the changelog.

> > How about
> >
> > 	if (personality != 0xffffffff) {
> > 		if (personality >= 0x7fffffff)
> > 			return -EINVAL;
>
> Sure.

OK. Please see the patches.

	1/3 - obviously makes sense to me
	2/3 - not sure
	3/3 - simple cleanup, doesn't depend on 1-2

Oleg.

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

[Oleg Nesterov]

sys_personality(personality) is obviously wrong. It calls set_personality()
which always sets current->personality = personality and then does

	if (current->personality != personality)
		return -EINVAL;

If this "u_long" argument doesn't fit into "unsigned int" ->personality,
we return -EINVAL but change the caller's ->personality.

Move this check up to ensure the overflow is not possible, before calling
set_personality() which never fails.

Pointed-out-by: Wenming Zhang <wezhang@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/exec_domain.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- 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);
 	}
 
 	return (long)old;

[PATCH 2/3] sys_personality: make sure (int)personality >= 0

[Oleg Nesterov]

Not sure about this patch. The kernel/libc part is correct, but
since user-space declares "int personality(unsigned long persona)"
the current behaviour can confuse the (poor written) applications
even on 64-bit machines.

Consider:

	personality(0xffffffff - 1);	// == (int)-2

	...

	int ret = personality(0);	// returns the old personality
	if (ret < 0)
		oops_we_cant_set_PER_LINUX(errno);

And, since libc correctly detects the successful return from syscall,
errno is random.

Change sys_personality() to ensure personality can not look like a
negative int. This disallows the MSB, it is not used for PER_ flags.

Suggested-by: Wenming Zhang <wezhang@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/exec_domain.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- 34-rc1/kernel/exec_domain.c~2_MAKE_IT_POSITIVE	2010-05-27 15:15:12.000000000 +0200
+++ 34-rc1/kernel/exec_domain.c	2010-05-27 15:54:33.000000000 +0200
@@ -193,7 +193,8 @@ SYSCALL_DEFINE1(personality, u_long, per
 	u_long old = current->personality;
 
 	if (personality != 0xffffffff) {
-		if ((unsigned int)personality != personality)
+		/* ensure it never looks like a negative int to user-space */
+		if (personality > 0x7fffffff)
 			return -EINVAL;
 		set_personality(personality);
 	}

[PATCH 3/3] __set_personality: no need to check the old ->exec_domain

[Oleg Nesterov]

Cleanup. __set_personality() always changes ->exec_domain/personality,
the special case when ->exec_domain remains the same buys nothing but
complicates the code.

I don't understand why lookup_exec_domain() always succeeds even if
the search in ->exec_domains list fails, we use default_exec_domain
in this case.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/exec_domain.c |   17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

--- 34-rc1/kernel/exec_domain.c~3_CLEANUP_SET_PERSONALITY	2010-05-27 15:54:33.000000000 +0200
+++ 34-rc1/kernel/exec_domain.c	2010-05-27 17:03:29.000000000 +0200
@@ -134,23 +134,14 @@ unregister:
 	return 0;
 }
 
-int
-__set_personality(u_long personality)
+int __set_personality(u_long personality)
 {
-	struct exec_domain	*ep, *oep;
-
-	ep = lookup_exec_domain(personality);
-	if (ep == current_thread_info()->exec_domain) {
-		current->personality = personality;
-		module_put(ep->module);
-		return 0;
-	}
+	struct exec_domain *oep = current_thread_info()->exec_domain;
 
+	current_thread_info()->exec_domain = lookup_exec_domain(personality);
 	current->personality = personality;
-	oep = current_thread_info()->exec_domain;
-	current_thread_info()->exec_domain = ep;
-
 	module_put(oep->module);
+
 	return 0;
 }
 

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

[Linus Torvalds]

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

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

[Oleg Nesterov]

On 05/27, Linus Torvalds wrote:
>
> 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.

Of course! I agree very much.

> 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!

Certainly, this was my first thought.

But I didn't dare to do this change because it is obviously user-visible,
and while this is not very important, we should change the declaration
of personality() in /usr/include/sys/personality.h

> -SYSCALL_DEFINE1(personality, u_long, personality)
> +SYSCALL_DEFINE1(personality, unsigned int, personality)

Indeed!

But. Suppose an application does personality(0xffffffff << 32) on x86_64.

Before this patch we return -EINVAL (but wrongly change ->personality).
After this patch this is equal to personality(0), right?

If you think this is fine - I agree. In case we have a bug report we
know who should be blamed ;)

As for 2/3 - once again, I think this is user-space problem, but I
can't explain this to the bug-reportes.

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

You can also remove this "return -EINVAL", this is no longer possible.

Oleg.

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

[Linus Torvalds]

On Thu, 27 May 2010, Oleg Nesterov wrote:
> 
> But. Suppose an application does personality(0xffffffff << 32) on x86_64.
> 
> Before this patch we return -EINVAL (but wrongly change ->personality).
> After this patch this is equal to personality(0), right?

Yes. And I'm willing to take that "risk" in the name of not having to 
carry crazy stuff around in the kernel.

I don't think anybody does anything like that. At worst, they may have 
done sign-extension (due to lack of prototypes, whatever), and _wanted_ to 
do "personality(0xffffffff)" but instead ended up with the high bits set, 
and a non-working system call.

In which case the cleanup just helps such clueless cases. Not that I think 
those exist _either_, but whatever.

> If you think this is fine - I agree. In case we have a bug report we
> know who should be blamed ;)

Yup. I'll take the blame.

> You can also remove this "return -EINVAL", this is no longer possible.

I've already removed the patch from my tree, I wasn't going to commit it 
without somebody testing it. So maybe you could re-do the series with that 
cleanup too?

		Linus

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

[Oleg Nesterov]

On 05/27, Linus Torvalds wrote:
>
>
> On Thu, 27 May 2010, Oleg Nesterov wrote:
> >
> > You can also remove this "return -EINVAL", this is no longer possible.
>
> I've already removed the patch from my tree, I wasn't going to commit it
> without somebody testing it. So maybe you could re-do the series with that
> cleanup too?

Sure, will do tomorrow.

Oleg.

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

[Andi Kleen]

On Thu, May 27, 2010 at 10:51:46AM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 27 May 2010, Oleg Nesterov wrote:
> > 
> > But. Suppose an application does personality(0xffffffff << 32) on x86_64.
> > 
> > Before this patch we return -EINVAL (but wrongly change ->personality).
> > After this patch this is equal to personality(0), right?
> 
> Yes. And I'm willing to take that "risk" in the name of not having to 
> carry crazy stuff around in the kernel.

Perhaps we can have a personality with the old personality behaviour @) 

Just joking, I doubt anything really cares. I don't think personality
was used much in 64bit except for uname emulation.

-Andi

Re: [PATCH 2/3] sys_personality: make sure (int)personality >= 0

[H. Peter Anvin]

On 05/27/2010 08:36 AM, Oleg Nesterov wrote:
> Not sure about this patch. The kernel/libc part is correct, but
> since user-space declares "int personality(unsigned long persona)"
> the current behaviour can confuse the (poor written) applications
> even on 64-bit machines.
> 
> Consider:
> 
> 	personality(0xffffffff - 1);	// == (int)-2
> 
> 	...
> 
> 	int ret = personality(0);	// returns the old personality
> 	if (ret < 0)
> 		oops_we_cant_set_PER_LINUX(errno);
> 
> And, since libc correctly detects the successful return from syscall,
> errno is random.
> 
> Change sys_personality() to ensure personality can not look like a
> negative int. This disallows the MSB, it is not used for PER_ flags.
> 
> Suggested-by: Wenming Zhang <wezhang@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

I'm fine with this, even though it is indeed there to support extremely
poorly written applications (error is specifially -1, not < 0).

However, since we almost certainly have enough brokenness in here, and
since there definitely don't seem to be a whole lot of demand for new
personality bits, I'm more than happy to waste bit 31 at not having to
deal with it, ever.

However, it would be better if we returned -EINVAL on attempts to set
*any* of the reserved bits, not just bit 31.

	-hpa

Re: [PATCH 2/3] sys_personality: make sure (int)personality >= 0

[Oleg Nesterov]

On 05/27, H. Peter Anvin wrote:
>
> On 05/27/2010 08:36 AM, Oleg Nesterov wrote:
> >
> > Change sys_personality() to ensure personality can not look like a
> > negative int. This disallows the MSB, it is not used for PER_ flags.
> >
> I'm fine with this, even though it is indeed there to support extremely
> poorly written applications (error is specifially -1, not < 0).

Completely agreed. I never liked this patch, just tried to discuss this
"problem" and report either ACK or NACK back to bugzilla.

Now I dislike it even more. I am not going to resend it, but I added
the fat note to the patch-v2 I am sending.

> However, since we almost certainly have enough brokenness in here, and
> since there definitely don't seem to be a whole lot of demand for new
> personality bits, I'm more than happy to waste bit 31 at not having to
> deal with it, ever.
>
> However, it would be better if we returned -EINVAL on attempts to set
> *any* of the reserved bits, not just bit 31.

If only I knew what is the supposed behaviour of sys_personality ;)

Another reason to forget this patch but add the "right" check if needed.

Oleg.

[PATCH 0/2] sys_personality fixes v2

[Oleg Nesterov]

On 05/27, Linus Torvalds wrote:
>
> I've already removed the patch from my tree, I wasn't going to commit it
> without somebody testing it. So maybe you could re-do the series with that
> cleanup too?

I am just resending Linus's patch + another one with the minor cleanup.

1/2 lacks the "From: Linus" tag, but this is only because I'm afraid he
won't be proud by the changelog I wrote.

Oleg.

[PATCH 1/2] change sys_personality() to accept "unsigned int" instead of u_long

[Oleg Nesterov]

task_struct->pesonality is "unsigned int", but sys_personality() paths
use "unsigned long pesonality". This means that every assignment or
comparison is not right. In particular, if this argument does not fit
into "unsigned int" __set_personality() changes the caller's personality
and then sys_personality() returns -EINVAL.

Turn this argument into "unsigned int" and avoid overflows. Obviously,
this is the user-visible change, we just ignore the upper bits. But
this can't break the sane application.

There is another thing which can confuse the poorly written applications.
User-space thinks that this syscall returns int, not long. This means that
the returned value can be negative and look like the error code. But note
that libc won't be confused and thus errno won't be set, and with this
patch the user-space can never get -1 unless sys_personality() really fails.
And, most importantly, the negative RET != -1 is only possible if that app
previously called personality(RET).

Pointed-out-by: Wenming Zhang <wezhang@redhat.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

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

--- 34-rc1/include/linux/personality.h~1_CHANGE_SIGNATURE	2009-07-13 17:39:44.000000000 +0200
+++ 34-rc1/include/linux/personality.h	2010-05-28 17:07:03.000000000 +0200
@@ -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__ */
 
--- 34-rc1/include/linux/syscalls.h~1_CHANGE_SIGNATURE	2010-03-11 13:11:50.000000000 +0100
+++ 34-rc1/include/linux/syscalls.h	2010-05-28 17:07:03.000000000 +0200
@@ -308,7 +308,7 @@ asmlinkage long sys_capget(cap_user_head
 				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,
--- 34-rc1/kernel/exec_domain.c~1_CHANGE_SIGNATURE	2010-05-28 14:31:14.000000000 +0200
+++ 34-rc1/kernel/exec_domain.c	2010-05-28 18:23:25.000000000 +0200
@@ -27,7 +27,7 @@ static struct exec_domain *exec_domains 
 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_r
 }
 
 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);
+	unsigned int pers = personality(personality);
+	struct exec_domain *ep;
 
 	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(
 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, per
 			return -EINVAL;
 	}
 
-	return (long)old;
+	return old;
 }
 
 

[PATCH 2/2] remove the bogus checks in sys_personality()->__set_personality() path

[Oleg Nesterov]

Cleanup, no functional changes.

- __set_personality() always changes ->exec_domain/personality, the
  special case when ->exec_domain remains the same buys nothing but
  complicates the code. Unify both cases to simplify the code.

- The -EINVAL check in sys_personality() was never right. If we assume
  that set_personality() can fail we should check the value it returns
  instead of verifying that task->personality was actually changed.

  Remove it. Before the previous patch it was possible to hit this case
  due to overflow problems, but this -EINVAL just indicated the kernel
  bug.

OTOH, probably it makes sense to change lookup_exec_domain() to return
ERR_PTR() instead of default_exec_domain if the search in exec_domains
list fails, and report this error to the user-space. But this means
another user-space change, and we have in-kernel users which need fixes.
For example, PER_OSF4 falls into PER_MASK for unkown reason and nobody
cares to register this domain.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/exec_domain.c |   22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

--- 34-rc1/kernel/exec_domain.c~2_CLEANUP	2010-05-28 18:23:25.000000000 +0200
+++ 34-rc1/kernel/exec_domain.c	2010-05-28 20:07:48.000000000 +0200
@@ -134,23 +134,14 @@ unregister:
 	return 0;
 }
 
-int
-__set_personality(unsigned int personality)
+int __set_personality(unsigned int personality)
 {
-	struct exec_domain	*ep, *oep;
-
-	ep = lookup_exec_domain(personality);
-	if (ep == current_thread_info()->exec_domain) {
-		current->personality = personality;
-		module_put(ep->module);
-		return 0;
-	}
+	struct exec_domain *oep = current_thread_info()->exec_domain;
 
+	current_thread_info()->exec_domain = lookup_exec_domain(personality);
 	current->personality = personality;
-	oep = current_thread_info()->exec_domain;
-	current_thread_info()->exec_domain = ep;
-
 	module_put(oep->module);
+
 	return 0;
 }
 
@@ -192,11 +183,8 @@ SYSCALL_DEFINE1(personality, unsigned in
 {
 	unsigned int old = current->personality;
 
-	if (personality != 0xffffffff) {
+	if (personality != 0xffffffff)
 		set_personality(personality);
-		if (current->personality != personality)
-			return -EINVAL;
-	}
 
 	return old;
 }

Re: [PATCH 0/2] sys_personality fixes v2

[Linus Torvalds]

On Fri, 28 May 2010, Oleg Nesterov wrote:
> 
> I am just resending Linus's patch + another one with the minor cleanup.
> 
> 1/2 lacks the "From: Linus" tag, but this is only because I'm afraid he
> won't be proud by the changelog I wrote.

Ack to both, and I don't care if the first one gets put down as mine or 
not, it's obviously trivial.

Who wants to merge them? Do I just take them directly, or what?

			Linus

Re: [PATCH 0/2] sys_personality fixes v2

[H. Peter Anvin]

On 05/28/2010 12:28 PM, Linus Torvalds wrote:
> 
> 
> On Fri, 28 May 2010, Oleg Nesterov wrote:
>>
>> I am just resending Linus's patch + another one with the minor cleanup.
>>
>> 1/2 lacks the "From: Linus" tag, but this is only because I'm afraid he
>> won't be proud by the changelog I wrote.
> 
> Ack to both, and I don't care if the first one gets put down as mine or 
> not, it's obviously trivial.
> 
> Who wants to merge them? Do I just take them directly, or what?
> 

That would make most sense to me.

Feel free to add my

Acked-by: H. Peter Anvin <hpa@zytor.com>

	-hpa

Re: [PATCH 0/2] sys_personality fixes v2

[Oleg Nesterov]

On 05/28, Linus Torvalds wrote:
>
> Ack to both, and I don't care if the first one gets put down as mine or
> not, it's obviously trivial.
>
> Who wants to merge them? Do I just take them directly, or what?

I meant to ask Andrew to take them, the fix is not urgent.

On the other hand they are very trivial, I just do not know
what will be more convenient to you and Andrew.

Oleg.