lkml.org 
[lkml]   [2007]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] Fix blktrace setup 32-bit ioctl on 64-bit kernels
    Date
    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-10-03 11:37    [W:0.024 / U:3.620 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site