Re: [patch 19/24] TASK_SIZE is variable.

[Andi Kleen <ak@suse.de>]

> int compat_sys_foo(compat_uptr_t u_buf, compat_uptr_t u_ret_val)
> {
> 	const char __user *buf = compat_ptr(u_buf);
> 	unsigned long k_val;
> 	mm_segment_t old_fs = get_fs();
> 	int err;
> 
> 	set_fs(KERNEL_DS);
> 	err = sys_foo(buf, (unsigned long __user *) &k_val);
>  ...
> 
> This does not fault on x86_64, but it does on platforms like sparc64.

Actually it faults on UML/x86-64 too :)

> Even though it doesn't fault on x86_64, it's a security hole because it
> allows the user to pass in kernel addresses, and such kernel addresses
> will just work since we're in KERNEL_DS.

the caller just has to verify_area() everything. Not doing that
would be a security hole yes.

One of the reason I think it's a good idea to discourage because
driver writers often get this detail wrong.

But even with all that compat code going away set_fs will stay:
there are places like network IO in kernel etc. where there
is just no way around it.

> If set_fs() updated some mm->max_addr thing, access_ok() and friends
> would trap things like this in software even on x86_64.  Therefore,
> I think if anything it's a very good bug check.

Hmm, I don't see what change it would make. Currently in 
KERNEL_DS access_ok is always true.  I don't see how you can change
this without breaking everything? 

In theory you could make it check for user space addresses and
then fail on i386/x86-64 ((addr) >= TASK_SIZE), but that would
bloat the code generated by this common macro a lot and it's probably
not worth it. But mm->max_addr wouldn't help you with this at all,
you would need a new mm->min_addr which I didn't think anybody
was proposing.

-Andi