Re: [PATCH 1/3] sys_personality: validate personality before set_personality()

[Oleg Nesterov <oleg@redhat.com>]

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.