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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/