fanotify: Allow user space to pass back additional audit info

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, an audit 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 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]>

---

 fs/notify/fanotify/fanotify.c      |  10 ++-
 fs/notify/fanotify/fanotify.h      |   2 +
 fs/notify/fanotify/fanotify_user.c | 104 +++++++++++++++++++++++------
 include/linux/fanotify.h           |   5 ++
 include/uapi/linux/fanotify.h      |  27 +++++++-
 5 files changed, 123 insertions(+), 25 deletions(-)

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

index 4f897e109547..0f36062521f4 100644
@@ -262,13 +262,16 @@ 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) {

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

        }
 
        /* Check if the response should be audited */

@@ -560,6 +563,8 @@ static struct fanotify_event *fanotify_alloc_perm_event(const struct path *path,
 
        pevent->fae.type = FANOTIFY_EVENT_TYPE_PATH_PERM;
        pevent->response = 0;
+       pevent->info_len = 0;
+       pevent->info_buf = NULL;
        pevent->state = FAN_EVENT_INIT;
        pevent->path = *path;
        path_get(path);

@@ -996,6 +1001,7 @@ static void fanotify_free_path_event(struct fanotify_event *event)
 static void fanotify_free_perm_event(struct fanotify_event *event)
 {
        path_put(fanotify_event_path(event));
+       kfree(FANOTIFY_PERM(event)->info_buf);
        kmem_cache_free(fanotify_perm_event_cachep, FANOTIFY_PERM(event));
 }
 

fs/notify/fanotify/fanotify.h +2 -0

index abfa3712c185..14c30e173632 100644
@@ -428,6 +428,8 @@ 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 */
+       size_t info_len;
+       char *info_buf;
 };
 
 static inline struct fanotify_perm_event *

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

index ff67ca0d25cc..a4ae953f0e62 100644
@@ -289,13 +289,18 @@ 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,
+                                   size_t info_len, char *info_buf)
                                    __releases(&group->notification_lock)
 {
        bool destroy = false;
 
        assert_spin_locked(&group->notification_lock);
-       event->response = response;
+       event->response = response->response & ~FAN_INFO;
+       if (response->response & FAN_INFO) {
+               event->info_len = info_len;
+               event->info_buf = info_buf;
+       }
        if (event->state == FAN_EVENT_CANCELED)
                destroy = true;
        else

@@ -306,33 +311,71 @@ 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 *buf,
+                                  size_t count)
 {
        struct fanotify_perm_event *event;
        int fd = response_struct->fd;
        u32 response = response_struct->response;
+       struct fanotify_response_info_header info_hdr;
+       char *info_buf = NULL;
 
-       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=%lu\n", __func__,
+                group, fd, response, info_buf, count);
        /*
         * 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;
        default:

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

+                       if (copy_from_user(&info_hdr, ib, sizeof(info_hdr)))
+                               return -EFAULT;
+                       if (info_hdr.pad != 0)
+                               return -EINVAL;
+                       if (c < info_hdr.len)
+                               return -EINVAL;
+                       switch (info_hdr.type) {
+                       case FAN_RESPONSE_INFO_AUDIT_RULE:
+                               break;
+                       case FAN_RESPONSE_INFO_NONE:
+                       default:
+                               return -EINVAL;
+                       }
+                       c -= info_hdr.len;
+                       ib += info_hdr.len;
+               }
+               if (c != 0)

fs/notify/fanotify/fanotify_user.c +13 -9

+       }
        spin_lock(&group->notification_lock);
        list_for_each_entry(event, &group->fanotify_data.access_list,
                            fae.fse.list) {

@@ -340,7 +383,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() eats info_buf */
+               finish_permission_event(group, event, response_struct,
+                                       count, info_buf);
                wake_up(&group->fanotify_data.access_waitq);
                return 0;
        }

@@ -802,9 +847,14 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
                        fsnotify_destroy_event(group, &event->fse);
                } else {
                        if (ret <= 0) {
+                               struct fanotify_response response = {
+                                       .fd = FAN_NOFD,
+                                       .response = FAN_DENY };
+
                                spin_lock(&group->notification_lock);
                                finish_permission_event(group,
-                                       FANOTIFY_PERM(event), FAN_DENY);
+                                       FANOTIFY_PERM(event), &response,
+                                       0, NULL);
                                wake_up(&group->fanotify_data.access_waitq);
                        } else {
                                spin_lock(&group->notification_lock);

@@ -827,26 +877,33 @@ 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 c;
 
        if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
                return -EINVAL;
 
        group = file->private_data;
 
-       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))

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

+       } else {
+               if (c != 0)
+                       return -EINVAL;
+       }
+       ret = process_access_response(group, &response, info_buf, c);
        if (ret < 0)
                count = ret;
 

@@ -857,6 +914,9 @@ static int fanotify_release(struct inode *ignored, struct file *file)
 {
        struct fsnotify_group *group = file->private_data;
        struct fsnotify_event *fsn_event;
+       struct fanotify_response response = {
+               .fd = FAN_NOFD,
+               .response = FAN_ALLOW };
 
        /*
         * Stop new events from arriving in the notification queue. since

@@ -876,7 +936,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, &response, 0, NULL);
                spin_lock(&group->notification_lock);
        }
 

@@ -893,7 +953,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);
+                                               &response, 0, NULL);
                }
                spin_lock(&group->notification_lock);
        }

include/linux/fanotify.h +5 -0

index edc28555814c..ce9f97eb69f2 100644
@@ -114,6 +114,11 @@
 #define ALL_FANOTIFY_EVENT_BITS                (FANOTIFY_OUTGOING_EVENTS | \
                                         FANOTIFY_EVENT_FLAGS)
 
+/* This mask is to check for invalid bits of a user space permission response */
+#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

include/uapi/linux/fanotify.h +26 -2

index f1f89132d60e..4d08823a5698 100644
@@ -180,15 +180,40 @@ 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 context type is Rule, then the context following is the rule number
+ * that triggered the user space decision.
+ */
+
+#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 audit_rule;
+};
+
 /* 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

[+linux-api]

Quoted 15 lines · Richard Guy Briggs

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, an audit 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 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]>

---

Quoted 29 lines

static int process_access_response(struct fsnotify_group *group,

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

-                                  struct fanotify_response *response_struct)
+                                  struct fanotify_response *response_struct,
+                                  const char __user *buf,
+                                  size_t count)
 {
        struct fanotify_perm_event *event;
        int fd = response_struct->fd;
        u32 response = response_struct->response;
+       struct fanotify_response_info_header info_hdr;
+       char *info_buf = NULL;
 
-       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=%lu\n", __func__,
+                group, fd, response, info_buf, count);
        /*
         * 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;
        default:

fs/notify/fanotify/fanotify_user.c static void finish_permission_event(struct fsnotify_group *group, · line 343

Quoted 10 lines

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

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

Since you did not accept my suggestion of FAN_TEST [1], I am not sure why this check was moved.

However, if you move this check past FAN_INFO processing, you could change the error value to -ENOENT, same as the return value for an fd that is >= 0 but does not correspond to any pending permission event.

The idea was that userspace could write a test fanotify_response_info_audit_rule payload to fanotify fd with FAN_NOFD in the response.fd field.
On old kernel, this will return EINVAL.
On new kernel, if the fanotify_response_info_audit_rule payload passes all the validations, this will do nothing and return ENOENT.

[1] https://lore.kernel.org/linux-fsdevel/CAOQ4uxi+8HUqyGxQBNMqSong92nreOWLKdy9MCrYg8wgW9Dj4g@mail.gmail.com/

Quoted 13 lines

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

-       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 (count < sizeof(response))
+               return -EINVAL;
+       if (copy_from_user(&response, buf, sizeof(response)))
                return -EFAULT;

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

Quoted 5 lines

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

-       ret = process_access_response(group, &response);
+       c = count - sizeof(response);
+       if (response.response & FAN_INFO) {
+               if (c < sizeof(struct fanotify_response_info_header))
+                       return -EINVAL;

Should FAN_INFO require FAN_AUDIT?

Hi Richard,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jack-fs/fsnotify]
[also build test WARNING on pcmoore-audit/next linus/master v5.19 next-20220810] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220810-012825
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20220810/202208102231.qSUdYAdb-lkp(a)intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/a943676abc023c094f05b45f4d61936c567507a2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220810-012825 git checkout a943676abc023c094f05b45f4d61936c567507a2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/notify/fanotify/

If you fix the issue, kindly add following tag where applicable

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

Quoted 1 line

Quoted 1 line

fs/notify/fanotify/fanotify_user.c:325:35: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]

                    group, fd, response, info_buf, count);
                                                   ^~~~~
   include/linux/printk.h:594:38: note: expanded from macro 'pr_debug'
           no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
                                       ~~~     ^~~~~~~~~~~
   include/linux/printk.h:131:17: note: expanded from macro 'no_printk'
                   printk(fmt, ##__VA_ARGS__);             \
                          ~~~    ^~~~~~~~~~~
   include/linux/printk.h:464:60: note: expanded from macro 'printk'
   #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                                                       ~~~    ^~~~~~~~~~~
   include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
                   _p_func(_fmt, ##__VA_ARGS__);                           \
                           ~~~~    ^~~~~~~~~~~

1 warning generated.

vim +325 fs/notify/fanotify/fanotify_user.c

   312	
   313	static int process_access_response(struct fsnotify_group *group,
   314					   struct fanotify_response *response_struct,
   315					   const char __user *buf,
   316					   size_t count)
   317	{
   318		struct fanotify_perm_event *event;
   319		int fd = response_struct->fd;
   320		u32 response = response_struct->response;
   321		struct fanotify_response_info_header info_hdr;
   322		char *info_buf = NULL;
   323	
   324		pr_debug("%s: group=%p fd=%d response=%u buf=%p size=%lu\n", __func__,

Quoted 1 line

325 group, fd, response, info_buf, count);

326 /*
327 * make sure the response is valid, if invalid we do nothing and either
328 * userspace can send a valid response or we will clean it up after the
329 * timeout
330 */
331 if (response & ~FANOTIFY_RESPONSE_VALID_MASK)

   332			return -EINVAL;
   333		switch (response & FANOTIFY_RESPONSE_ACCESS) {
   334		case FAN_ALLOW:
   335		case FAN_DENY:
   336			break;
   337		default:
   338			return -EINVAL;
   339		}
   340		if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
   341			return -EINVAL;
   342		if (fd < 0)
   343			return -EINVAL;
   344		if (response & FAN_INFO) {
   345			size_t c = count;
   346			const char __user *ib = buf;
   347	
   348			if (c <= 0)
   349				return -EINVAL;
   350			while (c >= sizeof(info_hdr)) {
   351				if (copy_from_user(&info_hdr, ib, sizeof(info_hdr)))
   352					return -EFAULT;
   353				if (info_hdr.pad != 0)
   354					return -EINVAL;
   355				if (c < info_hdr.len)
   356					return -EINVAL;
   357				switch (info_hdr.type) {
   358				case FAN_RESPONSE_INFO_AUDIT_RULE:
   359					break;
   360				case FAN_RESPONSE_INFO_NONE:
   361				default:
   362					return -EINVAL;
   363				}
   364				c -= info_hdr.len;
   365				ib += info_hdr.len;
   366			}
   367			if (c != 0)
   368				return -EINVAL;
   369			/* Simplistic check for now */
   370			if (count != sizeof(struct fanotify_response_info_audit_rule))
   371				return -EINVAL;
   372			info_buf = kmalloc(sizeof(struct fanotify_response_info_audit_rule),
   373					   GFP_KERNEL);
   374			if (!info_buf)
   375				return -ENOMEM;
   376			if (copy_from_user(info_buf, buf, count))
   377				return -EFAULT;
   378		}
   379		spin_lock(&group->notification_lock);
   380		list_for_each_entry(event, &group->fanotify_data.access_list,
   381				    fae.fse.list) {
   382			if (event->fd != fd)
   383				continue;
   384	
   385			list_del_init(&event->fae.fse.list);
   386			/* finish_permission_event() eats info_buf */
   387			finish_permission_event(group, event, response_struct,
   388						count, info_buf);

   389			wake_up(&group->fanotify_data.access_waitq);
   390			return 0;
   391		}
   392		spin_unlock(&group->notification_lock);
   393	
   394		return -ENOENT;
   395	}
   396	

