fanotify: Allow user space to pass back additional audit info

The user space API for the response variable is __u32. This patch makes sure that the whole path through the kernel uses __u32 so that there is no sign extension or truncation of the user space response.

Suggested-by Steve Grubb <[email protected]>

Link https://lore.kernel.org/r/12617626.uLZWGnKmhe@x2 open here

Signed-off-by Richard Guy Briggs <[email protected]>

Link https://lore.kernel.org/r/aa98a3ad00666a6fc0ce411755de4a1a60f5c0cd.1651174324.git.rgb@redhat.com open here

---
 fs/notify/fanotify/fanotify.h      | 2 +-
 fs/notify/fanotify/fanotify_user.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

Quoted 5 lines · Richard Guy Briggs

The user space API for the response variable is __u32. This patch makes sure that the whole path through the kernel uses __u32 so that there is no sign extension or truncation of the user space response.

Quoted 8 lines

Suggested-by Steve Grubb <[email protected]>

Link https://lore.kernel.org/r/12617626.uLZWGnKmhe@x2 open here

Signed-off-by Richard Guy Briggs <[email protected]>

Link https://lore.kernel.org/r/aa98a3ad00666a6fc0ce411755de4a1a60f5c0cd.1651174324.git.rgb@redhat.com open here

---
 fs/notify/fanotify/fanotify.h      | 2 +-
 fs/notify/fanotify/fanotify_user.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

It seems like audit_fanotify()/__audit_fanotify() should also be changed, yes? Granted, in this case it's an unsigned int to u32 conversion so not really all that critical, but if you are going to update the fanotify code you might as well update the audit code as well for the sake of completeness.

paul-moore.com

This patch adds 2 structure members to the response returned from user space on a permission event. The first field is 16 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 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 <[email protected]>

Link https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2 open here

Suggested-by Jan Kara <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Signed-off-by Richard Guy Briggs <[email protected]>

Link https://lore.kernel.org/r/17660b3f2817e5c0a19d1e9e5d40b53ff4561845.1651174324.git.rgb@redhat.com open here

---
 fs/notify/fanotify/fanotify.c      |  1 -
 fs/notify/fanotify/fanotify.h      |  4 +-
 fs/notify/fanotify/fanotify_user.c | 59 ++++++++++++++++++++----------
 include/linux/fanotify.h           |  3 ++
 include/uapi/linux/fanotify.h      | 27 +++++++++++++-
 5 files changed, 72 insertions(+), 22 deletions(-)

fs/notify/fanotify/fanotify.c +0 -0

index 985e995d2a39..00aff6e29bf8 100644

fs/notify/fanotify/fanotify.c +0 -0

        default:
                ret = -EPERM;
        }

fs/notify/fanotify/fanotify_user.c +1 -1

index 694516470660..f1ff4cf683fb 100644
@@ -289,13 +289,19 @@ static int create_fd(struct fsnotify_group *group, struct path *path,
  */
 static void finish_permission_event(struct fsnotify_group *group,
                                    struct fanotify_perm_event *event,
-                                   __u32 response)
+                                   struct fanotify_response *response)
                                    __releases(&group->notification_lock)
 {

fs/notify/fanotify/fanotify_user.c +1 -0

+       }
        if (event->state == FAN_EVENT_CANCELED)
                destroy = true;
        else

fs/notify/fanotify/fanotify_user.c +1 -1

@@ -827,26 +845,25 @@ 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;

fs/notify/fanotify/fanotify_user.c +2 -4

-       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, size))
                return -EFAULT;
 
-       ret = process_access_response(group, &response);
+       ret = process_access_response(group, &response, count);
        if (ret < 0)
                count = ret;
 

include/uapi/linux/fanotify.h +20 -0

