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.
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.
>
> 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[]
> 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 */
> if (orig_len != hdr->ioc_len)
> return -EINVAL;
>
> 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.
>
> regards,
> dan carpenter
> _______________________________________________
> devel mailing list
> devel@xxxxxxxxxxxxxxxxxxxxxx
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
