> 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