index e8ac38cc2fd6..efb5a3a6f814 100644
@@ -179,9 +179,34 @@ 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 informaion buffer whose
+ * size is determined by the extra information type.
+ *
+ * If the context type is Rule, then the context following is the rule number
+ * that triggered the user space decision.
+ */
+
+#define FAN_RESPONSE_INFO_AUDIT_NONE   0
+#define FAN_RESPONSE_INFO_AUDIT_RULE   1
+
+struct fanotify_response_audit_rule {
+       __u32 rule;
+};
+
+#define FANOTIFY_RESPONSE_EXTRA_LEN_MAX        \
+       (sizeof(union { \
+               struct fanotify_response_audit_rule r; \

Quoted 29 lines · Richard Guy Briggs

This patch adds 2 structure members to the response returned from user space on a permission event. The first field is 16 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 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 <[email protected]>

Link https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2 open here

Suggested-by Jan Kara <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Signed-off-by Richard Guy Briggs <[email protected]>

Link https://lore.kernel.org/r/17660b3f2817e5c0a19d1e9e5d40b53ff4561845.1651174324.git.rgb@redhat.com open here

---
 fs/notify/fanotify/fanotify.c      |  1 -
 fs/notify/fanotify/fanotify.h      |  4 +-
 fs/notify/fanotify/fanotify_user.c | 59 ++++++++++++++++++++----------
 include/linux/fanotify.h           |  3 ++
 include/uapi/linux/fanotify.h      | 27 +++++++++++++-
 5 files changed, 72 insertions(+), 22 deletions(-)

fs/notify/fanotify/fanotify.c static int fanotify_get_response(struct fsnotify_group *group, · line 269

Quoted 9 lines

fs/notify/fanotify/fanotify.c +0 -1

index 985e995d2a39..00aff6e29bf8 100644
@@ -266,7 +266,6 @@ static int fanotify_get_response(struct fsnotify_group *group,
        case FAN_ALLOW:
                ret = 0;
                break;
-       case FAN_DENY:

I personally would drop this from the patch if it was me, it doesn't change the behavior so it falls under the "noise" category, which could be a problem considering the lack of response on the original posting and this one. Small, focused patches have a better shot of review/merging.

Quoted 3 lines

fs/notify/fanotify/fanotify.c +0 -0

        default:
                ret = -EPERM;
        }

...

Quoted 12 lines

fs/notify/fanotify/fanotify_user.c +1 -1

index 694516470660..f1ff4cf683fb 100644
@@ -289,13 +289,19 @@ static int create_fd(struct fsnotify_group *group, struct path *path,
  */
 static void finish_permission_event(struct fsnotify_group *group,
                                    struct fanotify_perm_event *event,
-                                   __u32 response)
+                                   struct fanotify_response *response)
                                    __releases(&group->notification_lock)
 {

fs/notify/fanotify/fanotify_user.c static int create_fd(struct fsnotify_group *group, struct path *path, · line 303

Quoted 10 lines

fs/notify/fanotify/fanotify_user.c +6 -1

        bool destroy = false;
 
        assert_spin_locked(&group->notification_lock);
-       event->response = response;
+       event->response = response->response;
+       event->extra_info_type = response->extra_info_type;
+       switch (event->extra_info_type) {
+       case FAN_RESPONSE_INFO_AUDIT_RULE:
+               memcpy(event->extra_info_buf, response->extra_info_buf,
+                      sizeof(struct fanotify_response_audit_rule));

Since the fanotify_perm_event:extra_info_buf and fanotify_response:extra_info_buf are the same type/length, and they will be the same regardless of the extra_info_type field, why not simply get rid of the above switch statement and do something like this:

  memcpy(event->extra_info_buf, response->extra_info_buf,
         sizeof(response->extra_info_buf));

Quoted 4 lines

fs/notify/fanotify/fanotify_user.c +1 -0

+       }
        if (event->state == FAN_EVENT_CANCELED)
                destroy = true;
        else

...

Quoted 15 lines

fs/notify/fanotify/fanotify_user.c +2 -1

@@ -827,26 +845,25 @@ 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;
+       size_t size = min(count, sizeof(struct fanotify_response));
 
        if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
                return -EINVAL;
 
        group = file->private_data;
 

fs/notify/fanotify/fanotify_user.c static ssize_t fanotify_read(struct file *file, char __user *buf, · line 859

Quoted 3 lines

fs/notify/fanotify/fanotify_user.c +1 -1

-       if (count < sizeof(response))
+       if (count < offsetof(struct fanotify_response, extra_info_buf))
                return -EINVAL;

Is this why you decided to shrink the fanotify_response:response field from 32-bits to 16-bits? I hope not. I would suggest both keeping the existing response field as 32-bits and explicitly checking for writes that are either the existing/compat length as well as the newer, longer length.

Quoted 13 lines

fs/notify/fanotify/fanotify_user.c +2 -4

-       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, size))
                return -EFAULT;
 
-       ret = process_access_response(group, &response);
+       ret = process_access_response(group, &response, count);
        if (ret < 0)
                count = ret;
 

...

Quoted 28 lines

include/uapi/linux/fanotify.h +20 -0

index e8ac38cc2fd6..efb5a3a6f814 100644
@@ -179,9 +179,34 @@ 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 informaion buffer whose
+ * size is determined by the extra information type.
+ *
+ * If the context type is Rule, then the context following is the rule number
+ * that triggered the user space decision.
+ */
+
+#define FAN_RESPONSE_INFO_AUDIT_NONE   0
+#define FAN_RESPONSE_INFO_AUDIT_RULE   1
+
+struct fanotify_response_audit_rule {
+       __u32 rule;
+};
+
+#define FANOTIFY_RESPONSE_EXTRA_LEN_MAX        \
+       (sizeof(union { \
+               struct fanotify_response_audit_rule r; \

include/uapi/linux/fanotify.h struct fanotify_event_info_error { · line 210

Quoted 10 lines

include/uapi/linux/fanotify.h +6 -1

+               /* add other extra info structures here */ \
+       }))
+
 struct fanotify_response {
        __s32 fd;
-       __u32 response;
+       __u16 response;
+       __u16 extra_info_type;
+       char extra_info_buf[FANOTIFY_RESPONSE_EXTRA_LEN_MAX];
 };

Since both the kernel and userspace are going to need to agree on the content and formatting of the fanotify_response:extra_info_buf field, why is it hidden behind a char array? You might as well get rid of that abstraction and put the union directly in the fanotify_response struct. It is possible you could also get rid of the fanotify_response_audit_rule struct this way too and just access the rule scalar directly.

paul-moore.com

Quoted 29 lines · Paul Moore

Quoted 28 lines · Richard Guy Briggs

This patch adds 2 structure members to the response returned from user space on a permission event. The first field is 16 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 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 <[email protected]>

Link https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2 open here

Suggested-by Jan Kara <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Signed-off-by Richard Guy Briggs <[email protected]>

Link https://lore.kernel.org/r/17660b3f2817e5c0a19d1e9e5d40b53ff4561845.1651174324.git.rgb@redhat.com open here

---
 fs/notify/fanotify/fanotify.c      |  1 -
 fs/notify/fanotify/fanotify.h      |  4 +-
 fs/notify/fanotify/fanotify_user.c | 59 ++++++++++++++++++++----------
 include/linux/fanotify.h           |  3 ++
 include/uapi/linux/fanotify.h      | 27 +++++++++++++-
 5 files changed, 72 insertions(+), 22 deletions(-)

Quoted 29 lines

Quoted 12 lines

fs/notify/fanotify/fanotify_user.c +1 -1

index 694516470660..f1ff4cf683fb 100644
@@ -289,13 +289,19 @@ static int create_fd(struct fsnotify_group *group, struct path *path,
  */
 static void finish_permission_event(struct fsnotify_group *group,
                                    struct fanotify_perm_event *event,
-                                   __u32 response)
+                                   struct fanotify_response *response)
                                    __releases(&group->notification_lock)
 {

fs/notify/fanotify/fanotify_user.c static int create_fd(struct fsnotify_group *group, struct path *path, · line 303

Quoted 10 lines

fs/notify/fanotify/fanotify_user.c +6 -1

        bool destroy = false;
 
        assert_spin_locked(&group->notification_lock);
-       event->response = response;
+       event->response = response->response;
+       event->extra_info_type = response->extra_info_type;
+       switch (event->extra_info_type) {
+       case FAN_RESPONSE_INFO_AUDIT_RULE:
+               memcpy(event->extra_info_buf, response->extra_info_buf,
+                      sizeof(struct fanotify_response_audit_rule));

Since the fanotify_perm_event:extra_info_buf and fanotify_response:extra_info_buf are the same type/length, and they will be the same regardless of the extra_info_type field, why not simply get rid of the above switch statement and do something like this:

Quoted 2 lines

  memcpy(event->extra_info_buf, response->extra_info_buf,
         sizeof(response->extra_info_buf));

I've been wrestling with the possibility of doing a split between what is presented to userspace and what's used in the kernel for struct fanotify_response, while attempting to future-proof it.

Quoted 15 lines

Quoted 15 lines

fs/notify/fanotify/fanotify_user.c +2 -1

@@ -827,26 +845,25 @@ 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;
+       size_t size = min(count, sizeof(struct fanotify_response));
 
        if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
                return -EINVAL;
 
        group = file->private_data;
 

Quoted 9 lines

fs/notify/fanotify/fanotify_user.c static ssize_t fanotify_read(struct file *file, char __user *buf, · line 859

Quoted 3 lines

fs/notify/fanotify/fanotify_user.c +1 -1

-       if (count < sizeof(response))
+       if (count < offsetof(struct fanotify_response, extra_info_buf))
                return -EINVAL;

Is this why you decided to shrink the fanotify_response:response field from 32-bits to 16-bits? I hope not. I would suggest both keeping the existing response field as 32-bits and explicitly checking for writes that are either the existing/compat length as well as the newer, longer length.

No. I shrank it at Jan's suggestion. I think I agree with you that the response field should be kept at u32 as it is defined in userspace and purge the doubt about what would happen with a new userspace with an old kernel.

Quoted 39 lines

Quoted 28 lines

include/uapi/linux/fanotify.h +20 -0

index e8ac38cc2fd6..efb5a3a6f814 100644
@@ -179,9 +179,34 @@ 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 informaion buffer whose
+ * size is determined by the extra information type.
+ *
+ * If the context type is Rule, then the context following is the rule number
+ * that triggered the user space decision.
+ */
+
+#define FAN_RESPONSE_INFO_AUDIT_NONE   0
+#define FAN_RESPONSE_INFO_AUDIT_RULE   1
+
+struct fanotify_response_audit_rule {
+       __u32 rule;
+};
+
+#define FANOTIFY_RESPONSE_EXTRA_LEN_MAX        \
+       (sizeof(union { \
+               struct fanotify_response_audit_rule r; \

include/uapi/linux/fanotify.h struct fanotify_event_info_error { · line 210

Quoted 10 lines

include/uapi/linux/fanotify.h +6 -1

+               /* add other extra info structures here */ \
+       }))
+
 struct fanotify_response {
        __s32 fd;
-       __u32 response;
+       __u16 response;
+       __u16 extra_info_type;
+       char extra_info_buf[FANOTIFY_RESPONSE_EXTRA_LEN_MAX];
 };

Quoted 7 lines

Since both the kernel and userspace are going to need to agree on the content and formatting of the fanotify_response:extra_info_buf field, why is it hidden behind a char array? You might as well get rid of that abstraction and put the union directly in the fanotify_response struct. It is possible you could also get rid of the fanotify_response_audit_rule struct this way too and just access the rule scalar directly.

This does make sense and my only concern would be a variable-length type. There isn't any reason to hide it. If userspace chooses to use the old interface and omit the type field then it defaults to NONE.

Quoted 16 lines · Paul Moore

Quoted 15 lines · Richard Guy Briggs

This patch adds 2 structure members to the response returned from user space on a permission event. The first field is 16 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 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().

Quoted 17 lines

Quoted 6 lines

static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos) {

fs/notify/fanotify/fanotify_user.c +1 -1

-       struct fanotify_response response = { .fd = -1, .response = -1 };
+       struct fanotify_response response;
        struct fsnotify_group *group;
        int ret;

fs/notify/fanotify/fanotify_user.c static ssize_t fanotify_read(struct file *file, char __user *buf, · line 859

Quoted 10 lines

fs/notify/fanotify/fanotify_user.c +2 -1

+       size_t size = min(count, sizeof(struct fanotify_response));
 
        if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
                return -EINVAL;
 
        group = file->private_data;
 
-       if (count < sizeof(response))
+       if (count < offsetof(struct fanotify_response, extra_info_buf))
                return -EINVAL;

Quoted 5 lines

Is this why you decided to shrink the fanotify_response:response field from 32-bits to 16-bits? I hope not. I would suggest both keeping the existing response field as 32-bits and explicitly checking for writes that are either the existing/compat length as well as the newer, longer length.

No. I shrank it at Jan's suggestion. I think I agree with you that the response field should be kept at u32 as it is defined in userspace and purge the doubt about what would happen with a new userspace with an old kernel.

Quoted 15 lines

Quoted 4 lines

include/uapi/linux/fanotify.h +4 -0

+
+#define FANOTIFY_RESPONSE_EXTRA_LEN_MAX        \
+       (sizeof(union { \
+               struct fanotify_response_audit_rule r; \

include/uapi/linux/fanotify.h struct fanotify_event_info_error { · line 210

Quoted 10 lines

include/uapi/linux/fanotify.h +6 -1

+               /* add other extra info structures here */ \
+       }))
+
 struct fanotify_response {
        __s32 fd;
-       __u32 response;
+       __u16 response;
+       __u16 extra_info_type;
+       char extra_info_buf[FANOTIFY_RESPONSE_EXTRA_LEN_MAX];
 };

Quoted 7 lines

Since both the kernel and userspace are going to need to agree on the content and formatting of the fanotify_response:extra_info_buf field, why is it hidden behind a char array? You might as well get rid of that abstraction and put the union directly in the fanotify_response struct. It is possible you could also get rid of the fanotify_response_audit_rule struct this way too and just access the rule scalar directly.

This does make sense and my only concern would be a variable-length type. There isn't any reason to hide it. If userspace chooses to use the old interface and omit the type field then it defaults to NONE.

If future types with variable data are defined, the first field could be a u32 that unions with the rule number that won't change the struct size.

One open question I have is what should the kernel do with 'info_type' in response it does not understand (in the future when there are possibly more different info types). It could just skip it because this should be just additional info for introspection (the only mandatory part is in fanotify_response, however it could surprise userspace that passed info is just getting ignored. To solve this we would have to somewhere report supported info types (maybe in fanotify fdinfo in proc). I guess we'll cross that bridge when we get to it.

Amir, what do you think?

Quoted 9 lines

One open question I have is what should the kernel do with 'info_type' in response it does not understand (in the future when there are possibly more different info types). It could just skip it because this should be just additional info for introspection (the only mandatory part is in fanotify_response, however it could surprise userspace that passed info is just getting ignored. To solve this we would have to somewhere report supported info types (maybe in fanotify fdinfo in proc). I guess we'll cross that bridge when we get to it.

Quoted 1 line

Amir, what do you think?

Regardless if and how we provide a way to enumerate supported info types, I would prefer to reject (EINVAL) unknown info types.

We can provide a command FAN_RESPONSE_TEST to write a test response with FAN_NOFD and some extra info so the program can test if certain info types are supported.

Quoted 2 lines · Richard Guy Briggs

Quoted 1 line · Paul Moore

On Thu, Apr 28, 2022 at 8:45 PM Richard Guy Briggs <[email protected]>

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

This patch adds 2 structure members to the response returned from user
space on a permission event. The first field is 16 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 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().

...

Quoted 30 lines

Quoted 29 lines

Quoted 22 lines

static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos) {

fs/notify/fanotify/fanotify_user.c +3 -3

-       struct fanotify_response response = { .fd = -1, .response =
-1 };
+       struct fanotify_response response;
 
        struct fsnotify_group *group;
        int ret;
 
+       size_t size = min(count, sizeof(struct fanotify_response));
 
        if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
 
                return -EINVAL;
 
        group = file->private_data;
 
-       if (count < sizeof(response))
+       if (count < offsetof(struct fanotify_response,

extra_info_buf))

Quoted 1 line

                return -EINVAL;

Is this why you decided to shrink the fanotify_response:response field from 32-bits to 16-bits? I hope not. I would suggest both keeping the existing response field as 32-bits and explicitly checking for writes that are either the existing/compat length as well as the newer, longer length.

Quoted 4 lines

No. I shrank it at Jan's suggestion. I think I agree with you that the response field should be kept at u32 as it is defined in userspace and purge the doubt about what would happen with a new userspace with an old kernel.

Hum, for the life of me I cannot find my response you mention here. Can you send a link so that I can refresh my memory? It has been a long time...

Quoted 27 lines

Quoted 17 lines

Quoted 17 lines

include/uapi/linux/fanotify.h +10 -1

+
+#define FANOTIFY_RESPONSE_EXTRA_LEN_MAX        \
+       (sizeof(union { \
+               struct fanotify_response_audit_rule r; \
+               /* add other extra info structures here */ \
+       }))
+
 
 struct fanotify_response {
 
        __s32 fd;
 
-       __u32 response;
+       __u16 response;
+       __u16 extra_info_type;
+       char extra_info_buf[FANOTIFY_RESPONSE_EXTRA_LEN_MAX];

Quoted 9 lines

Quoted 1 line

};

Since both the kernel and userspace are going to need to agree on the content and formatting of the fanotify_response:extra_info_buf field, why is it hidden behind a char array? You might as well get rid of that abstraction and put the union directly in the fanotify_response struct. It is possible you could also get rid of the fanotify_response_audit_rule struct this way too and just access the rule scalar directly.

Quoted 7 lines

This does make sense and my only concern would be a variable-length type. There isn't any reason to hide it. If userspace chooses to use the old interface and omit the type field then it defaults to NONE.

If future types with variable data are defined, the first field could be a u32 that unions with the rule number that won't change the struct size.

Struct fanotify_response size must not change, it is part of the kernel ABI. In particular your above change would break userspace code that is currently working just fine (e.g. allocating 8 bytes and expecting struct fanotify_response fits there, or just writing sizeof(struct fanotify_response) as a response while initializing only first 8 bytes). How I'd suggest doing it now (and I'd like to refresh my memory from my past emails you mention because in the past I might have thought something else ;)) is that you add another flag to 'response' field similar to FAN_AUDIT - like FAN_EXTRA_INFO. If that is present, it means extra info is to be expected after struct fanotify_response. The extra info would always start with a header like:

struct fanotify_response_info_header {
        __u8 info_type;
        __u8 pad;
        __u16 len;		/* This is including the header itself */
}

And after such header there would be the 'blob' of data 'len - header size' long. We use this same scheme when passing fanotify events to userspace and it has proven to be lightweight and extensible. It covers the situation when in the future audit would decide it wants other data (just change data type), it would also cover the situation when some other subsystem wants its information passed as well - there can be more structures like this attached at the end, we can process the response up to the length of the write.

Now these are just possible future extensions making sure we can extend the ABI without too much pain. In the current implementation I'd just return EINVAL whenever more than FANOTIFY_RESPONSE_MAX_LEN (16 bytes) is written and do very strict checks on what gets passed in. It is also trivially backwards compatible (old userspace on new kernel works just fine).

If you want to achieve compatibility of running new userspace on old kernel (I guess that's desirable), we have group flags for that - like we introduced FAN_ENABLE_AUDIT to allow for FAN_AUDIT flag in response we now need to add a flag like FAN_EXTENDED_PERMISSION_INFO telling the kernel it should expect an allow more info returning for permission events. At the same time this is the way for userspace to be able to tell whether the kernel supports this. I know this sounds tedious but that's the cost of extending the ABI in the compatible way. We've made various API mistakes in the past having to add weird workarounds to fanotify and we don't want to repeat those mistakes :).

One open question I have is what should the kernel do with 'info_type' in response it does not understand (in the future when there are possibly more different info types). It could just skip it because this should be just additional info for introspection (the only mandatory part is in fanotify_response, however it could surprise userspace that passed info is just getting ignored. To solve this we would have to somewhere report supported info types (maybe in fanotify fdinfo in proc). I guess we'll cross that bridge when we get to it.

Amir, what do you think?

Honza

Quoted 17 lines · Richard Guy Briggs

Quoted 16 lines · Paul Moore

Quoted 15 lines · Richard Guy Briggs

This patch adds 2 structure members to the response returned from user space on a permission event. The first field is 16 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 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().

...

Quoted 23 lines

Quoted 22 lines

Quoted 6 lines

static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos) {

fs/notify/fanotify/fanotify_user.c +1 -1

-       struct fanotify_response response = { .fd = -1, .response = -1 };
+       struct fanotify_response response;
        struct fsnotify_group *group;
        int ret;

fs/notify/fanotify/fanotify_user.c static ssize_t fanotify_read(struct file *file, char __user *buf, · line 859

Quoted 10 lines

fs/notify/fanotify/fanotify_user.c +2 -1

+       size_t size = min(count, sizeof(struct fanotify_response));
 
        if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
                return -EINVAL;
 
        group = file->private_data;
 
-       if (count < sizeof(response))
+       if (count < offsetof(struct fanotify_response, extra_info_buf))
                return -EINVAL;

Is this why you decided to shrink the fanotify_response:response field from 32-bits to 16-bits? I hope not. I would suggest both keeping the existing response field as 32-bits and explicitly checking for writes that are either the existing/compat length as well as the newer, longer length.

Quoted 4 lines

No. I shrank it at Jan's suggestion. I think I agree with you that the response field should be kept at u32 as it is defined in userspace and purge the doubt about what would happen with a new userspace with an old kernel.

Hum, for the life of me I cannot find my response you mention here. Can you send a link so that I can refresh my memory? It has been a long time...

Quoted 23 lines

Quoted 15 lines

Quoted 4 lines

include/uapi/linux/fanotify.h +4 -0

+
+#define FANOTIFY_RESPONSE_EXTRA_LEN_MAX        \
+       (sizeof(union { \
+               struct fanotify_response_audit_rule r; \

include/uapi/linux/fanotify.h struct fanotify_event_info_error { · line 210

Quoted 10 lines

include/uapi/linux/fanotify.h +6 -1

+               /* add other extra info structures here */ \
+       }))
+
 struct fanotify_response {
        __s32 fd;
-       __u32 response;
+       __u16 response;
+       __u16 extra_info_type;
+       char extra_info_buf[FANOTIFY_RESPONSE_EXTRA_LEN_MAX];
 };

Quoted 7 lines

Since both the kernel and userspace are going to need to agree on the content and formatting of the fanotify_response:extra_info_buf field, why is it hidden behind a char array? You might as well get rid of that abstraction and put the union directly in the fanotify_response struct. It is possible you could also get rid of the fanotify_response_audit_rule struct this way too and just access the rule scalar directly.

Quoted 7 lines

This does make sense and my only concern would be a variable-length type. There isn't any reason to hide it. If userspace chooses to use the old interface and omit the type field then it defaults to NONE.

If future types with variable data are defined, the first field could be a u32 that unions with the rule number that won't change the struct size.

Struct fanotify_response size must not change, it is part of the kernel ABI. In particular your above change would break userspace code that is currently working just fine (e.g. allocating 8 bytes and expecting struct fanotify_response fits there, or just writing sizeof(struct fanotify_response) as a response while initializing only first 8 bytes).

How I'd suggest doing it now (and I'd like to refresh my memory from my past emails you mention because in the past I might have thought something else ;)) is that you add another flag to 'response' field similar to FAN_AUDIT - like FAN_EXTRA_INFO. If that is present, it means extra info is to be expected after struct fanotify_response.

The extra info would always start with a header like:

struct fanotify_response_info_header {
        __u8 info_type;
        __u8 pad;
        __u16 len;		/* This is including the header itself */
}

And after such header there would be the 'blob' of data 'len - header size' long. We use this same scheme when passing fanotify events to userspace and it has proven to be lightweight and extensible. It covers the situation when in the future audit would decide it wants other data (just change data type), it would also cover the situation when some other subsystem wants its information passed as well - there can be more structures like this attached at the end, we can process the response up to the length of the write.

Now these are just possible future extensions making sure we can extend the ABI without too much pain. In the current implementation I'd just return EINVAL whenever more than FANOTIFY_RESPONSE_MAX_LEN (16 bytes) is written and do very strict checks on what gets passed in. It is also trivially backwards compatible (old userspace on new kernel works just fine).

If you want to achieve compatibility of running new userspace on old kernel (I guess that's desirable), we have group flags for that - like we introduced FAN_ENABLE_AUDIT to allow for FAN_AUDIT flag in response we now need to add a flag like FAN_EXTENDED_PERMISSION_INFO telling the kernel it should expect an allow more info returning for permission events. At the same time this is the way for userspace to be able to tell whether the kernel supports this. I know this sounds tedious but that's the cost of extending the ABI in the compatible way. We've made various API mistakes in the past having to add weird workarounds to fanotify and we don't want to repeat those mistakes :).

One open question I have is what should the kernel do with 'info_type' in response it does not understand (in the future when there are possibly more different info types). It could just skip it because this should be just additional info for introspection (the only mandatory part is in fanotify_response, however it could surprise userspace that passed info is just getting ignored. To solve this we would have to somewhere report supported info types (maybe in fanotify fdinfo in proc). I guess we'll cross that bridge when we get to it.

Amir, what do you think?

Jan Kara <[email protected]>

Quoted 49 lines · Jan Kara

Quoted 17 lines · Richard Guy Briggs

Quoted 16 lines · Paul Moore

Quoted 15 lines · Richard Guy Briggs

This patch adds 2 structure members to the response returned from user space on a permission event. The first field is 16 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 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().

...

Quoted 27 lines

Quoted 17 lines

Quoted 6 lines

static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos) {

fs/notify/fanotify/fanotify_user.c +1 -1

-       struct fanotify_response response = { .fd = -1, .response = -1 };
+       struct fanotify_response response;
        struct fsnotify_group *group;
        int ret;

fs/notify/fanotify/fanotify_user.c static ssize_t fanotify_read(struct file *file, char __user *buf, · line 859

Quoted 10 lines

fs/notify/fanotify/fanotify_user.c +2 -1

+       size_t size = min(count, sizeof(struct fanotify_response));
 
        if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
                return -EINVAL;
 
        group = file->private_data;
 
-       if (count < sizeof(response))
+       if (count < offsetof(struct fanotify_response, extra_info_buf))
                return -EINVAL;

Quoted 5 lines

Is this why you decided to shrink the fanotify_response:response field from 32-bits to 16-bits? I hope not. I would suggest both keeping the existing response field as 32-bits and explicitly checking for writes that are either the existing/compat length as well as the newer, longer length.

No. I shrank it at Jan's suggestion. I think I agree with you that the response field should be kept at u32 as it is defined in userspace and purge the doubt about what would happen with a new userspace with an old kernel.

Quoted 2 lines

Hum, for the life of me I cannot find my response you mention here. Can you send a link so that I can refresh my memory? It has been a long time...

https://listman.redhat.com/archives/linux-audit/2020-October/017066.html
https://listman.redhat.com/archives/linux-audit/2020-October/017067.html

Quoted 31 lines

Quoted 23 lines

Quoted 15 lines

Quoted 4 lines

include/uapi/linux/fanotify.h +4 -0

+
+#define FANOTIFY_RESPONSE_EXTRA_LEN_MAX        \
+       (sizeof(union { \
+               struct fanotify_response_audit_rule r; \

include/uapi/linux/fanotify.h struct fanotify_event_info_error { · line 210

Quoted 10 lines

include/uapi/linux/fanotify.h +6 -1

+               /* add other extra info structures here */ \
+       }))
+
 struct fanotify_response {
        __s32 fd;
-       __u32 response;
+       __u16 response;
+       __u16 extra_info_type;
+       char extra_info_buf[FANOTIFY_RESPONSE_EXTRA_LEN_MAX];
 };

Quoted 7 lines

Since both the kernel and userspace are going to need to agree on the content and formatting of the fanotify_response:extra_info_buf field, why is it hidden behind a char array? You might as well get rid of that abstraction and put the union directly in the fanotify_response struct. It is possible you could also get rid of the fanotify_response_audit_rule struct this way too and just access the rule scalar directly.

Quoted 7 lines

This does make sense and my only concern would be a variable-length type. There isn't any reason to hide it. If userspace chooses to use the old interface and omit the type field then it defaults to NONE.

If future types with variable data are defined, the first field could be a u32 that unions with the rule number that won't change the struct size.

Quoted 5 lines

Struct fanotify_response size must not change, it is part of the kernel ABI. In particular your above change would break userspace code that is currently working just fine (e.g. allocating 8 bytes and expecting struct fanotify_response fits there, or just writing sizeof(struct fanotify_response) as a response while initializing only first 8 bytes).

Many kernel ABIs have been expanded without breaking them.

Is it reasonable for a userspace program to use a kernel structure without also using its size for allocation and initialization?

Quoted 5 lines

How I'd suggest doing it now (and I'd like to refresh my memory from my past emails you mention because in the past I might have thought something else ;)) is that you add another flag to 'response' field similar to FAN_AUDIT - like FAN_EXTRA_INFO. If that is present, it means extra info is to be expected after struct fanotify_response.

That's an interesting possibility... I'm trying to predict if that would be a problem for an old kernel... In process_access_response() it would fallthrough to the default case of -EINVAL but do so safely.

Quoted 8 lines

The extra info would always start with a header like:

struct fanotify_response_info_header {
        __u8 info_type;
        __u8 pad;
        __u16 len;		/* This is including the header itself */
}

Quoted 8 lines

And after such header there would be the 'blob' of data 'len - header size' long. We use this same scheme when passing fanotify events to userspace and it has proven to be lightweight and extensible. It covers the situation when in the future audit would decide it wants other data (just change data type), it would also cover the situation when some other subsystem wants its information passed as well - there can be more structures like this attached at the end, we can process the response up to the length of the write.

This reminds me of the RFC2367 (PF_KEYv2, why is that still right there at the tip of my fingers?)

Quoted 5 lines

Now these are just possible future extensions making sure we can extend the ABI without too much pain. In the current implementation I'd just return EINVAL whenever more than FANOTIFY_RESPONSE_MAX_LEN (16 bytes) is written and do very strict checks on what gets passed in. It is also trivially backwards compatible (old userspace on new kernel works just fine).

Old userspace on new kernel would work fine with this idea, the v2 patch posted, or with leaving the response field of struct fanotify_response as __u32.

Quoted 11 lines

If you want to achieve compatibility of running new userspace on old kernel (I guess that's desirable), we have group flags for that - like we introduced FAN_ENABLE_AUDIT to allow for FAN_AUDIT flag in response we now need to add a flag like FAN_EXTENDED_PERMISSION_INFO telling the kernel it should expect an allow more info returning for permission events. At the same time this is the way for userspace to be able to tell whether the kernel supports this. I know this sounds tedious but that's the cost of extending the ABI in the compatible way. We've made various API mistakes in the past having to add weird workarounds to fanotify and we don't want to repeat those mistakes :).

Quoted 8 lines

One open question I have is what should the kernel do with 'info_type' in response it does not understand (in the future when there are possibly more different info types). It could just skip it because this should be just additional info for introspection (the only mandatory part is in fanotify_response, however it could surprise userspace that passed info is just getting ignored. To solve this we would have to somewhere report supported info types (maybe in fanotify fdinfo in proc). I guess we'll cross that bridge when we get to it.

Well, one possibility is to return -EINVAL to signal the kernel is out of date.

Quoted 50 lines · Richard Guy Briggs

Quoted 49 lines · Jan Kara

Quoted 17 lines · Richard Guy Briggs

Quoted 16 lines · Paul Moore

Quoted 15 lines · Richard Guy Briggs

This patch adds 2 structure members to the response returned from user space on a permission event. The first field is 16 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 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().

...

Quoted 27 lines

Quoted 17 lines

Quoted 6 lines

static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos) {

fs/notify/fanotify/fanotify_user.c +1 -1

-       struct fanotify_response response = { .fd = -1, .response = -1 };
+       struct fanotify_response response;
        struct fsnotify_group *group;
        int ret;

fs/notify/fanotify/fanotify_user.c static ssize_t fanotify_read(struct file *file, char __user *buf, · line 859

Quoted 10 lines

fs/notify/fanotify/fanotify_user.c +2 -1

+       size_t size = min(count, sizeof(struct fanotify_response));
 
        if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
                return -EINVAL;
 
        group = file->private_data;
 
-       if (count < sizeof(response))
+       if (count < offsetof(struct fanotify_response, extra_info_buf))
                return -EINVAL;

Quoted 5 lines

Is this why you decided to shrink the fanotify_response:response field from 32-bits to 16-bits? I hope not. I would suggest both keeping the existing response field as 32-bits and explicitly checking for writes that are either the existing/compat length as well as the newer, longer length.

No. I shrank it at Jan's suggestion. I think I agree with you that the response field should be kept at u32 as it is defined in userspace and purge the doubt about what would happen with a new userspace with an old kernel.

Quoted 5 lines

Quoted 2 lines

Hum, for the life of me I cannot find my response you mention here. Can you send a link so that I can refresh my memory? It has been a long time...

https://listman.redhat.com/archives/linux-audit/2020-October/017066.html
https://listman.redhat.com/archives/linux-audit/2020-October/017067.html

Thanks!

Quoted 37 lines

Quoted 36 lines

Quoted 23 lines

Quoted 15 lines

Quoted 4 lines

include/uapi/linux/fanotify.h +4 -0

+
+#define FANOTIFY_RESPONSE_EXTRA_LEN_MAX        \
+       (sizeof(union { \
+               struct fanotify_response_audit_rule r; \

include/uapi/linux/fanotify.h struct fanotify_event_info_error { · line 210

Quoted 10 lines

include/uapi/linux/fanotify.h +6 -1

+               /* add other extra info structures here */ \
+       }))
+
 struct fanotify_response {
        __s32 fd;
-       __u32 response;
+       __u16 response;
+       __u16 extra_info_type;
+       char extra_info_buf[FANOTIFY_RESPONSE_EXTRA_LEN_MAX];
 };

Quoted 7 lines

Since both the kernel and userspace are going to need to agree on the content and formatting of the fanotify_response:extra_info_buf field, why is it hidden behind a char array? You might as well get rid of that abstraction and put the union directly in the fanotify_response struct. It is possible you could also get rid of the fanotify_response_audit_rule struct this way too and just access the rule scalar directly.

Quoted 7 lines

This does make sense and my only concern would be a variable-length type. There isn't any reason to hide it. If userspace chooses to use the old interface and omit the type field then it defaults to NONE.

If future types with variable data are defined, the first field could be a u32 that unions with the rule number that won't change the struct size.

Struct fanotify_response size must not change, it is part of the kernel ABI. In particular your above change would break userspace code that is currently working just fine (e.g. allocating 8 bytes and expecting struct fanotify_response fits there, or just writing sizeof(struct fanotify_response) as a response while initializing only first 8 bytes).

Quoted 4 lines

Many kernel ABIs have been expanded without breaking them.

Is it reasonable for a userspace program to use a kernel structure without also using its size for allocation and initialization?

Well, I'm not sure whether to call it reasonable but it certainly happens and we are generally obliged to keep backwards compatibility (the golden "don't break userspace" rule).

Quoted 6 lines

Quoted 5 lines

How I'd suggest doing it now (and I'd like to refresh my memory from my past emails you mention because in the past I might have thought something else ;)) is that you add another flag to 'response' field similar to FAN_AUDIT - like FAN_EXTRA_INFO. If that is present, it means extra info is to be expected after struct fanotify_response.

Quoted 3 lines

That's an interesting possibility... I'm trying to predict if that would be a problem for an old kernel... In process_access_response() it would fallthrough to the default case of -EINVAL but do so safely.

Yes, old kernel would just refuse such response so new userspace can adapt to that.

Quoted 26 lines

Quoted 8 lines

The extra info would always start with a header like:

struct fanotify_response_info_header {
        __u8 info_type;
        __u8 pad;
        __u16 len;		/* This is including the header itself */
}

Quoted 8 lines

And after such header there would be the 'blob' of data 'len - header size' long. We use this same scheme when passing fanotify events to userspace and it has proven to be lightweight and extensible. It covers the situation when in the future audit would decide it wants other data (just change data type), it would also cover the situation when some other subsystem wants its information passed as well - there can be more structures like this attached at the end, we can process the response up to the length of the write.

This reminds me of the RFC2367 (PF_KEYv2, why is that still right there at the tip of my fingers?)

Quoted 5 lines

Now these are just possible future extensions making sure we can extend the ABI without too much pain. In the current implementation I'd just return EINVAL whenever more than FANOTIFY_RESPONSE_MAX_LEN (16 bytes) is written and do very strict checks on what gets passed in. It is also trivially backwards compatible (old userspace on new kernel works just fine).

Quoted 3 lines

Old userspace on new kernel would work fine with this idea, the v2 patch posted, or with leaving the response field of struct fanotify_response as __u32.

So I've realized the idea to reduce 'response' field to __u16 will not work for big endian architectures (that was a thinko of my old suggestion from 2020). Old userspace binaries will break with that. So we have to keep 'response' to __u32 and just add a flag like I'm suggesting now or something like that.

Quoted 20 lines

Quoted 19 lines

If you want to achieve compatibility of running new userspace on old kernel (I guess that's desirable), we have group flags for that - like we introduced FAN_ENABLE_AUDIT to allow for FAN_AUDIT flag in response we now need to add a flag like FAN_EXTENDED_PERMISSION_INFO telling the kernel it should expect an allow more info returning for permission events. At the same time this is the way for userspace to be able to tell whether the kernel supports this. I know this sounds tedious but that's the cost of extending the ABI in the compatible way. We've made various API mistakes in the past having to add weird workarounds to fanotify and we don't want to repeat those mistakes :).

One open question I have is what should the kernel do with 'info_type' in response it does not understand (in the future when there are possibly more different info types). It could just skip it because this should be just additional info for introspection (the only mandatory part is in fanotify_response, however it could surprise userspace that passed info is just getting ignored. To solve this we would have to somewhere report supported info types (maybe in fanotify fdinfo in proc). I guess we'll cross that bridge when we get to it.

Quoted 2 lines

Well, one possibility is to return -EINVAL to signal the kernel is out of date.

Yes, I think we've settled with Amir on returning -EINVAL in case unknown info is spotted.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

This patch passes the full value so that the audit function can use all of it. The audit function was updated to log the additional information in the AUDIT_FANOTIFY record. The following is an example of the new record format:

type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17

Suggested-by Steve Grubb <[email protected]>

Link https://lore.kernel.org/r/3075502.aeNJFYEL58@x2 open here

Signed-off-by Richard Guy Briggs <[email protected]>

Link https://lore.kernel.org/r/23c7f206a465d88cc646a944515fcc6a365f5eb2.1651174324.git.rgb@redhat.com open here

---
 fs/notify/fanotify/fanotify.c |  4 +++-
 include/linux/audit.h         |  8 ++++----
 kernel/auditsc.c              | 18 +++++++++++++++---
 3 files changed, 22 insertions(+), 8 deletions(-)

kernel/auditsc.c +1 -0

index ea2ee1181921..afdbc416069a 100644
@@ -64,6 +64,7 @@
 #include <uapi/linux/limits.h>
 #include <uapi/linux/netfilter/nf_tables.h>
 #include <uapi/linux/openat2.h> // struct open_how
+#include <uapi/linux/fanotify.h>
 
 #include "audit.h"
 

@@ -2893,10 +2894,21 @@ void __audit_log_kern_module(char *name)
        context->type = AUDIT_KERN_MODULE;
 }
 

The Fanotify API can be used for access control by requesting permission event notification. The user space tooling that uses it may have a complicated policy that inherently contains additional context for the decision. If this information were available in the audit trail, policy writers can close the loop on debugging policy. Also, if this additional information were available, it would enable the creation of tools that can suggest changes to the policy similar to how audit2allow can help refine labeled security.

This patch defines 2 additional fields within the response structure returned from user space on a permission event. The first field is 16 bits for the context type. The context type will describe what the meaning is of the second field. The audit system will separate the pieces and log them individually.

The audit function was updated to log the additional information in the AUDIT_FANOTIFY record. The following is an example of the new record format:

type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17

changelog:
v1:
- first version by Steve Grubb <[email protected]>

Link https://lore.kernel.org/r/2042449.irdbgypaU6@x2 open here

v2:
- enhancements suggested by Jan Kara <[email protected]>
- 1/3 change %d to %u in pr_debug
- 2/3 change response from __u32 to __u16
- mod struct fanotify_response and fanotify_perm_event add extra_info_type, extra_info_buf
- extra_info_buf size max FANOTIFY_MAX_RESPONSE_EXTRA_LEN, add struct fanotify_response_audit_rule
- extend debug statements
- remove unneeded macros
- [internal] change interface to finish_permission_event() and process_access_response()
- 3/3 update format of extra information
- [internal] change interface to audit_fanotify()
- change ctx_type= to fan_type=

Link https://lore.kernel.org/r/[email protected] open here

Richard Guy Briggs (3):
fanotify: Ensure consistent variable type for response
fanotify: define struct members to hold response decision context
fanotify: Allow audit to use the full permission event response

 fs/notify/fanotify/fanotify.c      |  5 ++-
 fs/notify/fanotify/fanotify.h      |  4 +-
 fs/notify/fanotify/fanotify_user.c | 59 ++++++++++++++++++++----------
 include/linux/audit.h              |  8 ++--
 include/linux/fanotify.h           |  3 ++
 include/uapi/linux/fanotify.h      | 27 +++++++++++++-
 kernel/auditsc.c                   | 18 +++++++--
 7 files changed, 94 insertions(+), 30 deletions(-)

Signature · 1 line

2.27.0

Quoted 19 lines · Richard Guy Briggs

The Fanotify API can be used for access control by requesting permission event notification. The user space tooling that uses it may have a complicated policy that inherently contains additional context for the decision. If this information were available in the audit trail, policy writers can close the loop on debugging policy. Also, if this additional information were available, it would enable the creation of tools that can suggest changes to the policy similar to how audit2allow can help refine labeled security.

This patch defines 2 additional fields within the response structure returned from user space on a permission event. The first field is 16 bits for the context type. The context type will describe what the meaning is of the second field. The audit system will separate the pieces and log them individually.

The audit function was updated to log the additional information in the AUDIT_FANOTIFY record. The following is an example of the new record format:

Quoted 1 line

type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17

It might have been a good idea to tag this as RFC... I have a few questions:

1. Where did "resp=" come from?

It isn't in the field dictionary. It
seems like a needless duplication of "res=". If it isn't, maybe it should have a "fan_" namespace prefix and become "fan_res="?

2. It appears I'm ok changing the "__u32 response" to "__u16" without breaking old userspace. Is this true on all arches?

3. What should be the action if response contains unknown flags or types? Is it reasonable to return -EINVAL?

4. Currently, struct fanotify_response has a fixed size, but if future types get defined that have variable buffer sizes, how would that be communicated or encoded?

Quoted 20 lines · Richard Guy Briggs

Quoted 19 lines · Richard Guy Briggs

The Fanotify API can be used for access control by requesting permission event notification. The user space tooling that uses it may have a complicated policy that inherently contains additional context for the decision. If this information were available in the audit trail, policy writers can close the loop on debugging policy. Also, if this additional information were available, it would enable the creation of tools that can suggest changes to the policy similar to how audit2allow can help refine labeled security.

This patch defines 2 additional fields within the response structure returned from user space on a permission event. The first field is 16 bits for the context type. The context type will describe what the meaning is of the second field. The audit system will separate the pieces and log them individually.

The audit function was updated to log the additional information in the AUDIT_FANOTIFY record. The following is an example of the new record format:

Quoted 6 lines

Quoted 1 line

type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17

It might have been a good idea to tag this as RFC... I have a few questions:

1. Where did "resp=" come from?

According to the git log, it came from Steve Grubb via de8cd83e91bc ("audit: Record fanotify access control decisions"). Steve should have known what he was doing with respect to field names so I'm assuming he had a reason.

Quoted 3 lines

It isn't in the field dictionary. It
seems like a needless duplication of "res=". If it isn't, maybe it should have a "fan_" namespace prefix and become "fan_res="?

Regardless of what it should have been, it is "resp" now and we can't really change it. As far as the field dictionary is concerned, while we should document these fields, it is important to note that when the dictionary conflicts with the kernel, the kernel wins by definition.

Quoted 2 lines

2. It appears I'm ok changing the "__u32 response" to "__u16" without breaking old userspace. Is this true on all arches?

I can't answer that for you, the fanotify folks will need to look at that, but you likely already know that. While I haven't gone through the entire patchset yet, if it was me I probably would have left response as a u32 and just added the extra fields; you are already increasing the size of fanotify_response so why bother with shrinking an existing field?

Quoted 2 lines

3. What should be the action if response contains unknown flags or types? Is it reasonable to return -EINVAL?

Once again, not a fanotify expert, but EINVAL is intended for invalid input so it seems like a reasonable choice.

Quoted 3 lines

4. Currently, struct fanotify_response has a fixed size, but if future types get defined that have variable buffer sizes, how would that be communicated or encoded?

If that is a concern, you should probably include a length field in the structure before the variable length field. You can't put it before fd or response, so it's really a question of before or after your new extra_info_type; I might suggest *after* extra_info_type, but that's just me.

paul-moore.com

Quoted 19 lines · Richard Guy Briggs

The Fanotify API can be used for access control by requesting permission event notification. The user space tooling that uses it may have a complicated policy that inherently contains additional context for the decision. If this information were available in the audit trail, policy writers can close the loop on debugging policy. Also, if this additional information were available, it would enable the creation of tools that can suggest changes to the policy similar to how audit2allow can help refine labeled security.

This patch defines 2 additional fields within the response structure returned from user space on a permission event. The first field is 16 bits for the context type. The context type will describe what the meaning is of the second field. The audit system will separate the pieces and log them individually.

The audit function was updated to log the additional information in the AUDIT_FANOTIFY record. The following is an example of the new record format:

Quoted 1 line

type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17

It might have been a good idea to tag this as RFC... I have a few questions:

1. Where did "resp=" come from?

It isn't in the field dictionary. It seems like a needless duplication of "res=". If it isn't, maybe it should have a "fan_" namespace prefix and become "fan_res="?

2. It appears I'm ok changing the "__u32 response" to "__u16" without breaking old userspace. Is this true on all arches?

3. What should be the action if response contains unknown flags or types? Is it reasonable to return -EINVAL?

4. Currently, struct fanotify_response has a fixed size, but if future types get defined that have variable buffer sizes, how would that be communicated or encoded?