Hello Amir Goldstein,
The patch cacfb956d46e: "fanotify: record name info for
FAN_DIR_MODIFY event" from Mar 19, 2020, leads to the following
Smatch static checker warning:
fs/notify/fanotify/fanotify_user.c:401 copy_fid_info_to_user()
error: we previously assumed 'fh' could be null (see line 362)
fs/notify/fanotify/fanotify_user.c
354 static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
355 int info_type, const char *name,
356 size_t name_len,
357 char __user *buf, size_t count)
358 {
359 struct fanotify_event_info_fid info = { };
360 struct file_handle handle = { };
361 unsigned char bounce[FANOTIFY_INLINE_FH_LEN], *fh_buf;
362 size_t fh_len = fh ? fh->len : 0;
^^^^^^^^^^^^^
The patch adds a check for in "fh" is NULL
363 size_t info_len = fanotify_fid_info_len(fh_len, name_len);
364 size_t len = info_len;
365
366 pr_debug("%s: fh_len=%zu name_len=%zu, info_len=%zu, count=%zu\n",
367 __func__, fh_len, name_len, info_len, count);
368
369 if (WARN_ON_ONCE(len < sizeof(info) || len > count))
370 return -EFAULT;
371
372 /*
373 * Copy event info fid header followed by variable sized file handle
374 * and optionally followed by variable sized filename.
375 */
376 switch (info_type) {
377 case FAN_EVENT_INFO_TYPE_FID:
378 case FAN_EVENT_INFO_TYPE_DFID:
379 if (WARN_ON_ONCE(name_len))
380 return -EFAULT;
381 break;
382 case FAN_EVENT_INFO_TYPE_DFID_NAME:
383 if (WARN_ON_ONCE(!name || !name_len))
384 return -EFAULT;
385 break;
386 default:
387 return -EFAULT;
388 }
389
390 info.hdr.info_type = info_type;
391 info.hdr.len = len;
392 info.fsid = *fsid;
393 if (copy_to_user(buf, &info, sizeof(info)))
394 return -EFAULT;
395
396 buf += sizeof(info);
397 len -= sizeof(info);
398 if (WARN_ON_ONCE(len < sizeof(handle)))
399 return -EFAULT;
400
--> 401 handle.handle_type = fh->type;
^^^^^^^^
But this code dereferences "fh" without checking.
402 handle.handle_bytes = fh_len;
403
404 /* Mangle handle_type for bad file_handle */
405 if (!fh_len)
406 handle.handle_type = FILEID_INVALID;
407
408 if (copy_to_user(buf, &handle, sizeof(handle)))
409 return -EFAULT;
410
411 buf += sizeof(handle);
412 len -= sizeof(handle);
413 if (WARN_ON_ONCE(len < fh_len))
414 return -EFAULT;
415
416 /*
417 * For an inline fh and inline file name, copy through stack to exclude
418 * the copy from usercopy hardening protections.
419 */
420 fh_buf = fanotify_fh_buf(fh);
421 if (fh_len <= FANOTIFY_INLINE_FH_LEN) {
422 memcpy(bounce, fh_buf, fh_len);
423 fh_buf = bounce;
424 }
425 if (copy_to_user(buf, fh_buf, fh_len))
426 return -EFAULT;
427
428 buf += fh_len;
429 len -= fh_len;
430
431 if (name_len) {
432 /* Copy the filename with terminating null */
433 name_len++;
434 if (WARN_ON_ONCE(len < name_len))
435 return -EFAULT;
436
437 if (copy_to_user(buf, name, name_len))
438 return -EFAULT;
439
440 buf += name_len;
441 len -= name_len;
442 }
443
444 /* Pad with 0's */
445 WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN);
446 if (len > 0 && clear_user(buf, len))
447 return -EFAULT;
448
449 return info_len;
450 }
regards,
dan carpenter
