Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <cover.1652730821.git.rgb@redhat.com> <1520f08c023d1e919b1a2af161d5a19367b6b4bf.1652730821.git.rgb@redhat.com>
 <CAOQ4uxjV-eNxJ=O_WFTTzspCxXZqpMdh3Fe-N5aB-h1rDr_1hQ@mail.gmail.com> <20220517103236.i7gtsw7akiikqwam@quack3.lan>
In-Reply-To: <20220517103236.i7gtsw7akiikqwam@quack3.lan>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Tue, 17 May 2022 14:31:02 +0300
Message-ID: <CAOQ4uxj5HZva82g_ku8uexnqE65K-ThKFJqABNg-A-rc03cVfg@mail.gmail.com>
Subject: Re: [PATCH v3 2/3] fanotify: define struct members to hold response
 decision context
To:     Jan Kara <jack@suse.cz>
Cc:     Richard Guy Briggs <rgb@redhat.com>,
        Linux-Audit Mailing List <linux-audit@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Paul Moore <paul@paul-moore.com>,
        Eric Paris <eparis@parisplace.org>,
        Steve Grubb <sgrubb@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, May 17, 2022 at 1:32 PM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> > On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > >
> > > This patch adds 2 structure members to the response returned from user
> > > space on a permission event. The first field is 32 bits for the context
> > > type.  The context type will describe what the meaning is of the second
> > > field. The default is none. The patch defines one additional context
> > > type which means that the second field is a union containing a 32-bit
> > > rule number. This will allow for the creation of other context types in
> > > the future if other users of the API identify different needs.  The
> > > second field size is defined by the context type and can be used to pass
> > > along the data described by the context.
> > >
> > > To support this, there is a macro for user space to check that the data
> > > being sent is valid. Of course, without this check, anything that
> > > overflows the bit field will trigger an EINVAL based on the use of
> > > FAN_INVALID_RESPONSE_MASK in process_access_response().
> > >
> > > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > > Suggested-by: Jan Kara <jack@suse.cz>
> > > Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>
> ...
> > >  static int process_access_response(struct fsnotify_group *group,
> > > -                                  struct fanotify_response *response_struct)
> > > +                                  struct fanotify_response *response_struct,
> > > +                                  size_t count)
> > >  {
> > >         struct fanotify_perm_event *event;
> > >         int fd = response_struct->fd;
> > >         u32 response = response_struct->response;
> > >
> > > -       pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > > -                fd, response);
> > > +       pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > > +                group, fd, response, response_struct->extra_info_type, count);
> > > +       if (fd < 0)
> > > +               return -EINVAL;
> > >         /*
> > >          * make sure the response is valid, if invalid we do nothing and either
> > >          * userspace can send a valid response or we will clean it up after the
> > >          * timeout
> > >          */
> > > -       switch (response & ~FAN_AUDIT) {
> > > -       case FAN_ALLOW:
> > > -       case FAN_DENY:
> > > -               break;
> > > -       default:
> > > -               return -EINVAL;
> > > -       }
> > > -
> > > -       if (fd < 0)
> > > +       if (FAN_INVALID_RESPONSE_MASK(response))
> >
> > That is a logic change, because now the response value of 0 becomes valid.
> >
> > Since you did not document this change in the commit message I assume this was
> > non intentional?
> > However, this behavior change is something that I did ask for, but it should be
> > done is a separate commit:
> >
> >  /* These are NOT bitwise flags.  Both bits can be used together.  */
> > #define FAN_TEST          0x00
> > #define FAN_ALLOW       0x01
> > #define FAN_DENY        0x02
> > #define FANOTIFY_RESPONSE_ACCESS \
> >             (FAN_TEST|FAN_ALLOW | FAN_DENY)
> >
> > ...
> > int access = response & FANOTIFY_RESPONSE_ACCESS;
> >
> > 1. Do return EINVAL for access == 0
> > 2. Let all the rest of the EINVAL checks run (including extra type)
> > 3. Move if (fd < 0) to last check
> > 4. Add if (!access) return 0 before if (fd < 0)
> >
> > That will provide a mechanism for userspace to probe the
> > kernel support for extra types in general and specific types
> > that it may respond with.
>
> I have to admit I didn't quite grok your suggestion here although I
> understand (and agree with) the general direction of the proposal :). Maybe
> code would explain it better what you have in mind?
>

+/* These are NOT bitwise flags.  Both bits can be used together.  */
+#define FAN_TEST          0x00
#define FAN_ALLOW       0x01
#define FAN_DENY        0x02
#define FAN_AUDIT       0x10    /* Bit mask to create audit record for result */
+#define FANOTIFY_RESPONSE_ACCESS \
+            (FAN_TEST|FAN_ALLOW | FAN_DENY)

...

@@ -311,6 +314,7 @@ static int process_access_response(struct
fsnotify_group *group,
        struct fanotify_perm_event *event;
        int fd = response_struct->fd;
        int response = response_struct->response;
+       int access = response & FANOTIFY_RESPONSE_ACCESS;

        pr_debug("%s: group=%p fd=%d response=%d\n", __func__, group,
                 fd, response);
@@ -319,18 +323,33 @@ static int process_access_response(struct
fsnotify_group *group,
         * userspace can send a valid response or we will clean it up after the
         * timeout
         */
-       switch (response & ~FAN_AUDIT) {
+       if (!response)
+               return -EINVAL;
+
+       switch (access) {
        case FAN_ALLOW:
        case FAN_DENY:
+       case FAN_TEST:
                break;
        default:
                return -EINVAL;
        }

-       if (fd < 0)
-                return -EINVAL;
-
       if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
                return -EINVAL;

+       /*
+        * FAN_TEST|FAN_AUDIT response can be written during setup time to probe
+        * if the kernel has support for FAN_AUDIT.
+        * For FAN_TEST, fd must not be valid.
+        */
+       if (access == FAN_TEST) {
+               if (fd >= 0)
+                       return -EINVAL;
+               return 0;
+       }
+
+       if (fd < 0)
+                return -EINVAL;

Thanks,
Amir.