On Tuesday 02 October 2007, Jens Axboe wrote:
> Hi Arnd,
>
> Updated patch below. I kept the code in compat_ioctl.c, to me it seems
> like the cleanest approach. I need the BLKTRACESETUP32 define both in
> compat_ioctl.c and blktrace.c if I move it, and I need to hard-core the
> struct size or define it in both places. And guard the code in
> blktrace.c with an ifdef for CONFIG_COMPAT. Not pretty, imho.
>
> I haven't tested this one yet, but at least it compiles and the sizing
> seems right. The u16 padding was an artifact of the
> __attribute__((packed)) so that could be removed.
The sizes are ok now, but I still don't like the idea of adding more
stuff to fs/compat_ioctl.c. I also noticed another problem now, see below.
The preferred way to define compat_ioctl handlers is to use a ->compat_ioctl
file operation so you don't need any code in compat_ioctl.c at all.
You still need the #ifdef in blktrace.c though if you want to building extra
code on the architectures that don't need it.
> +static int blktrace32_setup(int fd, unsigned cmd, unsigned long arg)
> +{
> + struct blk_user_trace_setup __user *buts =
> compat_alloc_user_space(sizeof(*buts));
> + struct blk_user_trace_setup32 __user *buts32 = compat_ptr(arg);
> + int err;
> +
> + if (copy_in_user(&buts->name, &buts32->name, 32) ||
> + get_user(buts->act_mask, &buts32->act_mask) ||
> + get_user(buts->buf_size, &buts32->buf_size) ||
> + get_user(buts->buf_nr, &buts32->buf_nr) ||
> + get_user(buts->start_lba, &buts32->start_lba) ||
> + get_user(buts->end_lba, &buts32->end_lba) ||
> + get_user(buts->pid, &buts32->pid))
> + return -EFAULT;
You are dereferencing 'buts' here, which is a user space pointer. This is
broken and cannot work on architectures that have split kernel/user address
spaces, and a potential security hole on those that don't.
sparse would warn about this kind of bug, but of course one of the problems
with fs/compat_ioctl.c is that it isn't sparse clean in the first place.
> + err = sys_ioctl(fd, cmd, (unsigned long) buts);
> + if (err)
> + return err;
> +
> + if (copy_to_user(&buts32->name, &buts->name, 32))
> + return -EFAULT;
Same here, this needs to be copy_in_user.
Arnd <><
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
