Re: staging: add Lustre file system client support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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. I guess you hit
some dead corners mainly on macros like
vim drivers/staging/lustre/lustre/include/obd_class.h +323
 323 #define OBT(dev)        (dev)->obd_type
 324 #define OBP(dev, op)    (dev)->obd_type->typ_dt_ops->o_ ## op
 325 #define MDP(dev, op)    (dev)->obd_type->typ_md_ops->m_ ## op
 326 #define CTXTP(ctxt, op) (ctxt)->loc_logops->lop_##op

These macros did hit me as well when I first started reading the
lustre code. I guess we should get rid of them somehow in the end, is
it correct Andreas and Oleg?

> regards,
> dan carpenter
>
> On Wed, Apr 23, 2014 at 04:54:26PM +0300, Dan Carpenter wrote:
>> Hello Peng Tao,
>>
>> The patch d7e09d0397e8: "staging: add Lustre file system client
>> support" from May 2, 2013, leads to the following static checker
>> warning:
>>
>>       drivers/staging/lustre/include//linux/libcfs/libcfs_ioctl.h:200 libcfs_ioctl_is_invalid()
>>       error: buffer overflow 'data->ioc_bulk' 896 <= 1073741823
>>
>> drivers/staging/lustre/include//linux/libcfs/libcfs_ioctl.h
>>    157  static inline int libcfs_ioctl_is_invalid(struct libcfs_ioctl_data *data)
>>    158  {
>>    159          if (data->ioc_len > (1<<30)) {
>>    160                  CERROR ("LIBCFS ioctl: ioc_len larger than 1<<30\n");
>>    161                  return 1;
>>    162          }
>>    163          if (data->ioc_inllen1 > (1<<30)) {
>>    164                  CERROR ("LIBCFS ioctl: ioc_inllen1 larger than 1<<30\n");
>>
>> We limit data->ioc_inllen1 to 1073741823 bytes here.
>>
>>    165                  return 1;
>>    166          }
>>    167          if (data->ioc_inllen2 > (1<<30)) {
>>    168                  CERROR ("LIBCFS ioctl: ioc_inllen2 larger than 1<<30\n");
>>    169                  return 1;
>>    170          }
>>    171          if (data->ioc_inlbuf1 && !data->ioc_inllen1) {
>>    172                  CERROR ("LIBCFS ioctl: inlbuf1 pointer but 0 length\n");
>>    173                  return 1;
>>    174          }
>>    175          if (data->ioc_inlbuf2 && !data->ioc_inllen2) {
>>    176                  CERROR ("LIBCFS ioctl: inlbuf2 pointer but 0 length\n");
>>    177                  return 1;
>>    178          }
>>    179          if (data->ioc_pbuf1 && !data->ioc_plen1) {
>>    180                  CERROR ("LIBCFS ioctl: pbuf1 pointer but 0 length\n");
>>    181                  return 1;
>>    182          }
>>    183          if (data->ioc_pbuf2 && !data->ioc_plen2) {
>>    184                  CERROR ("LIBCFS ioctl: pbuf2 pointer but 0 length\n");
>>    185                  return 1;
>>    186          }
>>    187          if (data->ioc_plen1 && !data->ioc_pbuf1) {
>>    188                  CERROR ("LIBCFS ioctl: plen1 nonzero but no pbuf1 pointer\n");
>>    189                  return 1;
>>    190          }
>>    191          if (data->ioc_plen2 && !data->ioc_pbuf2) {
>>    192                  CERROR ("LIBCFS ioctl: plen2 nonzero but no pbuf2 pointer\n");
>>    193                  return 1;
>>    194          }
>>    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?

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

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


>>       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. But can you please explain how you came to the value 1024?

Thanks,
Tao
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux