On Thu, Jul 22, 2021 at 1:39 AM Matthew Bobrowski <repnop@xxxxxxxxxx> wrote:
>
> On Wed, Jul 21, 2021 at 05:54:07AM -0700, Dan Carpenter wrote:
> > Hello Matthew Bobrowski,
> >
> > The patch f644bc449b37: "fanotify: fix copy_event_to_user() fid error
> > clean up" from Jun 11, 2021, leads to the following static checker
> > warning:
> >
> > fs/notify/fanotify/fanotify_user.c:533 copy_event_to_user()
> > error: we previously assumed 'f' could be null (see line 462)
>
> I've made a couple comments below. What am I missing?
>
> > fs/notify/fanotify/fanotify_user.c
> > 401 static ssize_t copy_event_to_user(struct fsnotify_group *group,
> > 402 struct fanotify_event *event,
> > 403 char __user *buf, size_t count)
> > 404 {
> > 405 struct fanotify_event_metadata metadata;
> > 406 struct path *path = fanotify_event_path(event);
> > 407 struct fanotify_info *info = fanotify_event_info(event);
> > 408 unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
> > 409 struct file *f = NULL;
> > 410 int ret, fd = FAN_NOFD;
> > 411 int info_type = 0;
> > 412
> > 413 pr_debug("%s: group=%p event=%p\n", __func__, group, event);
> > 414
> > 415 metadata.event_len = FAN_EVENT_METADATA_LEN +
> > 416 fanotify_event_info_len(fid_mode, event);
> > 417 metadata.metadata_len = FAN_EVENT_METADATA_LEN;
> > 418 metadata.vers = FANOTIFY_METADATA_VERSION;
> > 419 metadata.reserved = 0;
> > 420 metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
> > 421 metadata.pid = pid_vnr(event->pid);
> > 422 /*
> > 423 * For an unprivileged listener, event->pid can be used to identify the
> > 424 * events generated by the listener process itself, without disclosing
> > 425 * the pids of other processes.
> > 426 */
> > 427 if (FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) &&
> > 428 task_tgid(current) != event->pid)
> > 429 metadata.pid = 0;
> > 430
> > 431 /*
> > 432 * For now, fid mode is required for an unprivileged listener and
> > 433 * fid mode does not report fd in events. Keep this check anyway
> > 434 * for safety in case fid mode requirement is relaxed in the future
> > 435 * to allow unprivileged listener to get events with no fd and no fid.
> > 436 */
> > 437 if (!FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) &&
> > 438 path && path->mnt && path->dentry) {
> > 439 fd = create_fd(group, path, &f);
> > 440 if (fd < 0)
> > 441 return fd;
> > 442 }
> >
> > "f" is NULL on the else path
>
> Uh ha, although "fd" on the else path also remains set to the initial value
> of FAN_NOFD, right?
>
> > 443 metadata.fd = fd;
> > 444
> > 445 ret = -EFAULT;
> > 446 /*
> > 447 * Sanity check copy size in case get_one_event() and
> > 448 * event_len sizes ever get out of sync.
> > 449 */
> > 450 if (WARN_ON_ONCE(metadata.event_len > count))
> > 451 goto out_close_fd;
> > 452
> > 453 if (copy_to_user(buf, &metadata, FAN_EVENT_METADATA_LEN))
> > 454 goto out_close_fd;
> > ^^^^^^^^^^^^^^^^^
> > This is problematic
> >
> > 455
> > 456 buf += FAN_EVENT_METADATA_LEN;
> > 457 count -= FAN_EVENT_METADATA_LEN;
> > 458
> > 459 if (fanotify_is_perm_event(event->mask))
> > 460 FANOTIFY_PERM(event)->fd = fd;
> > 461
> > 462 if (f)
> > ^^^
> >
> > 463 fd_install(fd, f);
> > 464
> > 465 /* Event info records order is: dir fid + name, child fid */
> > 466 if (fanotify_event_dir_fh_len(event)) {
> > 467 info_type = info->name_len ? FAN_EVENT_INFO_TYPE_DFID_NAME :
> > 468 FAN_EVENT_INFO_TYPE_DFID;
> > 469 ret = copy_info_to_user(fanotify_event_fsid(event),
> > 470 fanotify_info_dir_fh(info),
> > 471 info_type, fanotify_info_name(info),
> > 472 info->name_len, buf, count);
> > 473 if (ret < 0)
> > 474 goto out_close_fd;
> > 475
> > 476 buf += ret;
> > 477 count -= ret;
> > 478 }
> > 479
> > 480 if (fanotify_event_object_fh_len(event)) {
> > 481 const char *dot = NULL;
> > 482 int dot_len = 0;
> > 483
> > 484 if (fid_mode == FAN_REPORT_FID || info_type) {
> > 485 /*
> > 486 * With only group flag FAN_REPORT_FID only type FID is
> > 487 * reported. Second info record type is always FID.
> > 488 */
> > 489 info_type = FAN_EVENT_INFO_TYPE_FID;
> > 490 } else if ((fid_mode & FAN_REPORT_NAME) &&
> > 491 (event->mask & FAN_ONDIR)) {
> > 492 /*
> > 493 * With group flag FAN_REPORT_NAME, if name was not
> > 494 * recorded in an event on a directory, report the
> > 495 * name "." with info type DFID_NAME.
> > 496 */
> > 497 info_type = FAN_EVENT_INFO_TYPE_DFID_NAME;
> > 498 dot = ".";
> > 499 dot_len = 1;
> > 500 } else if ((event->mask & ALL_FSNOTIFY_DIRENT_EVENTS) ||
> > 501 (event->mask & FAN_ONDIR)) {
> > 502 /*
> > 503 * With group flag FAN_REPORT_DIR_FID, a single info
> > 504 * record has type DFID for directory entry modification
> > 505 * event and for event on a directory.
> > 506 */
> > 507 info_type = FAN_EVENT_INFO_TYPE_DFID;
> > 508 } else {
> > 509 /*
> > 510 * With group flags FAN_REPORT_DIR_FID|FAN_REPORT_FID,
> > 511 * a single info record has type FID for event on a
> > 512 * non-directory, when there is no directory to report.
> > 513 * For example, on FAN_DELETE_SELF event.
> > 514 */
> > 515 info_type = FAN_EVENT_INFO_TYPE_FID;
> > 516 }
> > 517
> > 518 ret = copy_info_to_user(fanotify_event_fsid(event),
> > 519 fanotify_event_object_fh(event),
> > 520 info_type, dot, dot_len, buf, count);
> > 521 if (ret < 0)
> > 522 goto out_close_fd;
> > ^^^^^^^^^^^^^^^^^
> >
> >
> > 523
> > 524 buf += ret;
> > 525 count -= ret;
> > 526 }
> > 527
> > 528 return metadata.event_len;
> > 529
> > 530 out_close_fd:
> > 531 if (fd != FAN_NOFD) {
> > 532 put_unused_fd(fd);
> > --> 533 fput(f);
> > ^^^^^^^
> > This leads to a NULL dereference
>
> Sure would, however if the intial else path is taken above skipping
> create_fd() then "fd" would remain set to FAN_NOFD and "f" would remain set
> to NULL, then this branch would not be taken and thus not leading to a NULL
> dereference?
>
> To make things clearer, avoid any future confusion and possibly tripping
> over such a bug, perhaps it'd be better to split up the fput(f) call into a
> separate branch outside of the current conditional, simply i.e.
>
> ...
>
> if (f)
> fput(f);
>
> ...
>
> Thoughts?
smatch (apparently) does not know about the relation that f is non NULL
if (fd == FAN_NOFD) it needs to study create_fd() for that.
I suggest to move fd_install(fd, f); right after checking of return value
from create_fd() and without the if (f) condition.
That should make it clear for human and robots reading this function
that the cleanup in out_close_fd label is correct.
Thanks,
Amir.
