Re: [bug report] fanotify: fix copy_event_to_user() fid error clean up

[Amir Goldstein <amir73il@gmail.com>]

On Thu, Jul 22, 2021 at 1:39 AM Matthew Bobrowski <repnop@google.com> 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.