Signature · 2 lines

0-DAY CI Kernel Test Service
https://01.org/lkp

Quoted 28 lines · kernel test robot

Hi Richard,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jack-fs/fsnotify]
[also build test WARNING on pcmoore-audit/next linus/master v5.19 next-20220810] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220810-012825
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20220810/202208102231.qSUdYAdb-lkp(a)intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/a943676abc023c094f05b45f4d61936c567507a2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220810-012825 git checkout a943676abc023c094f05b45f4d61936c567507a2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/notify/fanotify/

If you fix the issue, kindly add following tag where applicable

Reported-by kernel test robot <[email protected]>

Quoted 3 lines

All warnings (new ones prefixed by >>):

Quoted 1 line

Quoted 1 line

fs/notify/fanotify/fanotify_user.c:325:35: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]

Interesting. When I "fix" it, my compiler complains:

fs/notify/fanotify/fanotify_user.c:324:11: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 8 has type ‘size_t’ {aka ‘long unsigned int’} [-Wformat=]

Quoted 106 lines

                    group, fd, response, info_buf, count);
                                                   ^~~~~
   include/linux/printk.h:594:38: note: expanded from macro 'pr_debug'
           no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
                                       ~~~     ^~~~~~~~~~~
   include/linux/printk.h:131:17: note: expanded from macro 'no_printk'
                   printk(fmt, ##__VA_ARGS__);             \
                          ~~~    ^~~~~~~~~~~
   include/linux/printk.h:464:60: note: expanded from macro 'printk'
   #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                                                       ~~~    ^~~~~~~~~~~
   include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
                   _p_func(_fmt, ##__VA_ARGS__);                           \
                           ~~~~    ^~~~~~~~~~~

1 warning generated.

vim +325 fs/notify/fanotify/fanotify_user.c

   312
   313        static int process_access_response(struct fsnotify_group *group,
   314                                           struct fanotify_response *response_struct,
   315                                           const char __user *buf,
   316                                           size_t count)
   317        {
   318                struct fanotify_perm_event *event;
   319                int fd = response_struct->fd;
   320                u32 response = response_struct->response;
   321                struct fanotify_response_info_header info_hdr;
   322                char *info_buf = NULL;
   323
   324                pr_debug("%s: group=%p fd=%d response=%u buf=%p size=%lu\n", __func__,

Quoted 1 line

325 group, fd, response, info_buf, count);

326 /*

   327                 * make sure the response is valid, if invalid we do nothing and either
   328                 * userspace can send a valid response or we will clean it up after the
   329                 * timeout
   330                 */
   331                if (response & ~FANOTIFY_RESPONSE_VALID_MASK)
   332                        return -EINVAL;
   333                switch (response & FANOTIFY_RESPONSE_ACCESS) {
   334                case FAN_ALLOW:
   335                case FAN_DENY:
   336                        break;
   337                default:
   338                        return -EINVAL;
   339                }
   340                if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
   341                        return -EINVAL;
   342                if (fd < 0)
   343                        return -EINVAL;
   344                if (response & FAN_INFO) {
   345                        size_t c = count;
   346                        const char __user *ib = buf;
   347
   348                        if (c <= 0)
   349                                return -EINVAL;
   350                        while (c >= sizeof(info_hdr)) {
   351                                if (copy_from_user(&info_hdr, ib, sizeof(info_hdr)))
   352                                        return -EFAULT;
   353                                if (info_hdr.pad != 0)
   354                                        return -EINVAL;
   355                                if (c < info_hdr.len)
   356                                        return -EINVAL;
   357                                switch (info_hdr.type) {
   358                                case FAN_RESPONSE_INFO_AUDIT_RULE:
   359                                        break;
   360                                case FAN_RESPONSE_INFO_NONE:
   361                                default:
   362                                        return -EINVAL;
   363                                }
   364                                c -= info_hdr.len;
   365                                ib += info_hdr.len;
   366                        }
   367                        if (c != 0)
   368                                return -EINVAL;
   369                        /* Simplistic check for now */
   370                        if (count != sizeof(struct fanotify_response_info_audit_rule))
   371                                return -EINVAL;
   372                        info_buf = kmalloc(sizeof(struct fanotify_response_info_audit_rule),
   373                                           GFP_KERNEL);
   374                        if (!info_buf)
   375                                return -ENOMEM;
   376                        if (copy_from_user(info_buf, buf, count))
   377                                return -EFAULT;
   378                }
   379                spin_lock(&group->notification_lock);
   380                list_for_each_entry(event, &group->fanotify_data.access_list,
   381                                    fae.fse.list) {
   382                        if (event->fd != fd)
   383                                continue;
   384
   385                        list_del_init(&event->fae.fse.list);
   386                        /* finish_permission_event() eats info_buf */
   387                        finish_permission_event(group, event, response_struct,
   388                                                count, info_buf);
   389                        wake_up(&group->fanotify_data.access_waitq);
   390                        return 0;
   391                }
   392                spin_unlock(&group->notification_lock);
   393
   394                return -ENOENT;
   395        }
   396

Quoted 4 lines

--
0-DAY CI Kernel Test Service
https://01.org/lkp

- 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

Quoted 32 lines · Richard Guy Briggs

Quoted 30 lines · kernel test robot

Hi Richard,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jack-fs/fsnotify]
[also build test WARNING on pcmoore-audit/next linus/master v5.19 next-20220810] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220810-012825
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20220810/202208102231.qSUdYAdb-lkp(a)intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/a943676abc023c094f05b45f4d61936c567507a2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220810-012825 git checkout a943676abc023c094f05b45f4d61936c567507a2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/notify/fanotify/

If you fix the issue, kindly add following tag where applicable

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

Quoted 5 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

fs/notify/fanotify/fanotify_user.c:325:35: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]

Interesting. When I "fix" it, my compiler complains:

fs/notify/fanotify/fanotify_user.c:324:11: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 8 has type ‘size_t’ {aka ‘long unsigned int’} [-Wformat=]

The correct format specifier for size_t is %zu. This avoids issues between ILP32 vs LP64 targets.

Quoted 112 lines

