staging: add Lustre file system client support

[Dan Carpenter]

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 at oracle.com> 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