linux-kernel - Re: 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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 25 May 2010 12:33:48 -0700 (PDT)
From:	Roland McGrath <roland@...hat.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Andi Kleen <andi@...stfloor.org>, "H. Peter Anvin" <hpa@...or.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Richard Henderson <rth@...ddle.net>, wezhang@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: Q: sys_personality() && misc oddities

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