Quoted 105 lines

                    group, fd, response, info_buf, count);
                                                   ^~~~~
   include/linux/printk.h:594:38: note: expanded from macro 'pr_debug'
           no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
                                       ~~~     ^~~~~~~~~~~
   include/linux/printk.h:131:17: note: expanded from macro 'no_printk'
                   printk(fmt, ##__VA_ARGS__);             \
                          ~~~    ^~~~~~~~~~~
   include/linux/printk.h:464:60: note: expanded from macro 'printk'
   #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                                                       ~~~    ^~~~~~~~~~~
   include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
                   _p_func(_fmt, ##__VA_ARGS__);                           \
                           ~~~~    ^~~~~~~~~~~

1 warning generated.

vim +325 fs/notify/fanotify/fanotify_user.c

   312
   313        static int process_access_response(struct fsnotify_group *group,
   314                                           struct fanotify_response *response_struct,
   315                                           const char __user *buf,
   316                                           size_t count)
   317        {
   318                struct fanotify_perm_event *event;
   319                int fd = response_struct->fd;
   320                u32 response = response_struct->response;
   321                struct fanotify_response_info_header info_hdr;
   322                char *info_buf = NULL;
   323
   324                pr_debug("%s: group=%p fd=%d response=%u buf=%p size=%lu\n", __func__,

Quoted 1 line

325 group, fd, response, info_buf, count);

326 /*

   327                 * make sure the response is valid, if invalid we do nothing and either
   328                 * userspace can send a valid response or we will clean it up after the
   329                 * timeout
   330                 */
   331                if (response & ~FANOTIFY_RESPONSE_VALID_MASK)
   332                        return -EINVAL;
   333                switch (response & FANOTIFY_RESPONSE_ACCESS) {
   334                case FAN_ALLOW:
   335                case FAN_DENY:
   336                        break;
   337                default:
   338                        return -EINVAL;
   339                }
   340                if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
   341                        return -EINVAL;
   342                if (fd < 0)
   343                        return -EINVAL;
   344                if (response & FAN_INFO) {
   345                        size_t c = count;
   346                        const char __user *ib = buf;
   347
   348                        if (c <= 0)
   349                                return -EINVAL;
   350                        while (c >= sizeof(info_hdr)) {
   351                                if (copy_from_user(&info_hdr, ib, sizeof(info_hdr)))
   352                                        return -EFAULT;
   353                                if (info_hdr.pad != 0)
   354                                        return -EINVAL;
   355                                if (c < info_hdr.len)
   356                                        return -EINVAL;
   357                                switch (info_hdr.type) {
   358                                case FAN_RESPONSE_INFO_AUDIT_RULE:
   359                                        break;
   360                                case FAN_RESPONSE_INFO_NONE:
   361                                default:
   362                                        return -EINVAL;
   363                                }
   364                                c -= info_hdr.len;
   365                                ib += info_hdr.len;
   366                        }
   367                        if (c != 0)
   368                                return -EINVAL;
   369                        /* Simplistic check for now */
   370                        if (count != sizeof(struct fanotify_response_info_audit_rule))
   371                                return -EINVAL;
   372                        info_buf = kmalloc(sizeof(struct fanotify_response_info_audit_rule),
   373                                           GFP_KERNEL);
   374                        if (!info_buf)
   375                                return -ENOMEM;
   376                        if (copy_from_user(info_buf, buf, count))
   377                                return -EFAULT;
   378                }
   379                spin_lock(&group->notification_lock);
   380                list_for_each_entry(event, &group->fanotify_data.access_list,
   381                                    fae.fse.list) {
   382                        if (event->fd != fd)
   383                                continue;
   384
   385                        list_del_init(&event->fae.fse.list);
   386                        /* finish_permission_event() eats info_buf */
   387                        finish_permission_event(group, event, response_struct,
   388                                                count, info_buf);
   389                        wake_up(&group->fanotify_data.access_waitq);
   390                        return 0;
   391                }
   392                spin_unlock(&group->notification_lock);
   393
   394                return -ENOENT;
   395        }
   396

Quoted 4 lines

--
0-DAY CI Kernel Test Service
https://01.org/lkp

- RGB

Quoted 8 lines

--
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

Signature · 2 lines

Thanks,
~Nick Desaulniers

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, an audit 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 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]>

---

 fs/notify/fanotify/fanotify.c      |  10 ++-
 fs/notify/fanotify/fanotify.h      |   2 +
 fs/notify/fanotify/fanotify_user.c | 104 +++++++++++++++++++++++------
 include/linux/fanotify.h           |   5 ++
 include/uapi/linux/fanotify.h      |  27 +++++++-
 5 files changed, 123 insertions(+), 25 deletions(-)

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

index 4f897e109547..0f36062521f4 100644
@@ -262,13 +262,16 @@ 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) {

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

 	/* Check if the response should be audited */

@@ -560,6 +563,8 @@ static struct fanotify_event *fanotify_alloc_perm_event(const struct path *path,
 
 	pevent->fae.type = FANOTIFY_EVENT_TYPE_PATH_PERM;
 	pevent->response = 0;
+	pevent->info_len = 0;
+	pevent->info_buf = NULL;
 	pevent->state = FAN_EVENT_INIT;
 	pevent->path = *path;
 	path_get(path);

@@ -996,6 +1001,7 @@ static void fanotify_free_path_event(struct fanotify_event *event)
 static void fanotify_free_perm_event(struct fanotify_event *event)
 {
 	path_put(fanotify_event_path(event));
+	kfree(FANOTIFY_PERM(event)->info_buf);
 	kmem_cache_free(fanotify_perm_event_cachep, FANOTIFY_PERM(event));
 }
 

fs/notify/fanotify/fanotify.h +2 -0

index abfa3712c185..14c30e173632 100644
@@ -428,6 +428,8 @@ 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 */
+	size_t info_len;
+	char *info_buf;
 };
 
 static inline struct fanotify_perm_event *

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

index ff67ca0d25cc..a4ae953f0e62 100644
@@ -289,13 +289,18 @@ 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,
+				    size_t info_len, char *info_buf)
 				    __releases(&group->notification_lock)
 {
 	bool destroy = false;
 
 	assert_spin_locked(&group->notification_lock);
-	event->response = response;
+	event->response = response->response & ~FAN_INFO;
+	if (response->response & FAN_INFO) {
+		event->info_len = info_len;
+		event->info_buf = info_buf;
+	}
 	if (event->state == FAN_EVENT_CANCELED)
 		destroy = true;
 	else

@@ -306,33 +311,71 @@ static void finish_permission_event(struct fsnotify_group *group,
 }
 

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

 	/*
 	 * 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;
 	default:
 		return -EINVAL;
 	}
-

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

+		while (c >= sizeof(info_hdr)) {
+			if (copy_from_user(&info_hdr, ib, sizeof(info_hdr)))
+		 		return -EFAULT;
+			if (info_hdr.pad != 0)
+				return -EINVAL;
+			if (c < info_hdr.len)
+				return -EINVAL;
+			switch (info_hdr.type) {
+			case FAN_RESPONSE_INFO_AUDIT_RULE:
+				break;
+			case FAN_RESPONSE_INFO_NONE:
+			default:

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

+		finish_permission_event(group, event, response_struct,
+					count, info_buf);
 		wake_up(&group->fanotify_data.access_waitq);
 		return 0;
 	}

@@ -802,9 +847,14 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
 			fsnotify_destroy_event(group, &event->fse);
 		} else {
 			if (ret <= 0) {
+				struct fanotify_response response = {
+					.fd = FAN_NOFD,
+					.response = FAN_DENY };
+
 				spin_lock(&group->notification_lock);
 				finish_permission_event(group,
-					FANOTIFY_PERM(event), FAN_DENY);
+					FANOTIFY_PERM(event), &response,
+					0, NULL);
 				wake_up(&group->fanotify_data.access_waitq);
 			} else {
 				spin_lock(&group->notification_lock);

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

 	if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
 		return -EINVAL;
 
 	group = file->private_data;
 
-	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 (count < sizeof(response))
+		return -EINVAL;
+	if (copy_from_user(&response, buf, sizeof(response)))

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

 	if (ret < 0)
 		count = ret;
 

@@ -857,6 +914,9 @@ static int fanotify_release(struct inode *ignored, struct file *file)
 {
 	struct fsnotify_group *group = file->private_data;
 	struct fsnotify_event *fsn_event;
+	struct fanotify_response response = {
+		.fd = FAN_NOFD,
+		.response = FAN_ALLOW };
 
 	/*
 	 * Stop new events from arriving in the notification queue. since

@@ -876,7 +936,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, &response, 0, NULL);
 		spin_lock(&group->notification_lock);
 	}
 

@@ -893,7 +953,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);
+						&response, 0, NULL);
 		}
 		spin_lock(&group->notification_lock);
 	}

include/linux/fanotify.h +0 -0

index edc28555814c..ce9f97eb69f2 100644

include/linux/fanotify.h +4 -0

+#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

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

index f1f89132d60e..4d08823a5698 100644
@@ -180,15 +180,40 @@ struct fanotify_event_info_error {
 	__u32 error_count;

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

+#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 audit_rule;
+};
+
 /* 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

Signature · 1 line

2.27.0

Quoted 14 lines · Richard Guy Briggs

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, an audit 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 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]>

---

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

Quoted 10 lines

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

@@ -827,26 +877,33 @@ 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 c;

Can we rename this to something like len or info_len instead? I dislike single character variable names outside of the scope of things like loops.

Quoted 16 lines

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

 	if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
 		return -EINVAL;
 
 	group = file->private_data;
 
-	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 (count < sizeof(response))
+		return -EINVAL;
+	if (copy_from_user(&response, buf, sizeof(response)))

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

Quoted 10 lines

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

 		return -EFAULT;
 
-	ret = process_access_response(group, &response);
+	c = count - sizeof(response);
+	if (response.response & FAN_INFO) {
+		if (c < sizeof(struct fanotify_response_info_header))
+			return -EINVAL;
+	} else {
+		if (c != 0)
+			return -EINVAL;

Hm, prior to this change we truncated the copy operation to the sizeof(struct fanotify_response) and didn't care if there maybe was extra data supplied in the buf or count > sizeof(struct fanotify_response). This leaves me wondering whether this check is needed for cases that are not (FAN_INFO | FAN_AUDIT)? The buf may still hold a valid fanotify_response despite buf/count possibly being larger than sizeof(struct fanotify_response)... I can see why you'd want to enforce this, but I'm wondering if it might break things if event listeners are responding to the permission events in an awkward way i.e. by calculating and supplying count incorrectly.

Also, if we do decide to keep this check around, then maybe it can be simplified into an else if instead?

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, an audit 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 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]>

---
 fs/notify/fanotify/fanotify.c      |  10 ++-
 fs/notify/fanotify/fanotify.h      |   2 +
 fs/notify/fanotify/fanotify_user.c | 104 +++++++++++++++++++++++------
 include/linux/fanotify.h           |   5 ++
 include/uapi/linux/fanotify.h      |  27 +++++++-
 5 files changed, 123 insertions(+), 25 deletions(-)

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

+	if (response->response & FAN_INFO) {
+		event->info_len = info_len;
+		event->info_buf = info_buf;
+	}
 	if (event->state == FAN_EVENT_CANCELED)
 		destroy = true;
 	else

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

index f1f89132d60e..4d08823a5698 100644
@@ -180,15 +180,40 @@ struct fanotify_event_info_error {
 	__u32 error_count;
 };
 
+/*

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

+#define FAN_RESPONSE_INFO_AUDIT_RULE	1
+
 struct fanotify_response {
 	__s32 fd;
 	__u32 response;
 };
 
+struct fanotify_response_info_header {

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_info=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]>

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

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

index 0f36062521f4..36c3ed1af085 100644
@@ -276,7 +276,8 @@ static int fanotify_get_response(struct fsnotify_group *group,
 
        /* Check if the response should be audited */
        if (event->response & FAN_AUDIT)
-               audit_fanotify(event->response & ~FAN_AUDIT);
+               audit_fanotify(event->response & ~FAN_AUDIT,
+                              event->info_len, event->info_buf);
 
        pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__,
                 group, event, ret);

