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
