linux-kernel - Q: sys_personality() && misc oddities

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 25 May 2010 16:17:20 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Andi Kleen <andi@...stfloor.org>, "H. Peter Anvin" <hpa@...or.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Richard Henderson <rth@...ddle.net>,
	Roland McGrath <roland@...hat.com>
Cc:	wezhang@...hat.com, linux-kernel@...r.kernel.org
Subject: 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.

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