linux-kernel - Re: [PATCH 1/3] sys_personality: validate personality before set

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-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 27 May 2010 19:15:30 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Roland McGrath <roland@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andi Kleen <andi@...stfloor.org>,
	"H. Peter Anvin" <hpa@...or.com>,
	Richard Henderson <rth@...ddle.net>, wezhang@...hat.com,
	linux-kernel@...r.kernel.org,
	Michael Kerrisk <mtk.manpages@...il.com>,
	William Cohen <wcohen@...hat.com>
Subject: Re: [PATCH 1/3] sys_personality: validate personality before
	set_personality()

On 05/27, Linus Torvalds wrote:
>
> On Thu, 27 May 2010, Oleg Nesterov wrote:
> >
> > --- 34-rc1/kernel/exec_domain.c~1_CK_OVERFLOW_EARLIER	2009-04-06 00:03:42.000000000 +0200
> > +++ 34-rc1/kernel/exec_domain.c	2010-05-27 15:15:12.000000000 +0200
> > @@ -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);
> >  	}
>
> I think this is total random noise. The whole type system is crazy - don't
> just paper over it.

Of course! I agree very much.

> And if we decide that the field must fit in an unsigned int (reasonable),
> then let's just ignore the top bits, and make it work right even if
> somebody passes in an unsigned int!

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

> -SYSCALL_DEFINE1(personality, u_long, personality)
> +SYSCALL_DEFINE1(personality, unsigned int, personality)

Indeed!

But. Suppose an application does personality(0xffffffff << 32) on x86_64.

Before this patch we return -EINVAL (but wrongly change ->personality).
After this patch this is equal to personality(0), right?

If you think this is fine - I agree. In case we have a bug report we
know who should be blamed ;)

As for 2/3 - once again, I think this is user-space problem, but I
can't explain this to the bug-reportes.

> -	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, personality)
>  			return -EINVAL;

You can also remove this "return -EINVAL", this is no longer possible.

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/