include/linux/audit.h +5 -4

index 3ea198a2cd59..c69efdba12ca 100644
@@ -14,6 +14,7 @@
 #include <linux/audit_arch.h>
 #include <uapi/linux/audit.h>
 #include <uapi/linux/netfilter/nf_tables.h>
+#include <uapi/linux/fanotify.h>
 
 #define AUDIT_INO_UNSET ((unsigned long)-1)
 #define AUDIT_DEV_UNSET ((dev_t)-1)

@@ -417,7 +418,7 @@ extern void __audit_log_capset(const struct cred *new, const struct cred *old);
 extern void __audit_mmap_fd(int fd, int flags);
 extern void __audit_openat2_how(struct open_how *how);
 extern void __audit_log_kern_module(char *name);
-extern void __audit_fanotify(u32 response);
+extern void __audit_fanotify(u32 response, size_t len, char *buf);
 extern void __audit_tk_injoffset(struct timespec64 offset);
 extern void __audit_ntp_log(const struct audit_ntp_data *ad);
 extern void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,

@@ -524,10 +525,10 @@ static inline void audit_log_kern_module(char *name)
                __audit_log_kern_module(name);
 }
 
-static inline void audit_fanotify(u32 response)
+static inline void audit_fanotify(u32 response, size_t len, char *buf)
 {
        if (!audit_dummy_context())
-               __audit_fanotify(response);
+               __audit_fanotify(response, len, buf);
 }
 
 static inline void audit_tk_injoffset(struct timespec64 offset)

@@ -684,7 +685,7 @@ static inline void audit_log_kern_module(char *name)
 {
 }
 
-static inline void audit_fanotify(u32 response)
+static inline void audit_fanotify(u32 response, size_t len, char *buf)
 { }
 
 static inline void audit_tk_injoffset(struct timespec64 offset)

kernel/auditsc.c +3 -3

index 433418d73584..f000fec52360 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"
 

@@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
        context->type = AUDIT_KERN_MODULE;
 }
 
-void __audit_fanotify(u32 response)
+void __audit_fanotify(u32 response, size_t len, char *buf)
 {
-       audit_log(audit_context(), GFP_KERNEL,
-               AUDIT_FANOTIFY, "resp=%u", response);
+       struct fanotify_response_info_audit_rule *friar;

kernel/auditsc.c +15 -1

+               switch (friar->hdr.type) {
+               case FAN_RESPONSE_INFO_AUDIT_RULE:
+                       if (friar->hdr.len < sizeof(*friar)) {
+                               audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
+                                         "resp=%u fan_type=%u fan_info=(incomplete)",
+                                         response, friar->hdr.type);
+                               return;
+                       }
+                       audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
+                                 "resp=%u fan_type=%u fan_info=%u",
+                                 response, friar->hdr.type, friar->audit_rule);
+               }
+               c -= friar->hdr.len;
+               ib += friar->hdr.len;
+       }
 }
 
 void __audit_tk_injoffset(struct timespec64 offset)
--

2.27.0

Quoted 8 lines · Richard Guy Briggs

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_info=17

Quoted 8 lines

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]>

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

You've hopefully already seen the kernel test robot build warning, so I won't bring that up again, but a few comments below ...

Quoted 30 lines

kernel/auditsc.c +11 -3

index 433418d73584..f000fec52360 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"
 

@@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
        context->type = AUDIT_KERN_MODULE;
 }
 
-void __audit_fanotify(u32 response)
+void __audit_fanotify(u32 response, size_t len, char *buf)
 {
-       audit_log(audit_context(), GFP_KERNEL,
-               AUDIT_FANOTIFY, "resp=%u", response);
+       struct fanotify_response_info_audit_rule *friar;
+       size_t c = len;
+       char *ib = buf;
+
+       if (!(len && buf)) {
+               audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
+                         "resp=%u fan_type=0 fan_info=?", response);
+               return;
+       }

kernel/auditsc.c void __audit_log_kern_module(char *name) · line 2915

Quoted 2 lines

kernel/auditsc.c +2 -0

+       while (c >= sizeof(struct fanotify_response_info_header)) {
+               friar = (struct fanotify_response_info_audit_rule *)buf;

Since the only use of this at the moment is the fanotify_response_info_rule, why not pass the fanotify_response_info_rule struct directly into this function? We can always change it if we need to in the future without affecting userspace, and it would simplify the code.

Quoted 17 lines

kernel/auditsc.c +15 -0

+               switch (friar->hdr.type) {
+               case FAN_RESPONSE_INFO_AUDIT_RULE:
+                       if (friar->hdr.len < sizeof(*friar)) {
+                               audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
+                                         "resp=%u fan_type=%u fan_info=(incomplete)",
+                                         response, friar->hdr.type);
+                               return;
+                       }
+                       audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
+                                 "resp=%u fan_type=%u fan_info=%u",
+                                 response, friar->hdr.type, friar->audit_rule);
+               }
+               c -= friar->hdr.len;
+               ib += friar->hdr.len;
+       }
 }
 

kernel/auditsc.c void __audit_log_kern_module(char *name) · line 2933

Quoted 1 line

kernel/auditsc.c +0 -0

 void __audit_tk_injoffset(struct timespec64 offset)

paul-moore.com

Quoted 31 lines

Quoted 31 lines

kernel/auditsc.c +12 -3

index 433418d73584..f000fec52360 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"
 

@@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
 context->type = AUDIT_KERN_MODULE;
 }
 
-void __audit_fanotify(u32 response)
+void __audit_fanotify(u32 response, size_t len, char *buf)
 {
-       audit_log(audit_context(), GFP_KERNEL,
-               AUDIT_FANOTIFY, "resp=%u", response);
+       struct fanotify_response_info_audit_rule *friar;
+       size_t c = len;
+       char *ib = buf;
+
+       if (!(len && buf)) {
+               audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
+                         "resp=%u fan_type=0 fan_info=?", response);
+               return;
+       }
+       while (c >= sizeof(struct fanotify_response_info_header)) {

Quoted 8 lines

kernel/auditsc.c void __audit_log_kern_module(char *name) · line 2916

Quoted 2 lines

kernel/auditsc.c +1 -0

+               friar = (struct fanotify_response_info_audit_rule
 *)buf;

Since the only use of this at the moment is the fanotify_response_info_rule, why not pass the fanotify_response_info_rule struct directly into this function? We can always change it if we need to in the future without affecting userspace, and it would simplify the code.

Steve, would it make any sense for there to be more than one FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more than one rule that contributes to a notify reason? If not, would it be reasonable to return -EINVAL if there is more than one?

Quoted 40 lines · EDT Richard Guy Briggs

Quoted 31 lines

Quoted 31 lines

kernel/auditsc.c +12 -3

index 433418d73584..f000fec52360 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"
 

@@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
 context->type = AUDIT_KERN_MODULE;
 }
 
-void __audit_fanotify(u32 response)
+void __audit_fanotify(u32 response, size_t len, char *buf)
 {
-       audit_log(audit_context(), GFP_KERNEL,
-               AUDIT_FANOTIFY, "resp=%u", response);
+       struct fanotify_response_info_audit_rule *friar;
+       size_t c = len;
+       char *ib = buf;
+
+       if (!(len && buf)) {
+               audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
+                         "resp=%u fan_type=0 fan_info=?", response);
+               return;
+       }
+       while (c >= sizeof(struct fanotify_response_info_header)) {

Quoted 8 lines

kernel/auditsc.c void __audit_log_kern_module(char *name) · line 2916

Quoted 2 lines

kernel/auditsc.c +1 -0

+               friar = (struct fanotify_response_info_audit_rule
 *)buf;

Since the only use of this at the moment is the fanotify_response_info_rule, why not pass the fanotify_response_info_rule struct directly into this function? We can always change it if we need to in the future without affecting userspace, and it would simplify the code.

Quoted 4 lines

Steve, would it make any sense for there to be more than one FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more than one rule that contributes to a notify reason? If not, would it be reasonable to return -EINVAL if there is more than one?

I don't see a reason for sending more than one header. What is more probable is the need to send additional data in that header. I was thinking of maybe bit mapping it in the rule number. But I'd suggest padding the struct just in case it needs expanding some day.

-Steev

Quoted 48 lines · Steve Grubb

Quoted 46 lines · EDT Richard Guy Briggs

Quoted 33 lines

Quoted 33 lines

kernel/auditsc.c +12 -3

index 433418d73584..f000fec52360 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"
 

@@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
 context->type = AUDIT_KERN_MODULE;
 }
 
