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]

Message-Id: <20100526203105.59D7849A56@magilla.sf.frob.com>
Date:	Wed, 26 May 2010 13:31:05 -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

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