[bug report] fanotify: record name info for FAN_DIR_MODIFY event

* [bug report] fanotify: record name info for FAN_DIR_MODIFY event
@ 2021-11-16 11:45 Dan Carpenter
  2021-11-16 15:21 ` Amir Goldstein
  0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2021-11-16 11:45 UTC (permalink / raw)
  To: amir73il; +Cc: linux-fsdevel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread