Re: [PATCH] Fix blktrace setup 32-bit ioctl on 64-bit kernels

[Arnd Bergmann <arnd@arndb.de>]

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 <><