| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAOQ4uxj5HZva82g_ku8uexnqE65K-ThKFJqABNg-A-rc03cVfg@mail.gmail.com>
Date: Tue, 17 May 2022 14:31:02 +0300
From: Amir Goldstein <amir73il@...il.com>
To: Jan Kara <jack@...e.cz>
Cc: Richard Guy Briggs <rgb@...hat.com>,
Linux-Audit Mailing List <linux-audit@...hat.com>,
LKML <linux-kernel@...r.kernel.org>,
linux-fsdevel <linux-fsdevel@...r.kernel.org>,
Paul Moore <paul@...l-moore.com>,
Eric Paris <eparis@...isplace.org>,
Steve Grubb <sgrubb@...hat.com>
Subject: Re: [PATCH v3 2/3] fanotify: define struct members to hold response
decision context
On Tue, May 17, 2022 at 1:32 PM Jan Kara <jack@...e.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@...hat.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@...hat.com>
> > > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > > Suggested-by: Jan Kara <jack@...e.cz>
> > > Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
> > > Signed-off-by: Richard Guy Briggs <rgb@...hat.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.
Powered by blists - more mailing lists