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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200710031134.56940.arnd@arndb.de>
Date:	Wed, 3 Oct 2007 11:34:56 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Jens Axboe <jens.axboe@...cle.com>
Cc:	linux-kernel@...r.kernel.org, abhishekrai@...gle.com,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	davem@...emloft.net
Subject: Re: [PATCH] Fix blktrace setup 32-bit ioctl on 64-bit kernels

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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/