-void __audit_fanotify(u32 response)
+void __audit_fanotify(u32 response, size_t len, char *buf)
 {
-       audit_log(audit_context(), GFP_KERNEL,
-               AUDIT_FANOTIFY, "resp=%u", response);
+       struct fanotify_response_info_audit_rule *friar;
+       size_t c = len;
+       char *ib = buf;
+
+       if (!(len && buf)) {
+               audit_log(audit_context(), GFP_KERNEL,
 AUDIT_FANOTIFY,
+                         "resp=%u fan_type=0 fan_info=?",
 response);
+               return;
+       }
+       while (c >= sizeof(struct fanotify_response_info_header)) {

Quoted 8 lines

kernel/auditsc.c void __audit_log_kern_module(char *name) · line 2918

Quoted 2 lines

kernel/auditsc.c +1 -0

+               friar = (struct fanotify_response_info_audit_rule
 *)buf;

Since the only use of this at the moment is the fanotify_response_info_rule, why not pass the fanotify_response_info_rule struct directly into this function? We can always change it if we need to in the future without affecting userspace, and it would simplify the code.

Steve, would it make any sense for there to be more than one FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more than one rule that contributes to a notify reason? If not, would it be reasonable to return -EINVAL if there is more than one?

Quoted 4 lines

I don't see a reason for sending more than one header. What is more probable is the need to send additional data in that header. I was thinking of maybe bit mapping it in the rule number. But I'd suggest padding the struct just in case it needs expanding some day.

This doesn't exactly answer my question about multiple rules contributing to one decision.

The need for more as yet undefined information sounds like a good reason to define a new header if that happens.

At this point, is it reasonable to throw an error if more than one RULE header appears in a message?

The way I had coded this last patchset was to allow for more than one RULE header and each one would get its own record in the event.

How many rules total are likely to exist?

Quoted 49 lines · EDT Richard Guy Briggs

Quoted 48 lines · Steve Grubb

Quoted 46 lines · EDT Richard Guy Briggs

Quoted 33 lines

Quoted 33 lines

kernel/auditsc.c +12 -3

index 433418d73584..f000fec52360 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"
 

@@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
 context->type = AUDIT_KERN_MODULE;
 }
 
-void __audit_fanotify(u32 response)
+void __audit_fanotify(u32 response, size_t len, char *buf)
 {
-       audit_log(audit_context(), GFP_KERNEL,
-               AUDIT_FANOTIFY, "resp=%u", response);
+       struct fanotify_response_info_audit_rule *friar;
+       size_t c = len;
+       char *ib = buf;
+
+       if (!(len && buf)) {
+               audit_log(audit_context(), GFP_KERNEL,
 AUDIT_FANOTIFY,
+                         "resp=%u fan_type=0 fan_info=?",
 response);
+               return;
+       }
+       while (c >= sizeof(struct fanotify_response_info_header)) {

Quoted 8 lines

kernel/auditsc.c void __audit_log_kern_module(char *name) · line 2918

Quoted 2 lines

kernel/auditsc.c +1 -0

+               friar = (struct fanotify_response_info_audit_rule
 *)buf;

Since the only use of this at the moment is the fanotify_response_info_rule, why not pass the fanotify_response_info_rule struct directly into this function? We can always change it if we need to in the future without affecting userspace, and it would simplify the code.

Steve, would it make any sense for there to be more than one FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more than one rule that contributes to a notify reason? If not, would it be reasonable to return -EINVAL if there is more than one?

Quoted 7 lines

Quoted 4 lines

I don't see a reason for sending more than one header. What is more probable is the need to send additional data in that header. I was thinking of maybe bit mapping it in the rule number. But I'd suggest padding the struct just in case it needs expanding some day.

This doesn't exactly answer my question about multiple rules contributing to one decision.

I don't forsee that.

Quoted 2 lines

The need for more as yet undefined information sounds like a good reason to define a new header if that happens.

It's much better to pad the struct so that the size doesn't change.

Quoted 2 lines

At this point, is it reasonable to throw an error if more than one RULE header appears in a message?

It is a write syscall. I'd silently discard everything else and document that in the man pages. But the fanotify maintainers should really weigh in on this.

Quoted 2 lines

The way I had coded this last patchset was to allow for more than one RULE header and each one would get its own record in the event.

I do not forsee a need for this.

Quoted 1 line

How many rules total are likely to exist?

Could be a thousand. But I already know some missing information we'd like to return to user space in an audit event, so the bit mapping on the rule number might happen. I'd suggest padding one u32 for future use.

Quoted 79 lines · Steve Grubb

Quoted 49 lines · EDT Richard Guy Briggs

Quoted 48 lines · Steve Grubb

Quoted 46 lines · EDT Richard Guy Briggs

Quoted 33 lines

Quoted 33 lines

kernel/auditsc.c +12 -3

index 433418d73584..f000fec52360 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"
 

@@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
 context->type = AUDIT_KERN_MODULE;
 }
 
-void __audit_fanotify(u32 response)
+void __audit_fanotify(u32 response, size_t len, char *buf)
 {
-       audit_log(audit_context(), GFP_KERNEL,
-               AUDIT_FANOTIFY, "resp=%u", response);
+       struct fanotify_response_info_audit_rule *friar;
+       size_t c = len;
+       char *ib = buf;
+
+       if (!(len && buf)) {
+               audit_log(audit_context(), GFP_KERNEL,
 AUDIT_FANOTIFY,
+                         "resp=%u fan_type=0 fan_info=?",
 response);
+               return;
+       }
+       while (c >= sizeof(struct fanotify_response_info_header)) {

Quoted 8 lines

kernel/auditsc.c void __audit_log_kern_module(char *name) · line 2918

Quoted 2 lines

kernel/auditsc.c +1 -0

+               friar = (struct fanotify_response_info_audit_rule
 *)buf;

Since the only use of this at the moment is the fanotify_response_info_rule, why not pass the fanotify_response_info_rule struct directly into this function? We can always change it if we need to in the future without affecting userspace, and it would simplify the code.

Steve, would it make any sense for there to be more than one FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more than one rule that contributes to a notify reason? If not, would it be reasonable to return -EINVAL if there is more than one?

Quoted 7 lines

Quoted 4 lines

I don't see a reason for sending more than one header. What is more probable is the need to send additional data in that header. I was thinking of maybe bit mapping it in the rule number. But I'd suggest padding the struct just in case it needs expanding some day.

This doesn't exactly answer my question about multiple rules contributing to one decision.

I don't forsee that.

Quoted 2 lines

The need for more as yet undefined information sounds like a good reason to define a new header if that happens.

It's much better to pad the struct so that the size doesn't change.

Quoted 2 lines

At this point, is it reasonable to throw an error if more than one RULE header appears in a message?

It is a write syscall. I'd silently discard everything else and document that in the man pages. But the fanotify maintainers should really weigh in on this.

Quoted 2 lines

The way I had coded this last patchset was to allow for more than one RULE header and each one would get its own record in the event.

I do not forsee a need for this.

Quoted 1 line

How many rules total are likely to exist?

Quoted 3 lines

Could be a thousand. But I already know some missing information we'd like to return to user space in an audit event, so the bit mapping on the rule number might happen. I'd suggest padding one u32 for future use.

A better way to handle an expansion like that would be to have a length/version field at the top of the struct that could be used to determine the size and layout of the struct.

Quoted 80 lines · Paul Moore

Quoted 79 lines · Steve Grubb

Quoted 49 lines · EDT Richard Guy Briggs

Quoted 48 lines · Steve Grubb

Quoted 46 lines · EDT Richard Guy Briggs

Quoted 33 lines

Quoted 33 lines

kernel/auditsc.c +12 -3

index 433418d73584..f000fec52360 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"
 

@@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
 context->type = AUDIT_KERN_MODULE;
 }
 
-void __audit_fanotify(u32 response)
+void __audit_fanotify(u32 response, size_t len, char *buf)
 {
-       audit_log(audit_context(), GFP_KERNEL,
-               AUDIT_FANOTIFY, "resp=%u", response);
+       struct fanotify_response_info_audit_rule *friar;
+       size_t c = len;
+       char *ib = buf;
+
+       if (!(len && buf)) {
+               audit_log(audit_context(), GFP_KERNEL,
 AUDIT_FANOTIFY,
+                         "resp=%u fan_type=0 fan_info=?",
 response);
+               return;
+       }
+       while (c >= sizeof(struct fanotify_response_info_header)) {

Quoted 8 lines

kernel/auditsc.c void __audit_log_kern_module(char *name) · line 2918

Quoted 2 lines

kernel/auditsc.c +1 -0

+               friar = (struct fanotify_response_info_audit_rule
 *)buf;

Since the only use of this at the moment is the fanotify_response_info_rule, why not pass the fanotify_response_info_rule struct directly into this function? We can always change it if we need to in the future without affecting userspace, and it would simplify the code.

Steve, would it make any sense for there to be more than one FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more than one rule that contributes to a notify reason? If not, would it be reasonable to return -EINVAL if there is more than one?

Quoted 7 lines

Quoted 4 lines

I don't see a reason for sending more than one header. What is more probable is the need to send additional data in that header. I was thinking of maybe bit mapping it in the rule number. But I'd suggest padding the struct just in case it needs expanding some day.

This doesn't exactly answer my question about multiple rules contributing to one decision.

I don't forsee that.

Quoted 2 lines

The need for more as yet undefined information sounds like a good reason to define a new header if that happens.

It's much better to pad the struct so that the size doesn't change.

Quoted 2 lines

At this point, is it reasonable to throw an error if more than one RULE header appears in a message?

It is a write syscall. I'd silently discard everything else and document that in the man pages. But the fanotify maintainers should really weigh in on this.

Quoted 2 lines

The way I had coded this last patchset was to allow for more than one RULE header and each one would get its own record in the event.

I do not forsee a need for this.

Quoted 1 line

How many rules total are likely to exist?

Quoted 7 lines

Quoted 3 lines

Could be a thousand. But I already know some missing information we'd like to return to user space in an audit event, so the bit mapping on the rule number might happen. I'd suggest padding one u32 for future use.

A better way to handle an expansion like that would be to have a length/version field at the top of the struct that could be used to determine the size and layout of the struct.

We already do have the 'type' and 'len' fields in
struct fanotify_response_info_header. So if audit needs to pass more information, we can define a new 'type' and either make it replace the current struct fanotify_response_info_audit_rule or make it expand the information in it. At least this is how we handle similar situation when fanotify wants to report some new bits of information to userspace.

That being said if audit wants to have u32 pad in its struct fanotify_response_info_audit_rule for future "optional" expansion I'm not strictly opposed to that but I don't think it is a good idea.

Ultimately I guess I'll leave it upto audit subsystem what it wants to have in its struct fanotify_response_info_audit_rule because for fanotify subsystem, it is just an opaque blob it is passing.

Quoted 89 lines · Jan Kara

Quoted 80 lines · Paul Moore

Quoted 79 lines · Steve Grubb

Quoted 49 lines · EDT Richard Guy Briggs

Quoted 48 lines · Steve Grubb

Quoted 46 lines · EDT Richard Guy Briggs

Quoted 33 lines

Quoted 33 lines

kernel/auditsc.c +12 -3

index 433418d73584..f000fec52360 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"
 

@@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
 context->type = AUDIT_KERN_MODULE;
 }
 
-void __audit_fanotify(u32 response)
+void __audit_fanotify(u32 response, size_t len, char *buf)
 {
-       audit_log(audit_context(), GFP_KERNEL,
-               AUDIT_FANOTIFY, "resp=%u", response);
+       struct fanotify_response_info_audit_rule *friar;
+       size_t c = len;
+       char *ib = buf;
+
+       if (!(len && buf)) {
+               audit_log(audit_context(), GFP_KERNEL,
 AUDIT_FANOTIFY,
+                         "resp=%u fan_type=0 fan_info=?",
 response);
+               return;
+       }
+       while (c >= sizeof(struct fanotify_response_info_header)) {

Quoted 8 lines

kernel/auditsc.c void __audit_log_kern_module(char *name) · line 2918

Quoted 2 lines

kernel/auditsc.c +1 -0

+               friar = (struct fanotify_response_info_audit_rule
 *)buf;

Since the only use of this at the moment is the fanotify_response_info_rule, why not pass the fanotify_response_info_rule struct directly into this function? We can always change it if we need to in the future without affecting userspace, and it would simplify the code.

Steve, would it make any sense for there to be more than one FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more than one rule that contributes to a notify reason? If not, would it be reasonable to return -EINVAL if there is more than one?

Quoted 7 lines

Quoted 4 lines

I don't see a reason for sending more than one header. What is more probable is the need to send additional data in that header. I was thinking of maybe bit mapping it in the rule number. But I'd suggest padding the struct just in case it needs expanding some day.

This doesn't exactly answer my question about multiple rules contributing to one decision.

I don't forsee that.

Quoted 2 lines

The need for more as yet undefined information sounds like a good reason to define a new header if that happens.

It's much better to pad the struct so that the size doesn't change.

Quoted 2 lines

At this point, is it reasonable to throw an error if more than one RULE header appears in a message?

It is a write syscall. I'd silently discard everything else and document that in the man pages. But the fanotify maintainers should really weigh in on this.

Quoted 2 lines

The way I had coded this last patchset was to allow for more than one RULE header and each one would get its own record in the event.

I do not forsee a need for this.

Quoted 1 line

How many rules total are likely to exist?

Quoted 7 lines

Quoted 3 lines

Could be a thousand. But I already know some missing information we'd like to return to user space in an audit event, so the bit mapping on the rule number might happen. I'd suggest padding one u32 for future use.

A better way to handle an expansion like that would be to have a length/version field at the top of the struct that could be used to determine the size and layout of the struct.

Quoted 6 lines

We already do have the 'type' and 'len' fields in
struct fanotify_response_info_header. So if audit needs to pass more information, we can define a new 'type' and either make it replace the current struct fanotify_response_info_audit_rule or make it expand the information in it. At least this is how we handle similar situation when fanotify wants to report some new bits of information to userspace.

Perfect, I didn't know that was an option from the fanotify side; I agree that's the right approach.

Quoted 3 lines

That being said if audit wants to have u32 pad in its struct fanotify_response_info_audit_rule for future "optional" expansion I'm not strictly opposed to that but I don't think it is a good idea.

Yes, I'm not a fan of padding out this way, especially when we have better options.

Quoted 3 lines

Ultimately I guess I'll leave it upto audit subsystem what it wants to have in its struct fanotify_response_info_audit_rule because for fanotify subsystem, it is just an opaque blob it is passing.

In that case, let's stick with leveraging the type/len fields in the fanotify_response_info_header struct, that should give us all the flexibility we need.

Richard and Steve, it sounds like Steve is already aware of additional information that he wants to send via the fanotify_response_info_audit_rule struct, please include that in the next revision of this patchset. I don't want to get this merged and then soon after have to hack in additional info.

paul-moore.com

Quoted 8 lines

Quoted 3 lines

Ultimately I guess I'll leave it upto audit subsystem what it wants to have in its struct fanotify_response_info_audit_rule because for fanotify subsystem, it is just an opaque blob it is passing.

In that case, let's stick with leveraging the type/len fields in the fanotify_response_info_header struct, that should give us all the flexibility we need.

Quoted 5 lines

Richard and Steve, it sounds like Steve is already aware of additional information that he wants to send via the fanotify_response_info_audit_rule struct, please include that in the next revision of this patchset. I don't want to get this merged and then soon after have to hack in additional info.

Steve, please define the type and name of this additional field.

Quoted 14 lines · EDT Richard Guy Briggs

Quoted 13 lines

Quoted 3 lines

Ultimately I guess I'll leave it upto audit subsystem what it wants to have in its struct fanotify_response_info_audit_rule because for fanotify subsystem, it is just an opaque blob it is passing.

In that case, let's stick with leveraging the type/len fields in the fanotify_response_info_header struct, that should give us all the flexibility we need.

Richard and Steve, it sounds like Steve is already aware of additional information that he wants to send via the fanotify_response_info_audit_rule struct, please include that in the next revision of this patchset. I don't want to get this merged and then soon after have to hack in additional info.

Quoted 1 line

Steve, please define the type and name of this additional field.

Maybe extra_data, app_data, or extra_info. Something generic that can be reused by any application. Default to 0 if not present.

Quoted 17 lines · Steve Grubb

Quoted 16 lines · EDT Richard Guy Briggs

Quoted 15 lines

Quoted 4 lines

Ultimately I guess I'll leave it upto audit subsystem what it wants to
have in its struct fanotify_response_info_audit_rule because for fanotify subsystem, it is just an opaque blob it is passing.

In that case, let's stick with leveraging the type/len fields in the fanotify_response_info_header struct, that should give us all the flexibility we need.

Richard and Steve, it sounds like Steve is already aware of additional
information that he wants to send via the fanotify_response_info_audit_rule struct, please include that in the next revision of this patchset. I don't want to get this merged and then soon after have to hack in additional info.

Quoted 4 lines

Quoted 1 line

Steve, please define the type and name of this additional field.

Maybe extra_data, app_data, or extra_info. Something generic that can be reused by any application. Default to 0 if not present.

I think the point is being missed ... The idea is to not speculate on additional fields, as discussed we have ways to handle that, the issue was that Steve implied that he already had ideas for "things" he wanted to add. If there are "things" that need to be added, let's do that now, however if there is just speculation that maybe someday we might need to add something else we can leave that until later.

Quoted 23 lines · EDT Paul Moore

Quoted 17 lines · Steve Grubb

Quoted 16 lines · EDT Richard Guy Briggs

Quoted 15 lines

Quoted 4 lines

Ultimately I guess I'll leave it upto audit subsystem what it wants to
have in its struct fanotify_response_info_audit_rule because for fanotify subsystem, it is just an opaque blob it is passing.

In that case, let's stick with leveraging the type/len fields in the fanotify_response_info_header struct, that should give us all the flexibility we need.

Richard and Steve, it sounds like Steve is already aware of additional
information that he wants to send via the fanotify_response_info_audit_rule struct, please include that in the next revision of this patchset. I don't want to get this merged and then soon after have to hack in additional info.

Quoted 4 lines

Quoted 1 line

Steve, please define the type and name of this additional field.

Maybe extra_data, app_data, or extra_info. Something generic that can be reused by any application. Default to 0 if not present.

Quoted 6 lines

I think the point is being missed ... The idea is to not speculate on additional fields, as discussed we have ways to handle that, the issue was that Steve implied that he already had ideas for "things" he wanted to add. If there are "things" that need to be added, let's do that now, however if there is just speculation that maybe someday we might need to add something else we can leave that until later.

This is not speculation. I know what I want to put there. I know you want to pin it down to exactly what it is. However, when this started a couple years back, one of the concerns was that we're building something specific to 1 user of fanotify. And that it would be better for all future users to have a generic facility that everyone could use if they wanted to. That's why I'm suggesting something generic, its so this is not special purpose that doesn't fit any other use case.

Quoted 3 lines · Steve Grubb

Quoted 2 lines · EDT Paul Moore

Quoted 1 line · Steve Grubb

On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs

Quoted 34 lines

Quoted 27 lines

Quoted 21 lines

Quoted 21 lines

Quoted 20 lines

Quoted 6 lines

Ultimately I guess I'll leave it upto audit subsystem what it wants
to
have in its struct fanotify_response_info_audit_rule because for
fanotify subsystem, it is just an opaque blob it is passing.

In that case, let's stick with leveraging the type/len fields in the
fanotify_response_info_header struct, that should give us all the flexibility we need.

Richard and Steve, it sounds like Steve is already aware of additional
information that he wants to send via the fanotify_response_info_audit_rule struct, please include that in the
next revision of this patchset. I don't want to get this merged and
then soon after have to hack in additional info.

Quoted 5 lines

Quoted 1 line

Steve, please define the type and name of this additional field.

Maybe extra_data, app_data, or extra_info. Something generic that can be
reused by any application. Default to 0 if not present.

Quoted 6 lines

I think the point is being missed ... The idea is to not speculate on additional fields, as discussed we have ways to handle that, the issue was that Steve implied that he already had ideas for "things" he wanted to add. If there are "things" that need to be added, let's do that now, however if there is just speculation that maybe someday we might need to add something else we can leave that until later.

Quoted 7 lines

This is not speculation. I know what I want to put there. I know you want to pin it down to exactly what it is. However, when this started a couple years back, one of the concerns was that we're building something specific to 1 user of fanotify. And that it would be better for all future users to have a generic facility that everyone could use if they wanted to. That's why I'm suggesting something generic, its so this is not special purpose that doesn't fit any other use case.

Well, we are talking specifically about fanotify in this thread and dealing with data structures that are specific to fanotify. I can understand wanting to future proof things, but based on what we've seen in this thread I think we are all set in this regard.

You mention that you know what you want to put in the struct, why not share the details with all of us so we are all on the same page and can have a proper discussion.

Quoted 4 lines · EDT Paul Moore

Quoted 3 lines · Steve Grubb

Quoted 2 lines · EDT Paul Moore

Quoted 1 line · Steve Grubb

On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs

Quoted 42 lines · wrote:

Quoted 34 lines

Quoted 27 lines

Quoted 21 lines

Quoted 21 lines

Quoted 20 lines

Quoted 6 lines

Ultimately I guess I'll leave it upto audit subsystem what it wants
to
have in its struct fanotify_response_info_audit_rule because for
fanotify subsystem, it is just an opaque blob it is passing.

In that case, let's stick with leveraging the type/len fields in the
fanotify_response_info_header struct, that should give us all the flexibility we need.

Richard and Steve, it sounds like Steve is already aware of additional
information that he wants to send via the fanotify_response_info_audit_rule struct, please include that in the
next revision of this patchset. I don't want to get this merged and
then soon after have to hack in additional info.

Quoted 5 lines

Quoted 1 line

Steve, please define the type and name of this additional field.

Maybe extra_data, app_data, or extra_info. Something generic that can be
reused by any application. Default to 0 if not present.

Quoted 6 lines

I think the point is being missed ... The idea is to not speculate on additional fields, as discussed we have ways to handle that, the issue was that Steve implied that he already had ideas for "things" he wanted to add. If there are "things" that need to be added, let's do that now, however if there is just speculation that maybe someday we might need to add something else we can leave that until later.

Quoted 7 lines

This is not speculation. I know what I want to put there. I know you want to pin it down to exactly what it is. However, when this started a couple years back, one of the concerns was that we're building something specific to 1 user of fanotify. And that it would be better for all future users to have a generic facility that everyone could use if they wanted to. That's why I'm suggesting something generic, its so this is not special purpose that doesn't fit any other use case.

Quoted 4 lines

Well, we are talking specifically about fanotify in this thread and dealing with data structures that are specific to fanotify. I can understand wanting to future proof things, but based on what we've seen in this thread I think we are all set in this regard.

I'm trying to abide by what was suggested by the fs-devel folks. I can live with it. But if you want to make something non-generic for all users of fanotify, call the new field "trusted". This would decern when a decision was made because the file was untrusted or access denied for another reason.

Quoted 3 lines

You mention that you know what you want to put in the struct, why not share the details with all of us so we are all on the same page and can have a proper discussion.

Because I want to abide by the original agreement and not impose opinionated requirements that serve no one else. I'd rather have something anyone can use. I want to play nice.

-Steve

Quoted 53 lines · Steve Grubb

Quoted 4 lines · EDT Paul Moore

Quoted 3 lines · Steve Grubb

Quoted 2 lines · EDT Paul Moore

Quoted 1 line · Steve Grubb

On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs

Quoted 42 lines · wrote:

Quoted 34 lines

Quoted 27 lines

Quoted 21 lines

Quoted 21 lines

Quoted 20 lines

Quoted 6 lines

Ultimately I guess I'll leave it upto audit subsystem what it wants
to
have in its struct fanotify_response_info_audit_rule because for
fanotify subsystem, it is just an opaque blob it is passing.

In that case, let's stick with leveraging the type/len fields in the
fanotify_response_info_header struct, that should give us all the flexibility we need.

Richard and Steve, it sounds like Steve is already aware of additional
information that he wants to send via the fanotify_response_info_audit_rule struct, please include that in the
next revision of this patchset. I don't want to get this merged and
then soon after have to hack in additional info.

Quoted 5 lines

Quoted 1 line

Steve, please define the type and name of this additional field.

Maybe extra_data, app_data, or extra_info. Something generic that can be
reused by any application. Default to 0 if not present.

Quoted 6 lines

I think the point is being missed ... The idea is to not speculate on additional fields, as discussed we have ways to handle that, the issue was that Steve implied that he already had ideas for "things" he wanted to add. If there are "things" that need to be added, let's do that now, however if there is just speculation that maybe someday we might need to add something else we can leave that until later.

Quoted 7 lines

This is not speculation. I know what I want to put there. I know you want to pin it down to exactly what it is. However, when this started a couple years back, one of the concerns was that we're building something specific to 1 user of fanotify. And that it would be better for all future users to have a generic facility that everyone could use if they wanted to. That's why I'm suggesting something generic, its so this is not special purpose that doesn't fit any other use case.

Quoted 4 lines

Well, we are talking specifically about fanotify in this thread and dealing with data structures that are specific to fanotify. I can understand wanting to future proof things, but based on what we've seen in this thread I think we are all set in this regard.

Quoted 4 lines

I'm trying to abide by what was suggested by the fs-devel folks. I can live with it. But if you want to make something non-generic for all users of fanotify, call the new field "trusted". This would decern when a decision was made because the file was untrusted or access denied for another reason.

So, "u32 trusted;" ? How would you like that formatted? "fan_trust={0|1}"

Quoted 4 lines

Quoted 3 lines

You mention that you know what you want to put in the struct, why not share the details with all of us so we are all on the same page and can have a proper discussion.

Quoted 3 lines

Because I want to abide by the original agreement and not impose opinionated requirements that serve no one else. I'd rather have something anyone can use. I want to play nice.

If someone else wants to use something, why not give them a type of their own other than FAN_RESPONSE_INFO_AUDIT_RULE that they can shape however they like?

Quoted 5 lines

I'm trying to abide by what was suggested by the fs-devel folks. I can live with it. But if you want to make something non-generic for all users of fanotify, call the new field "trusted". This would decern when a decision was made because the file was untrusted or access denied for another reason.

So, "u32 trusted;" ? How would you like that formatted? "fan_trust={0|1}"

Quoted 7 lines

Quoted 3 lines

You mention that you know what you want to put in the struct, why not share the details with all of us so we are all on the same page and can have a proper discussion.

Because I want to abide by the original agreement and not impose opinionated requirements that serve no one else. I'd rather have something anyone can use. I want to play nice.

If someone else wants to use something, why not give them a type of their own other than FAN_RESPONSE_INFO_AUDIT_RULE that they can shape however they like?

Quoted 8 lines · EDT Richard Guy Briggs

Quoted 5 lines

I'm trying to abide by what was suggested by the fs-devel folks. I can live with it. But if you want to make something non-generic for all users of fanotify, call the new field "trusted". This would decern when a decision was made because the file was untrusted or access denied for another reason.

So, "u32 trusted;" ? How would you like that formatted? "fan_trust={0|1}"

So how does this play out if there is another user? Do they want a num= and trust= if not, then the AUDIT_FANOTIFY record will have multiple formats which is not good. I'd rather suggest something generic that can be interpreted based on who's attached to fanotify. IOW we have a fan_type=0 and then followed by info0= info1= the interpretation of those solely depend on fan_type. If the fan_type does not need both, then any interpretation skips what it doesn't need. If fan_type=1, then it follows what arg0= and arg1= is for that format. But make this pivot on fan_type and not actual names.

Hello Steve!

Quoted 13 lines · Steve Grubb

Quoted 11 lines · EDT Richard Guy Briggs

Quoted 8 lines

I'm trying to abide by what was suggested by the fs-devel folks. I can
live with it. But if you want to make something non-generic for all users of fanotify, call the new field "trusted". This would decern when
a decision was made because the file was untrusted or access denied for
another reason.

So, "u32 trusted;" ? How would you like that formatted? "fan_trust={0|1}"

Quoted 9 lines

So how does this play out if there is another user? Do they want a num= and trust= if not, then the AUDIT_FANOTIFY record will have multiple formats which is not good. I'd rather suggest something generic that can be interpreted based on who's attached to fanotify. IOW we have a fan_type=0 and then followed by info0= info1= the interpretation of those solely depend on fan_type. If the fan_type does not need both, then any interpretation skips what it doesn't need. If fan_type=1, then it follows what arg0= and arg1= is for that format. But make this pivot on fan_type and not actual names.

So I think there is some misunderstanding so let me maybe spell out in detail how I see things so that we can get on the same page:

It was a requirement from me (and probably Amir) that there is a generic way to attach additional info to a response to fanotify permission event. This is achieved by defining:

struct fanotify_response_info_header {
       __u8 type;
       __u8 pad;
       __u16 len;
};

which is a generic header and kernel can based on 'len' field decide how large the response structure is (to safely copy it from userspace) and based on 'type' field it can decide who should be the recipient of this extra information (or generally what to do with it). So any additional info needs to start with this header.

Then there is:

struct fanotify_response_info_audit_rule {
       struct fanotify_response_info_header hdr;
       __u32 audit_rule;
};

which properly starts with the header and hdr.type is expected to be FAN_RESPONSE_INFO_AUDIT_RULE. What happens after the header with type FAN_RESPONSE_INFO_AUDIT_RULE until length hdr.len is fully within *audit* subsystem's responsibility. Fanotify code will just pass this as an opaque blob to the audit subsystem.

So if you know audit subsystem will also need some other field together with 'audit_rule' now is a good time to add it and it doesn't have to be useful for anybody else besides audit. If someone else will need other information passed along with the response, he will append structure with another header with different 'type' field. In principle, there can be multiple structures appended to fanotify response like

<hdr> <data> <hdr> <data> ...

and fanotify subsystem will just pass them to different receivers based on the type in 'hdr' field.

Also if audit needs to pass even more information along with the respose, we can define a new 'type' for it. But the 'type' space is not infinite so I'd prefer this does not happen too often...

I hope this clears out things a bit.

Quoted 73 lines · EDT Jan Kara

Hello Steve!

Quoted 13 lines · Steve Grubb

Quoted 11 lines · EDT Richard Guy Briggs

Quoted 8 lines

I'm trying to abide by what was suggested by the fs-devel folks. I can
live with it. But if you want to make something non-generic for all users of fanotify, call the new field "trusted". This would decern when
a decision was made because the file was untrusted or access denied for
another reason.

So, "u32 trusted;" ? How would you like that formatted? "fan_trust={0|1}"

Quoted 9 lines

So how does this play out if there is another user? Do they want a num= and trust= if not, then the AUDIT_FANOTIFY record will have multiple formats which is not good. I'd rather suggest something generic that can be interpreted based on who's attached to fanotify. IOW we have a fan_type=0 and then followed by info0= info1= the interpretation of those solely depend on fan_type. If the fan_type does not need both, then any interpretation skips what it doesn't need. If fan_type=1, then it follows what arg0= and arg1= is for that format. But make this pivot on fan_type and not actual names.

So I think there is some misunderstanding so let me maybe spell out in detail how I see things so that we can get on the same page:

It was a requirement from me (and probably Amir) that there is a generic way to attach additional info to a response to fanotify permission event. This is achieved by defining:

struct fanotify_response_info_header {
       __u8 type;
       __u8 pad;
       __u16 len;
};

which is a generic header and kernel can based on 'len' field decide how large the response structure is (to safely copy it from userspace) and based on 'type' field it can decide who should be the recipient of this extra information (or generally what to do with it). So any additional info needs to start with this header.

Then there is:

struct fanotify_response_info_audit_rule {
       struct fanotify_response_info_header hdr;
       __u32 audit_rule;
};

which properly starts with the header and hdr.type is expected to be FAN_RESPONSE_INFO_AUDIT_RULE. What happens after the header with type FAN_RESPONSE_INFO_AUDIT_RULE until length hdr.len is fully within *audit* subsystem's responsibility. Fanotify code will just pass this as an opaque blob to the audit subsystem.

So if you know audit subsystem will also need some other field together with 'audit_rule' now is a good time to add it and it doesn't have to be useful for anybody else besides audit. If someone else will need other information passed along with the response, he will append structure with another header with different 'type' field. In principle, there can be multiple structures appended to fanotify response like

<hdr> <data> <hdr> <data> ...

and fanotify subsystem will just pass them to different receivers based on the type in 'hdr' field.

Also if audit needs to pass even more information along with the respose, we can define a new 'type' for it. But the 'type' space is not infinite so I'd prefer this does not happen too often...

Quoted 1 line

I hope this clears out things a bit.

Yes. Thank you.

Richard, add subj_trust and obj_trust. These can be 0|1|2 for no, yes, unknown.

-Steve

Quoted 2 lines

Richard, add subj_trust and obj_trust. These can be 0|1|2 for no, yes, unknown.

type? bitfield? My gut would say that "0" should be "unset"/"unknown", but that is counterintuitive to the values represented.

Or "trust" with sub-fields "subj" and "obj"?

Quoted 7 lines · EDT Richard Guy Briggs

Quoted 2 lines

Richard, add subj_trust and obj_trust. These can be 0|1|2 for no, yes, unknown.

type? bitfield? My gut would say that "0" should be "unset"/"unknown", but that is counterintuitive to the values represented.

Or "trust" with sub-fields "subj" and "obj"?

No. just make them separate and u32. subj_trust and obj_trust - no sub fields. If we have sub-fields, that probably means bit mapping and that wasn't wanted.

-Steve

Currently the only type of fanotify info that is defined is an audit rule number, but convert it to hex encoding to future-proof the field.

Sample record:
type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0
fan_info=3F

Suggested-by Paul Moore <[email protected]>

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

---
 kernel/auditsc.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

kernel/auditsc.c +0 -0

index f000fec52360..0f747015c577 100644

kernel/auditsc.c +6 -0

+						 response, friar->hdr.type);
+				audit_log_n_hex(ab, (char *)&friar->audit_rule,
+						sizeof(friar->audit_rule));
+				audit_log_end(ab);
+
+			}
 		}

kernel/auditsc.c +0 -0

 		c -= friar->hdr.len;
 		ib += friar->hdr.len;

Hell Richard,

Quoted 6 lines

Currently the only type of fanotify info that is defined is an audit rule number, but convert it to hex encoding to future-proof the field.

Sample record:
type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0
fan_info=3F

I compiled a new kernel and run old user space on this. The above event is exactly what I see in my audit logs. Why the fan_info=3F? I really would have expected 0. What if the actual rule number was 63? I think this will work better to leave everything 0 with old user space.

-Steve

Quoted 48 lines

Suggested-by Paul Moore <[email protected]>

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

---
 kernel/auditsc.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

kernel/auditsc.c +16 -7

index f000fec52360..0f747015c577 100644
@@ -2908,22 +2908,36 @@ void __audit_fanotify(u32 response, size_t len,
 char *buf)
 
 	if (!(len && buf)) {
 		audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
-			  "resp=%u fan_type=0 fan_info=?", response);
+			  "resp=%u fan_type=0 fan_info=3F", response); /* "?" */
 		return;
 	}
 	while (c >= sizeof(struct fanotify_response_info_header)) {
+		struct audit_context *ctx = audit_context();
+		struct audit_buffer *ab;
+
 		friar = (struct fanotify_response_info_audit_rule *)buf;
 		switch (friar->hdr.type) {
 		case FAN_RESPONSE_INFO_AUDIT_RULE:
 			if (friar->hdr.len < sizeof(*friar)) {
-				audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
-					  "resp=%u fan_type=%u fan_info=(incomplete)",
-					  response, friar->hdr.type);
+				ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY);
+				if (ab) {
+					audit_log_format(ab, "resp=%u fan_type=%u fan_info=",
+							 response, friar-
 hdr.type);
+#define INCOMPLETE "(incomplete)"
+					audit_log_n_hex(ab, INCOMPLETE, sizeof(INCOMPLETE));
+					audit_log_end(ab);
+				}
 				return;
 			}
-			audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
-				  "resp=%u fan_type=%u fan_info=%u",
-				  response, friar->hdr.type, friar->audit_rule);
+			ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY);
+			if (ab) {
+				audit_log_format(ab, "resp=%u fan_type=%u fan_info=",
+						 response, friar->hdr.type);

kernel/auditsc.c void __audit_fanotify(u32 response, size_t len, char *buf) · line 2943

Quoted 8 lines

kernel/auditsc.c +5 -0

+				audit_log_n_hex(ab, (char *)&friar->audit_rule,
+						sizeof(friar->audit_rule));
+				audit_log_end(ab);
+
+			}
 		}

