Q: sys_personality() && misc oddities

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.

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

Agreed.

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

Agreed.

No idea about that.

Thanks,
Roland

OK,

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

it is already 'unsigned int' ;)

I think the same.

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

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.

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.

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

Agreed.

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

Sure.

I can't see any sign that anybody cares.

Thanks,
Roland

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

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.

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.

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.

---

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

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

Of course! I agree very much.

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

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.

Yup. I'll take the blame.

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

Sure, will do tomorrow.

Oleg.

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

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.

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

---

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

@@ -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__ */
 

@@ -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,

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

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.

---

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

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

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

That would make most sense to me.

Feel free to add my

-hpa

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.

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.

---

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

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

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

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.

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.

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.

---

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

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