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