kernel/auditsc.c +0 -0

 		c -= friar->hdr.len;
 		ib += friar->hdr.len;

Quoted 5 lines

I compiled a new kernel and run old user space on this. The above event is
exactly what I see in my audit logs. Why the fan_info=3F? I really would have expected 0. What if the actual rule number was 63? I think this will work better to leave everything 0 with old user space.

Well, if it is to be consistently hex encoded, that corresponds to "?"

if it is to be interpreted as a string. Since the fan_type is 0, fan_info would be invalid, so a value of 0 would be entirely reasonable, hex encoded to fan_info=00. It could also be hex encoded to the string "(none)". If you wanted "0" for fan_type=FAN_RESPONSE_INFO_AUDIT_RULE, that would be fan_info=30 if it were interpreted as a string, or arguably 3F for an integer of rule (decimal) 63. Ultimately, fan_type should determine how fan_info's hex encoded value should be interpreted.

But ultimately, the point of this patch is to hex encode the fan_info field value.

Currently the only type of fanotify info that is defined is an audit rule number, but convert it to hex encoding to future-proof the field.

Sample record:
type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F

Suggested-by Paul Moore <[email protected]>

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

---
 kernel/auditsc.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

kernel/auditsc.c +3 -0

                return;
        }
        while (c >= sizeof(struct fanotify_response_info_header)) {
+               struct audit_context *ctx = audit_context();
+               struct audit_buffer *ab;
+
                friar = (struct fanotify_response_info_audit_rule *)buf;
                switch (friar->hdr.type) {
                case FAN_RESPONSE_INFO_AUDIT_RULE: