On Thu, Apr 24, 2014 at 11:14:46AM +0800, Peng Tao wrote:
> Hi Dan,
>
> Thanks for reporting this.
>
> On Wed, Apr 23, 2014 at 10:13 PM, Dan Carpenter
> <dan.carpenter@xxxxxxxxxx> wrote:
> > Btw, what's the trick to navigating the lustre source? I normally do
> > a make cscope but that doesn't work and I am having a very hard time
> > with this code.
> >
> I use cscope + ctags to navigate the lustre source.
How do you build your cscope database? The kernel has a "make cscope"
function but since "make drivers/staging/lustre/lustre/osc/lproc_osc.i"
doesn't work then building cscope doesn't work either.
I don't know enough about the kernel build system to make this work.
> >> 195 if ((__u32)libcfs_ioctl_packlen(data) != data->ioc_len ) {
> >> 196 CERROR ("LIBCFS ioctl: packlen != ioc_len\n");
> >>
> >> The idea was this would check it but this broken because we check
> >> data->ioc_len in obd_ioctl_getdata() and then we do a second copy from
> >> the user so the current value of ioc_len is unchecked.
> So it is more of a security problem because you are considering users
> modifying its buffer during an ioctl call, is it correct?
Yes.
>
> >>
> >> 197 return 1;
> >> 198 }
> >> 199 if (data->ioc_inllen1 &&
> >> 200 data->ioc_bulk[data->ioc_inllen1 - 1] != '\0') {
> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >>
> >> But data is 1024 byte struct with 896 bytes dedicated for ->ioc_bulk[]
> Why?
> We currently have
> #define CONFIG_LUSTRE_OBD_MAX_IOCTL_BUFFER 8192
> and clearly it is configurable.
Oops. I see now that I said the caller was obd_ioctl_getdata() but I
meant libcfs_ioctl_getdata(). Sorry for the confusion.
This comes from libcfs_ioctl() in
drivers/staging/lustre/lustre/libcfs/module.c where it allocates like
this:
290 static int libcfs_ioctl(struct cfs_psdev_file *pfile, unsigned long cmd, void *arg)
291 {
292 char *buf;
293 struct libcfs_ioctl_data *data;
294 int err = 0;
295
296 LIBCFS_ALLOC_GFP(buf, 1024, GFP_IOFS);
^^^^
We allocate 1k.
297 if (buf == NULL)
298 return -ENOMEM;
299
300 /* 'cmd' and permissions get checked in our arch-specific caller */
301 if (libcfs_ioctl_getdata(buf, buf + 800, (void *)arg)) {
^^^^^^^^^
But we only ever use the first 800 bytes. The libcfs_ioctl_getdata()
function should really take a size parameter instead of an *end pointer.
Everyone gets start/end calculations off-by-one because of the fence
post problem.
302 CERROR("PORTALS ioctl: data error\n");
303 GOTO(out, err = -EINVAL);
304 }
305 data = (struct libcfs_ioctl_data *)buf;
306
307 err = libcfs_ioctl_int(pfile, cmd, arg, data);
>
> >> so we are reading far beyond on the end of the array here. (Can cause
> >> an oops).
> >>
> >> The caller is obd_ioctl_getdata() and data->ioc_inllen1 comes directly
> >> from the user. If we added this in obd_ioctl_getdata() then it would
> >> fix the bug:
> >>
> >> orig_len = hdr->ioc_len;
> >> if (copy_from_user(buf, (void *)arg, hdr->ioc_len))
> >> return -EFAULT; /* the original return code is buggy */
> YES! We certainly cannot return copy_from_user()'s return value.
>
> >> if (orig_len != hdr->ioc_len)
> >> return -EINVAL;
> >>
> And no... the above code snip does not work as expected...
> hdr->ioc_len does not get modified in the copy_from_user() and then it
> is just comparing with its own copy. You should compare data->ioc_len
> instead. Care to revise and send a patch?
Sorry, this is again my mistake for geting libcfs_ioctl_getdata()
and obd_ioctl_getdata() mixed up. My patch works for the libcfs
version.
>
> >> if (libcfs_ioctl_is_invalid(data)) {
> >>
> >> Why do we even have all the "> (1<<30)" checks? I don't understand.
> >> Anything over 1024 is invalid.
> >>
> I believe it is just a safe keeper. Anything that large is certainly
> wrong.
Ah. It prevents integer overflow bugs in libcfs_ioctl_packlen().
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
