This patch adds a flag, FAN_INFO and an extensible buffer to provide
additional information about response decisions. The buffer contains
one or more headers defining the information type and the length of the
following information. The patch defines one additional information
type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number. This will
allow for the creation of other information types in the future if other
users of the API identify different needs.
Suggested-by: Steve Grubb <[email protected]>
Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
Suggested-by: Jan Kara <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Richard Guy Briggs <[email protected]>
---
fs/notify/fanotify/fanotify.c | 5 +-
fs/notify/fanotify/fanotify.h | 4 ++
fs/notify/fanotify/fanotify_user.c | 86 ++++++++++++++++++++++--------
include/linux/fanotify.h | 5 ++
include/uapi/linux/fanotify.h | 30 ++++++++++-
5 files changed, 107 insertions(+), 23 deletions(-)
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index a2a15bc4df28..24ec1d66d5a8 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -262,7 +262,7 @@ static int fanotify_get_response(struct fsnotify_group *group,
}
/* userspace responded, convert to something usable */
- switch (event->response & ~FAN_AUDIT) {
+ switch (event->response & FANOTIFY_RESPONSE_ACCESS) {
case FAN_ALLOW:
ret = 0;
break;
@@ -563,6 +563,9 @@ static struct fanotify_event *fanotify_alloc_perm_event(const struct path *path,
pevent->fae.type = FANOTIFY_EVENT_TYPE_PATH_PERM;
pevent->response = 0;
+ pevent->hdr.type = FAN_RESPONSE_INFO_NONE;
+ pevent->hdr.pad = 0;
+ pevent->hdr.len = 0;
pevent->state = FAN_EVENT_INIT;
pevent->path = *path;
path_get(path);
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index f899d610bc08..e8a3c28c5d12 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -428,6 +428,10 @@ struct fanotify_perm_event {
u32 response; /* userspace answer to the event */
unsigned short state; /* state of the event */
int fd; /* fd we passed to userspace for this event */
+ union {
+ struct fanotify_response_info_header hdr;
+ struct fanotify_response_info_audit_rule audit_rule;
+ };
};
static inline struct fanotify_perm_event *
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index caa1211bac8c..cf3584351e00 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -283,19 +283,44 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
return client_fd;
}
+static int process_access_response_info(int fd, const char __user *info, size_t info_len,
+ struct fanotify_response_info_audit_rule *friar)
+{
+ if (fd == FAN_NOFD)
+ return -ENOENT;
+
+ if (info_len != sizeof(*friar))
+ return -EINVAL;
+
+ if (copy_from_user(friar, info, sizeof(*friar)))
+ return -EFAULT;
+
+ if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
+ return -EINVAL;
+ if (friar->hdr.pad != 0)
+ return -EINVAL;
+ if (friar->hdr.len != sizeof(*friar))
+ return -EINVAL;
+
+ return info_len;
+}
+
/*
* Finish processing of permission event by setting it to ANSWERED state and
* drop group->notification_lock.
*/
static void finish_permission_event(struct fsnotify_group *group,
- struct fanotify_perm_event *event,
- u32 response)
+ struct fanotify_perm_event *event, u32 response,
+ struct fanotify_response_info_audit_rule *friar)
__releases(&group->notification_lock)
{
bool destroy = false;
assert_spin_locked(&group->notification_lock);
- event->response = response;
+ event->response = response & ~FAN_INFO;
+ if (response & FAN_INFO)
+ memcpy(&event->audit_rule, friar, sizeof(*friar));
+
if (event->state == FAN_EVENT_CANCELED)
destroy = true;
else
@@ -306,20 +331,27 @@ static void finish_permission_event(struct fsnotify_group *group,
}
static int process_access_response(struct fsnotify_group *group,
- struct fanotify_response *response_struct)
+ struct fanotify_response *response_struct,
+ const char __user *info,
+ size_t info_len)
{
struct fanotify_perm_event *event;
int fd = response_struct->fd;
u32 response = response_struct->response;
+ int ret = info_len;
+ struct fanotify_response_info_audit_rule friar;
- pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
- fd, response);
+ pr_debug("%s: group=%p fd=%d response=%u buf=%p size=%zu\n", __func__,
+ group, fd, response, info, info_len);
/*
* 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) {
+ if (response & ~FANOTIFY_RESPONSE_VALID_MASK)
+ return -EINVAL;
+
+ switch (response & FANOTIFY_RESPONSE_ACCESS) {
case FAN_ALLOW:
case FAN_DENY:
break;
@@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
return -EINVAL;
}
- if (fd < 0)
+ if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
return -EINVAL;
- if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
+ if (response & FAN_INFO) {
+ ret = process_access_response_info(fd, info, info_len, &friar);
+ if (ret < 0)
+ return ret;
+ } else {
+ ret = 0;
+ }
+
+ if (fd < 0)
return -EINVAL;
spin_lock(&group->notification_lock);
@@ -340,9 +380,9 @@ static int process_access_response(struct fsnotify_group *group,
continue;
list_del_init(&event->fae.fse.list);
- finish_permission_event(group, event, response);
+ finish_permission_event(group, event, response, &friar);
wake_up(&group->fanotify_data.access_waitq);
- return 0;
+ return ret;
}
spin_unlock(&group->notification_lock);
@@ -804,7 +844,7 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
if (ret <= 0) {
spin_lock(&group->notification_lock);
finish_permission_event(group,
- FANOTIFY_PERM(event), FAN_DENY);
+ FANOTIFY_PERM(event), FAN_DENY, NULL);
wake_up(&group->fanotify_data.access_waitq);
} else {
spin_lock(&group->notification_lock);
@@ -827,28 +867,32 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos)
{
- struct fanotify_response response = { .fd = -1, .response = -1 };
+ struct fanotify_response response;
struct fsnotify_group *group;
int ret;
+ const char __user *info_buf = buf + sizeof(struct fanotify_response);
+ size_t info_len;
if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
return -EINVAL;
group = file->private_data;
+ pr_debug("%s: group=%p count=%zu\n", __func__, group, count);
+
if (count < sizeof(response))
return -EINVAL;
- count = sizeof(response);
-
- pr_debug("%s: group=%p count=%zu\n", __func__, group, count);
-
- if (copy_from_user(&response, buf, count))
+ if (copy_from_user(&response, buf, sizeof(response)))
return -EFAULT;
- ret = process_access_response(group, &response);
+ info_len = count - sizeof(response);
+
+ ret = process_access_response(group, &response, info_buf, info_len);
if (ret < 0)
count = ret;
+ else
+ count = sizeof(response) + ret;
return count;
}
@@ -876,7 +920,7 @@ static int fanotify_release(struct inode *ignored, struct file *file)
event = list_first_entry(&group->fanotify_data.access_list,
struct fanotify_perm_event, fae.fse.list);
list_del_init(&event->fae.fse.list);
- finish_permission_event(group, event, FAN_ALLOW);
+ finish_permission_event(group, event, FAN_ALLOW, NULL);
spin_lock(&group->notification_lock);
}
@@ -893,7 +937,7 @@ static int fanotify_release(struct inode *ignored, struct file *file)
fsnotify_destroy_event(group, fsn_event);
} else {
finish_permission_event(group, FANOTIFY_PERM(event),
- FAN_ALLOW);
+ FAN_ALLOW, NULL);
}
spin_lock(&group->notification_lock);
}
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index 8ad743def6f3..4f1c4f603118 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -122,6 +122,11 @@
#define ALL_FANOTIFY_EVENT_BITS (FANOTIFY_OUTGOING_EVENTS | \
FANOTIFY_EVENT_FLAGS)
+/* These masks check for invalid bits in permission responses. */
+#define FANOTIFY_RESPONSE_ACCESS (FAN_ALLOW | FAN_DENY)
+#define FANOTIFY_RESPONSE_FLAGS (FAN_AUDIT | FAN_INFO)
+#define FANOTIFY_RESPONSE_VALID_MASK (FANOTIFY_RESPONSE_ACCESS | FANOTIFY_RESPONSE_FLAGS)
+
/* Do not use these old uapi constants internally */
#undef FAN_ALL_CLASS_BITS
#undef FAN_ALL_INIT_FLAGS
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index 436258214bb0..cd14c94e9a1e 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -188,15 +188,43 @@ struct fanotify_event_info_error {
__u32 error_count;
};
+/*
+ * User space may need to record additional information about its decision.
+ * The extra information type records what kind of information is included.
+ * The default is none. We also define an extra information buffer whose
+ * size is determined by the extra information type.
+ *
+ * If the information type is Audit Rule, then the information following
+ * is the rule number that triggered the user space decision that
+ * requires auditing.
+ */
+
+#define FAN_RESPONSE_INFO_NONE 0
+#define FAN_RESPONSE_INFO_AUDIT_RULE 1
+
struct fanotify_response {
__s32 fd;
__u32 response;
};
+struct fanotify_response_info_header {
+ __u8 type;
+ __u8 pad;
+ __u16 len;
+};
+
+struct fanotify_response_info_audit_rule {
+ struct fanotify_response_info_header hdr;
+ __u32 rule_number;
+ __u32 subj_trust;
+ __u32 obj_trust;
+};
+
/* Legit userspace responses to a _PERM event */
#define FAN_ALLOW 0x01
#define FAN_DENY 0x02
-#define FAN_AUDIT 0x10 /* Bit mask to create audit record for result */
+#define FAN_AUDIT 0x10 /* Bitmask to create audit record for result */
+#define FAN_INFO 0x20 /* Bitmask to indicate additional information */
/* No fd set in event */
#define FAN_NOFD -1
--
2.27.0
On Fri, Dec 16, 2022 at 11:43 AM Jan Kara <[email protected]> wrote:
>
> On Mon 12-12-22 09:06:10, Richard Guy Briggs wrote:
> > This patch adds a flag, FAN_INFO and an extensible buffer to provide
> > additional information about response decisions. The buffer contains
> > one or more headers defining the information type and the length of the
> > following information. The patch defines one additional information
> > type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number. This will
> > allow for the creation of other information types in the future if other
> > users of the API identify different needs.
> >
> > Suggested-by: Steve Grubb <[email protected]>
> > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > Suggested-by: Jan Kara <[email protected]>
> > Link: https://lore.kernel.org/r/[email protected]
> > Signed-off-by: Richard Guy Briggs <[email protected]>
>
> Thanks for the patches. They look very good to me. Just two nits below. I
> can do the small updates on commit if there would be no other changes. But
> I'd like to get some review from audit guys for patch 3/3 before I commit
> this.
It's in my review queue, but it's a bit lower in the pile as my
understanding is that the linux-next folks don't like to see new
things in the next branches until after the merge window closes.
--
paul-moore.com
On Mon 12-12-22 09:06:10, Richard Guy Briggs wrote:
> This patch adds a flag, FAN_INFO and an extensible buffer to provide
> additional information about response decisions. The buffer contains
> one or more headers defining the information type and the length of the
> following information. The patch defines one additional information
> type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number. This will
> allow for the creation of other information types in the future if other
> users of the API identify different needs.
>
> Suggested-by: Steve Grubb <[email protected]>
> Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> Suggested-by: Jan Kara <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Richard Guy Briggs <[email protected]>
Thanks for the patches. They look very good to me. Just two nits below. I
can do the small updates on commit if there would be no other changes. But
I'd like to get some review from audit guys for patch 3/3 before I commit
this.
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index caa1211bac8c..cf3584351e00 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -283,19 +283,44 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
> return client_fd;
> }
>
> +static int process_access_response_info(int fd, const char __user *info, size_t info_len,
> + struct fanotify_response_info_audit_rule *friar)
I prefer to keep lines within 80 columns, unless there is really good
reason (like with strings) to have them longer.
BTW, why do you call the info structure 'friar'? I feel some language twist
escapes me ;)
> +{
> + if (fd == FAN_NOFD)
> + return -ENOENT;
I would not test 'fd' in this function at all. After all it is not part of
the response info structure and you do check it in
process_access_response() anyway.
> +
> + if (info_len != sizeof(*friar))
> + return -EINVAL;
> +
> + if (copy_from_user(friar, info, sizeof(*friar)))
> + return -EFAULT;
> +
> + if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
> + return -EINVAL;
> + if (friar->hdr.pad != 0)
> + return -EINVAL;
> + if (friar->hdr.len != sizeof(*friar))
> + return -EINVAL;
> +
> + return info_len;
> +}
> +
...
> @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
> return -EINVAL;
> }
>
> - if (fd < 0)
> + if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> return -EINVAL;
>
> - if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> + if (response & FAN_INFO) {
> + ret = process_access_response_info(fd, info, info_len, &friar);
> + if (ret < 0)
> + return ret;
> + } else {
> + ret = 0;
> + }
> +
> + if (fd < 0)
> return -EINVAL;
And here I'd do:
if (fd == FAN_NOFD)
return 0;
if (fd < 0)
return -EINVAL;
As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
extra info is understood by the kernel so that application writing fanotify
responses has a way to check which information it can provide to the
kernel.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Fri 16-12-22 12:05:14, Paul Moore wrote:
> On Fri, Dec 16, 2022 at 11:43 AM Jan Kara <[email protected]> wrote:
> >
> > On Mon 12-12-22 09:06:10, Richard Guy Briggs wrote:
> > > This patch adds a flag, FAN_INFO and an extensible buffer to provide
> > > additional information about response decisions. The buffer contains
> > > one or more headers defining the information type and the length of the
> > > following information. The patch defines one additional information
> > > type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number. This will
> > > allow for the creation of other information types in the future if other
> > > users of the API identify different needs.
> > >
> > > Suggested-by: Steve Grubb <[email protected]>
> > > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > > Suggested-by: Jan Kara <[email protected]>
> > > Link: https://lore.kernel.org/r/[email protected]
> > > Signed-off-by: Richard Guy Briggs <[email protected]>
> >
> > Thanks for the patches. They look very good to me. Just two nits below. I
> > can do the small updates on commit if there would be no other changes. But
> > I'd like to get some review from audit guys for patch 3/3 before I commit
> > this.
>
> It's in my review queue, but it's a bit lower in the pile as my
> understanding is that the linux-next folks don't like to see new
> things in the next branches until after the merge window closes.
Sure, there's no hurry :). I just wanted to make it clear where the things
stand.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 2022-12-16 17:43, Jan Kara wrote:
> On Mon 12-12-22 09:06:10, Richard Guy Briggs wrote:
> > This patch adds a flag, FAN_INFO and an extensible buffer to provide
> > additional information about response decisions. The buffer contains
> > one or more headers defining the information type and the length of the
> > following information. The patch defines one additional information
> > type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number. This will
> > allow for the creation of other information types in the future if other
> > users of the API identify different needs.
> >
> > Suggested-by: Steve Grubb <[email protected]>
> > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > Suggested-by: Jan Kara <[email protected]>
> > Link: https://lore.kernel.org/r/[email protected]
> > Signed-off-by: Richard Guy Briggs <[email protected]>
>
> Thanks for the patches. They look very good to me. Just two nits below. I
> can do the small updates on commit if there would be no other changes. But
> I'd like to get some review from audit guys for patch 3/3 before I commit
> this.
I'd prefer to send a followup patch based on your recommendations rather
than have you modify it. It does save some back and forth though...
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index caa1211bac8c..cf3584351e00 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -283,19 +283,44 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
> > return client_fd;
> > }
> >
> > +static int process_access_response_info(int fd, const char __user *info, size_t info_len,
> > + struct fanotify_response_info_audit_rule *friar)
>
> I prefer to keep lines within 80 columns, unless there is really good
> reason (like with strings) to have them longer.
Sure. In this case, it buys us little since the last line is lined up
with the arguments openning bracket and it one long struct name unless I
unalign that argument and back up the indent by one.
> BTW, why do you call the info structure 'friar'? I feel some language twist
> escapes me ;)
Fanotify_Response_Info_Audit_Rule, it is a pronounceable word, and
besides they have a long reputation for making good beer. :-D
> > +{
> > + if (fd == FAN_NOFD)
> > + return -ENOENT;
>
> I would not test 'fd' in this function at all. After all it is not part of
> the response info structure and you do check it in
> process_access_response() anyway.
I wrestled with that. I was even tempted to swallow the following fd
check too, but the flow would not have made as much sense for the
non-INFO case.
My understanding from Amir was that FAN_NOFD was only to be sent in in
conjuction with FAN_INFO to test if a newer kernel was present.
I presumed that if FAN_NOFD was present without FAN_INFO that was an
invalid input to an old kernel.
> > +
> > + if (info_len != sizeof(*friar))
> > + return -EINVAL;
> > +
> > + if (copy_from_user(friar, info, sizeof(*friar)))
> > + return -EFAULT;
> > +
> > + if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
> > + return -EINVAL;
> > + if (friar->hdr.pad != 0)
> > + return -EINVAL;
> > + if (friar->hdr.len != sizeof(*friar))
> > + return -EINVAL;
> > +
> > + return info_len;
> > +}
> > +
>
> ...
>
> > @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
> > return -EINVAL;
> > }
> >
> > - if (fd < 0)
> > + if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > return -EINVAL;
> >
> > - if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > + if (response & FAN_INFO) {
> > + ret = process_access_response_info(fd, info, info_len, &friar);
> > + if (ret < 0)
> > + return ret;
> > + } else {
> > + ret = 0;
> > + }
> > +
> > + if (fd < 0)
> > return -EINVAL;
>
> And here I'd do:
>
> if (fd == FAN_NOFD)
> return 0;
> if (fd < 0)
> return -EINVAL;
>
> As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
> extra info is understood by the kernel so that application writing fanotify
> responses has a way to check which information it can provide to the
> kernel.
The reason for including it in process_access_response_info() is to make
sure that it is included in the FAN_INFO case to detect this extension.
If it were included here
> Honza
>
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
- RGB
--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
On Thu 22-12-22 15:47:21, Richard Guy Briggs wrote:
> On 2022-12-16 17:43, Jan Kara wrote:
> > On Mon 12-12-22 09:06:10, Richard Guy Briggs wrote:
> > > This patch adds a flag, FAN_INFO and an extensible buffer to provide
> > > additional information about response decisions. The buffer contains
> > > one or more headers defining the information type and the length of the
> > > following information. The patch defines one additional information
> > > type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number. This will
> > > allow for the creation of other information types in the future if other
> > > users of the API identify different needs.
> > >
> > > Suggested-by: Steve Grubb <[email protected]>
> > > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > > Suggested-by: Jan Kara <[email protected]>
> > > Link: https://lore.kernel.org/r/[email protected]
> > > Signed-off-by: Richard Guy Briggs <[email protected]>
> >
> > Thanks for the patches. They look very good to me. Just two nits below. I
> > can do the small updates on commit if there would be no other changes. But
> > I'd like to get some review from audit guys for patch 3/3 before I commit
> > this.
>
> I'd prefer to send a followup patch based on your recommendations rather
> than have you modify it. It does save some back and forth though...
OK, since there are updates to patch 3 as well, I agree this is a better
way forward.
> > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > > index caa1211bac8c..cf3584351e00 100644
> > > --- a/fs/notify/fanotify/fanotify_user.c
> > > +++ b/fs/notify/fanotify/fanotify_user.c
> > > @@ -283,19 +283,44 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
> > > return client_fd;
> > > }
> > >
> > > +static int process_access_response_info(int fd, const char __user *info, size_t info_len,
> > > + struct fanotify_response_info_audit_rule *friar)
> >
> > I prefer to keep lines within 80 columns, unless there is really good
> > reason (like with strings) to have them longer.
>
> Sure. In this case, it buys us little since the last line is lined up
> with the arguments openning bracket and it one long struct name unless I
> unalign that argument and back up the indent by one.
Yeah, that's what I'd generally do.
> > BTW, why do you call the info structure 'friar'? I feel some language twist
> > escapes me ;)
>
> Fanotify_Response_Info_Audit_Rule, it is a pronounceable word, and
> besides they have a long reputation for making good beer. :-D
Aha, ok :) Thanks for explanation.
> > > +{
> > > + if (fd == FAN_NOFD)
> > > + return -ENOENT;
> >
> > I would not test 'fd' in this function at all. After all it is not part of
> > the response info structure and you do check it in
> > process_access_response() anyway.
>
> I wrestled with that. I was even tempted to swallow the following fd
> check too, but the flow would not have made as much sense for the
> non-INFO case.
>
> My understanding from Amir was that FAN_NOFD was only to be sent in in
> conjuction with FAN_INFO to test if a newer kernel was present.
Yes, that is correct. But we not only want to check that FAN_INFO flag is
understood (as you do in your patch) but also whether a particular response
type is understood (which you don't verify for FAN_NOFD). Currently, there
is only one response type (FAN_RESPONSE_INFO_AUDIT_RULE) but if there are
more in the future we need old kernels to refuse new response types even
for FAN_NOFD case.
> I presumed that if FAN_NOFD was present without FAN_INFO that was an
> invalid input to an old kernel.
Yes, that is correct and I agree the conditions I've suggested below are
wrong in that regard and need a bit of tweaking. Thanks for catching it.
> > > +
> > > + if (info_len != sizeof(*friar))
> > > + return -EINVAL;
> > > +
> > > + if (copy_from_user(friar, info, sizeof(*friar)))
> > > + return -EFAULT;
> > > +
> > > + if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
> > > + return -EINVAL;
> > > + if (friar->hdr.pad != 0)
> > > + return -EINVAL;
> > > + if (friar->hdr.len != sizeof(*friar))
> > > + return -EINVAL;
> > > +
> > > + return info_len;
> > > +}
> > > +
> >
> > ...
> >
> > > @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
> > > return -EINVAL;
> > > }
> > >
> > > - if (fd < 0)
> > > + if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > return -EINVAL;
> > >
> > > - if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > + if (response & FAN_INFO) {
> > > + ret = process_access_response_info(fd, info, info_len, &friar);
> > > + if (ret < 0)
> > > + return ret;
> > > + } else {
> > > + ret = 0;
> > > + }
> > > +
> > > + if (fd < 0)
> > > return -EINVAL;
> >
> > And here I'd do:
> >
> > if (fd == FAN_NOFD)
> > return 0;
> > if (fd < 0)
> > return -EINVAL;
> >
> > As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
> > extra info is understood by the kernel so that application writing fanotify
> > responses has a way to check which information it can provide to the
> > kernel.
>
> The reason for including it in process_access_response_info() is to make
> sure that it is included in the FAN_INFO case to detect this extension.
> If it were included here
I see what you're getting at now. So the condition
if (fd == FAN_NOFD)
return 0;
needs to be moved into
if (response & FAN_INFO)
branch after process_access_response_info(). I still prefer to keep it
outside of the process_access_response_info() function itself as it looks
more logical to me. Does it address your concerns?
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 2023-01-03 13:42, Jan Kara wrote:
> On Thu 22-12-22 15:47:21, Richard Guy Briggs wrote:
> > On 2022-12-16 17:43, Jan Kara wrote:
> > > On Mon 12-12-22 09:06:10, Richard Guy Briggs wrote:
> > > > This patch adds a flag, FAN_INFO and an extensible buffer to provide
> > > > additional information about response decisions. The buffer contains
> > > > one or more headers defining the information type and the length of the
> > > > following information. The patch defines one additional information
> > > > type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number. This will
> > > > allow for the creation of other information types in the future if other
> > > > users of the API identify different needs.
> > > >
> > > > Suggested-by: Steve Grubb <[email protected]>
> > > > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > > > Suggested-by: Jan Kara <[email protected]>
> > > > Link: https://lore.kernel.org/r/[email protected]
> > > > Signed-off-by: Richard Guy Briggs <[email protected]>
> > > > +{
> > > > + if (fd == FAN_NOFD)
> > > > + return -ENOENT;
> > >
> > > I would not test 'fd' in this function at all. After all it is not part of
> > > the response info structure and you do check it in
> > > process_access_response() anyway.
> >
> > I wrestled with that. I was even tempted to swallow the following fd
> > check too, but the flow would not have made as much sense for the
> > non-INFO case.
> >
> > My understanding from Amir was that FAN_NOFD was only to be sent in in
> > conjuction with FAN_INFO to test if a newer kernel was present.
>
> Yes, that is correct. But we not only want to check that FAN_INFO flag is
> understood (as you do in your patch) but also whether a particular response
> type is understood (which you don't verify for FAN_NOFD). Currently, there
> is only one response type (FAN_RESPONSE_INFO_AUDIT_RULE) but if there are
> more in the future we need old kernels to refuse new response types even
> for FAN_NOFD case.
Ok, I agree the NOFD check should be after.
> > I presumed that if FAN_NOFD was present without FAN_INFO that was an
> > invalid input to an old kernel.
>
> Yes, that is correct and I agree the conditions I've suggested below are
> wrong in that regard and need a bit of tweaking. Thanks for catching it.
>
> > > > +
> > > > + if (info_len != sizeof(*friar))
> > > > + return -EINVAL;
> > > > +
> > > > + if (copy_from_user(friar, info, sizeof(*friar)))
> > > > + return -EFAULT;
> > > > +
> > > > + if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
> > > > + return -EINVAL;
> > > > + if (friar->hdr.pad != 0)
> > > > + return -EINVAL;
> > > > + if (friar->hdr.len != sizeof(*friar))
> > > > + return -EINVAL;
> > > > +
> > > > + return info_len;
> > > > +}
> > > > +
> > >
> > > ...
> > >
> > > > @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
> > > > return -EINVAL;
> > > > }
> > > >
> > > > - if (fd < 0)
> > > > + if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > > return -EINVAL;
> > > >
> > > > - if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > > + if (response & FAN_INFO) {
> > > > + ret = process_access_response_info(fd, info, info_len, &friar);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > + } else {
> > > > + ret = 0;
> > > > + }
> > > > +
> > > > + if (fd < 0)
> > > > return -EINVAL;
> > >
> > > And here I'd do:
> > >
> > > if (fd == FAN_NOFD)
> > > return 0;
> > > if (fd < 0)
> > > return -EINVAL;
> > >
> > > As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
> > > extra info is understood by the kernel so that application writing fanotify
> > > responses has a way to check which information it can provide to the
> > > kernel.
> >
> > The reason for including it in process_access_response_info() is to make
> > sure that it is included in the FAN_INFO case to detect this extension.
> > If it were included here
>
> I see what you're getting at now. So the condition
>
> if (fd == FAN_NOFD)
> return 0;
>
> needs to be moved into
>
> if (response & FAN_INFO)
>
> branch after process_access_response_info(). I still prefer to keep it
> outside of the process_access_response_info() function itself as it looks
> more logical to me. Does it address your concerns?
Ok. Note that this does not return zero to userspace, since this
function's return value is added to the size of the struct
fanotify_response when there is no error.
For that reason, I think it makes more sense to return -ENOENT, or some
other unused error code that fits, unless you think it is acceptable to
return sizeof(struct fanotify_response) when FAN_INFO is set to indicate
this.
> Jan Kara <[email protected]>
- RGB
--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
On Mon 16-01-23 15:42:29, Richard Guy Briggs wrote:
> On 2023-01-03 13:42, Jan Kara wrote:
> > On Thu 22-12-22 15:47:21, Richard Guy Briggs wrote:
> > > > > +
> > > > > + if (info_len != sizeof(*friar))
> > > > > + return -EINVAL;
> > > > > +
> > > > > + if (copy_from_user(friar, info, sizeof(*friar)))
> > > > > + return -EFAULT;
> > > > > +
> > > > > + if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
> > > > > + return -EINVAL;
> > > > > + if (friar->hdr.pad != 0)
> > > > > + return -EINVAL;
> > > > > + if (friar->hdr.len != sizeof(*friar))
> > > > > + return -EINVAL;
> > > > > +
> > > > > + return info_len;
> > > > > +}
> > > > > +
> > > >
> > > > ...
> > > >
> > > > > @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
> > > > > return -EINVAL;
> > > > > }
> > > > >
> > > > > - if (fd < 0)
> > > > > + if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > > > return -EINVAL;
> > > > >
> > > > > - if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > > > + if (response & FAN_INFO) {
> > > > > + ret = process_access_response_info(fd, info, info_len, &friar);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > + } else {
> > > > > + ret = 0;
> > > > > + }
> > > > > +
> > > > > + if (fd < 0)
> > > > > return -EINVAL;
> > > >
> > > > And here I'd do:
> > > >
> > > > if (fd == FAN_NOFD)
> > > > return 0;
> > > > if (fd < 0)
> > > > return -EINVAL;
> > > >
> > > > As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
> > > > extra info is understood by the kernel so that application writing fanotify
> > > > responses has a way to check which information it can provide to the
> > > > kernel.
> > >
> > > The reason for including it in process_access_response_info() is to make
> > > sure that it is included in the FAN_INFO case to detect this extension.
> > > If it were included here
> >
> > I see what you're getting at now. So the condition
> >
> > if (fd == FAN_NOFD)
> > return 0;
> >
> > needs to be moved into
> >
> > if (response & FAN_INFO)
> >
> > branch after process_access_response_info(). I still prefer to keep it
> > outside of the process_access_response_info() function itself as it looks
> > more logical to me. Does it address your concerns?
>
> Ok. Note that this does not return zero to userspace, since this
> function's return value is added to the size of the struct
> fanotify_response when there is no error.
Right, good point. 0 is not a good return value in this case.
> For that reason, I think it makes more sense to return -ENOENT, or some
> other unused error code that fits, unless you think it is acceptable to
> return sizeof(struct fanotify_response) when FAN_INFO is set to indicate
> this.
Yeah, my intention was to indicate "success" to userspace so I'd like to
return whatever we return for the case when struct fanotify_response is
accepted for a normal file descriptor - looks like info_len is the right
value. Thanks!
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 2023-01-17 09:27, Jan Kara wrote:
> On Mon 16-01-23 15:42:29, Richard Guy Briggs wrote:
> > On 2023-01-03 13:42, Jan Kara wrote:
> > > On Thu 22-12-22 15:47:21, Richard Guy Briggs wrote:
> > > > > > +
> > > > > > + if (info_len != sizeof(*friar))
> > > > > > + return -EINVAL;
> > > > > > +
> > > > > > + if (copy_from_user(friar, info, sizeof(*friar)))
> > > > > > + return -EFAULT;
> > > > > > +
> > > > > > + if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
> > > > > > + return -EINVAL;
> > > > > > + if (friar->hdr.pad != 0)
> > > > > > + return -EINVAL;
> > > > > > + if (friar->hdr.len != sizeof(*friar))
> > > > > > + return -EINVAL;
> > > > > > +
> > > > > > + return info_len;
> > > > > > +}
> > > > > > +
> > > > >
> > > > > ...
> > > > >
> > > > > > @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
> > > > > > return -EINVAL;
> > > > > > }
> > > > > >
> > > > > > - if (fd < 0)
> > > > > > + if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > > > > return -EINVAL;
> > > > > >
> > > > > > - if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > > > > + if (response & FAN_INFO) {
> > > > > > + ret = process_access_response_info(fd, info, info_len, &friar);
> > > > > > + if (ret < 0)
> > > > > > + return ret;
> > > > > > + } else {
> > > > > > + ret = 0;
> > > > > > + }
> > > > > > +
> > > > > > + if (fd < 0)
> > > > > > return -EINVAL;
> > > > >
> > > > > And here I'd do:
> > > > >
> > > > > if (fd == FAN_NOFD)
> > > > > return 0;
> > > > > if (fd < 0)
> > > > > return -EINVAL;
> > > > >
> > > > > As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
> > > > > extra info is understood by the kernel so that application writing fanotify
> > > > > responses has a way to check which information it can provide to the
> > > > > kernel.
> > > >
> > > > The reason for including it in process_access_response_info() is to make
> > > > sure that it is included in the FAN_INFO case to detect this extension.
> > > > If it were included here
> > >
> > > I see what you're getting at now. So the condition
> > >
> > > if (fd == FAN_NOFD)
> > > return 0;
> > >
> > > needs to be moved into
> > >
> > > if (response & FAN_INFO)
> > >
> > > branch after process_access_response_info(). I still prefer to keep it
> > > outside of the process_access_response_info() function itself as it looks
> > > more logical to me. Does it address your concerns?
> >
> > Ok. Note that this does not return zero to userspace, since this
> > function's return value is added to the size of the struct
> > fanotify_response when there is no error.
>
> Right, good point. 0 is not a good return value in this case.
>
> > For that reason, I think it makes more sense to return -ENOENT, or some
> > other unused error code that fits, unless you think it is acceptable to
> > return sizeof(struct fanotify_response) when FAN_INFO is set to indicate
> > this.
>
> Yeah, my intention was to indicate "success" to userspace so I'd like to
> return whatever we return for the case when struct fanotify_response is
> accepted for a normal file descriptor - looks like info_len is the right
> value. Thanks!
Ok, I hadn't thought of that. So, to confirm, when FAN_INFO is set, if
FAN_NOFD is also set, return info_len from process_access_response() and
then immediately return sizeof(struct fanotify_response) plus info_len
to userspace without issuing an audit record should indicate support for
FAN_INFO and the specific info type supplied.
Thanks for helping work through this.
> Honza
> --
> Jan Kara <[email protected]>
- RGB
--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635