[PATCH 0/1] fs: inotify: Add full paths option to inotify

[PATCH 0/1] fs: inotify: Add full paths option to inotify

[Oliver Ford]

Adds an option to return the full path in inotify events. Currently, user space has to keep track of watch descriptors and paths, mapping the descriptor returned when reading inotify events to the path. Adding an option to return the full path simplifies user space code.

The patch adds a flag, IN_FULL_PATHS, to the available mask in inotify_add_watch. When set, the full path is returned when events are added to the watch queue and a path is available. For the event IN_MOVE_SELF, a check is performed that the user has access to the new path. This prevents exposing the names of directories if, for example, root moves "/home/dmr/watched" to "/root/top_secret/watched". In that case, the watch is removed and a Permission Denied error is returned. For the IN_DELETE_SELF/IN_IGNORED pair, no path is returned.

Oliver Ford (1):
  fs: inotify: Add full paths option to inotify

 fs/notify/inotify/inotify_fsnotify.c | 55 ++++++++++++++++++++++------
 fs/notify/inotify/inotify_user.c     | 19 +++++++++-
 include/linux/inotify.h              |  2 +-
 include/uapi/linux/inotify.h         |  1 +
 4 files changed, 63 insertions(+), 14 deletions(-)

-- 
2.35.1

[PATCH 1/1] fs: inotify: Add full paths option to inotify

[Oliver Ford]

Adds a flag IN_FULL_PATHS which causes inotify
to return the full path instead of only a filename.

Includes a permissions check on IN_MOVE_SELF to prevent
exposing paths if the user does not have permission to view
the new path.

Signed-off-by: Oliver Ford <ojford@gmail.com>
---
 fs/notify/inotify/inotify_fsnotify.c | 55 ++++++++++++++++++++++------
 fs/notify/inotify/inotify_user.c     | 19 +++++++++-
 include/linux/inotify.h              |  2 +-
 include/uapi/linux/inotify.h         |  1 +
 4 files changed, 63 insertions(+), 14 deletions(-)

diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index 49cfe2ae6d23..6334d1d6d5f5 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -64,12 +64,39 @@ int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask,
 	struct inotify_event_info *event;
 	struct fsnotify_event *fsn_event;
 	struct fsnotify_group *group = inode_mark->group;
+	struct dentry *en = NULL;
 	int ret;
 	int len = 0;
 	int alloc_len = sizeof(struct inotify_event_info);
 	struct mem_cgroup *old_memcg;
-
-	if (name) {
+	char *path_buf, *path_bufp = NULL;
+	bool found_full_path = false;
+
+	if (inode_mark->mask & IN_FULL_PATHS && inode) {
+		mask |= IN_FULL_PATHS;
+		en = d_find_any_alias(inode);
+		if (en)
+			found_full_path = true;
+		else if (dir)
+			en = d_find_any_alias(dir);
+
+		if (en) {
+			path_buf = kmalloc(PATH_MAX, GFP_KERNEL);
+			if (unlikely(!path_buf))
+				goto oom;
+
+			path_bufp = dentry_path_raw(en, path_buf, PATH_MAX);
+			len = strlen(path_bufp);
+			alloc_len += len + 1;
+
+			if (!found_full_path) {
+				*(path_bufp + len) = '/';
+				strcat(path_bufp + len + 1, name->name);
+				len += name->len + 1;
+				alloc_len += name->len + 1;
+			}
+		}
+	} else if (name) {
 		len = name->len;
 		alloc_len += len + 1;
 	}
@@ -89,14 +116,8 @@ int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask,
 	event = kmalloc(alloc_len, GFP_KERNEL_ACCOUNT | __GFP_RETRY_MAYFAIL);
 	set_active_memcg(old_memcg);
 
-	if (unlikely(!event)) {
-		/*
-		 * Treat lost event due to ENOMEM the same way as queue
-		 * overflow to let userspace know event was lost.
-		 */
-		fsnotify_queue_overflow(group);
-		return -ENOMEM;
-	}
+	if (unlikely(!event))
+		goto oom;
 
 	/*
 	 * We now report FS_ISDIR flag with MOVE_SELF and DELETE_SELF events
@@ -113,8 +134,13 @@ int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask,
 	event->wd = i_mark->wd;
 	event->sync_cookie = cookie;
 	event->name_len = len;
-	if (len)
+
+	if (path_bufp) {
+		strcpy(event->name, path_bufp);
+		kfree(path_buf);
+	} else if (len) {
 		strcpy(event->name, name->name);
+	}
 
 	ret = fsnotify_add_event(group, fsn_event, inotify_merge);
 	if (ret) {
@@ -126,6 +152,13 @@ int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask,
 		fsnotify_destroy_mark(inode_mark, group);
 
 	return 0;
+oom:
+	/*
+	 * Treat lost event due to ENOMEM the same way as queue
+	 * overflow to let userspace know event was lost.
+	 */
+	fsnotify_queue_overflow(group);
+	return -ENOMEM;
 }
 
 static void inotify_freeing_mark(struct fsnotify_mark *fsn_mark, struct fsnotify_group *group)
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index ed42a189faa2..2a0ad59250ce 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -57,6 +57,7 @@ struct kmem_cache *inotify_inode_mark_cachep __read_mostly;
 
 static long it_zero = 0;
 static long it_int_max = INT_MAX;
+static struct inotify_inode_mark *inotify_idr_find(struct fsnotify_group *, int);
 
 static struct ctl_table inotify_table[] = {
 	{
@@ -110,7 +111,7 @@ static inline __u32 inotify_arg_to_mask(struct inode *inode, u32 arg)
 		mask |= FS_EVENT_ON_CHILD;
 
 	/* mask off the flags used to open the fd */
-	mask |= (arg & INOTIFY_USER_MASK);
+	mask |= (arg & (INOTIFY_USER_MASK | IN_FULL_PATHS));
 
 	return mask;
 }
@@ -203,6 +204,8 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 {
 	struct inotify_event inotify_event;
 	struct inotify_event_info *event;
+	struct path event_path;
+	struct inotify_inode_mark *i_mark;
 	size_t event_size = sizeof(struct inotify_event);
 	size_t name_len;
 	size_t pad_name_len;
@@ -210,6 +213,18 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	pr_debug("%s: group=%p event=%p\n", __func__, group, fsn_event);
 
 	event = INOTIFY_E(fsn_event);
+	/* ensure caller has access to view the full path */
+	if (event->mask & IN_FULL_PATHS && event->mask & IN_MOVE_SELF &&
+	    kern_path(event->name, 0, &event_path)) {
+		i_mark = inotify_idr_find(group, event->wd);
+		if (likely(i_mark)) {
+			fsnotify_destroy_mark(&i_mark->fsn_mark, group);
+			/* match ref taken by inotify_idr_find */
+			fsnotify_put_mark(&i_mark->fsn_mark);
+		}
+		return -EACCES;
+	}
+
 	name_len = event->name_len;
 	/*
 	 * round up name length so it is a multiple of event_size
@@ -860,7 +875,7 @@ static int __init inotify_user_setup(void)
 	BUILD_BUG_ON(IN_IGNORED != FS_IN_IGNORED);
 	BUILD_BUG_ON(IN_ISDIR != FS_ISDIR);
 
-	BUILD_BUG_ON(HWEIGHT32(ALL_INOTIFY_BITS) != 22);
+	BUILD_BUG_ON(HWEIGHT32(ALL_INOTIFY_BITS) != 23);
 
 	inotify_inode_mark_cachep = KMEM_CACHE(inotify_inode_mark,
 					       SLAB_PANIC|SLAB_ACCOUNT);
diff --git a/include/linux/inotify.h b/include/linux/inotify.h
index 8d20caa1b268..11db0541cff5 100644
--- a/include/linux/inotify.h
+++ b/include/linux/inotify.h
@@ -15,6 +15,6 @@
 			  IN_DELETE_SELF | IN_MOVE_SELF | IN_UNMOUNT | \
 			  IN_Q_OVERFLOW | IN_IGNORED | IN_ONLYDIR | \
 			  IN_DONT_FOLLOW | IN_EXCL_UNLINK | IN_MASK_ADD | \
-			  IN_MASK_CREATE | IN_ISDIR | IN_ONESHOT)
+			  IN_MASK_CREATE | IN_ISDIR | IN_ONESHOT | IN_FULL_PATHS)
 
 #endif	/* _LINUX_INOTIFY_H */
diff --git a/include/uapi/linux/inotify.h b/include/uapi/linux/inotify.h
index 884b4846b630..d95e05aa9bd6 100644
--- a/include/uapi/linux/inotify.h
+++ b/include/uapi/linux/inotify.h
@@ -50,6 +50,7 @@ struct inotify_event {
 #define IN_MOVE			(IN_MOVED_FROM | IN_MOVED_TO) /* moves */
 
 /* special flags */
+#define IN_FULL_PATHS		0x00800000	/* return the absolute path in the name */
 #define IN_ONLYDIR		0x01000000	/* only watch the path if it is a directory */
 #define IN_DONT_FOLLOW		0x02000000	/* don't follow a sym link */
 #define IN_EXCL_UNLINK		0x04000000	/* exclude events on unlinked objects */
-- 
2.35.1

Re: [PATCH 0/1] fs: inotify: Add full paths option to inotify

[Amir Goldstein]

On Tue, Jun 7, 2022 at 1:43 AM Oliver Ford <ojford@gmail.com> wrote:
>
> Adds an option to return the full path in inotify events. Currently, user space has to keep track of watch descriptors and paths, mapping the descriptor returned when reading inotify events to the path. Adding an option to return the full path simplifies user space code.

That is exactly what FAN_REPORT_DFID_NAME fanofiy mode is for.
Please try to use it and see if it fits your needs.

Thanks,
Amir.

Re: [PATCH 1/1] fs: inotify: Add full paths option to inotify

[kernel test robot]

Hi Oliver,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on jack-fs/fsnotify]
[also build test ERROR on linus/master v5.19-rc1 next-20220606]
[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]

url:    https://github.com/intel-lab-lkp/linux/commits/Oliver-Ford/fs-inotify-Add-full-paths-option-to-inotify/20220607-064615
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: hexagon-randconfig-r018-20220607 (https://download.01.org/0day-ci/archive/20220607/202206071212.ER5BjGEI-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project b92436efcb7813fc481b30f2593a4907568d917a)
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/67d0b1ab6f9129e4902f90506f2ab045ddbae43f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Oliver-Ford/fs-inotify-Add-full-paths-option-to-inotify/20220607-064615
        git checkout 67d0b1ab6f9129e4902f90506f2ab045ddbae43f
        # 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=hexagon SHELL=/bin/bash fs/notify/inotify/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

>> fs/notify/inotify/inotify_user.c:219:12: error: call to undeclared function 'inotify_idr_find'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   i_mark = inotify_idr_find(group, event->wd);
                            ^
>> fs/notify/inotify/inotify_user.c:219:10: warning: incompatible integer to pointer conversion assigning to 'struct inotify_inode_mark *' from 'int' [-Wint-conversion]
                   i_mark = inotify_idr_find(group, event->wd);
                          ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> fs/notify/inotify/inotify_user.c:451:35: error: static declaration of 'inotify_idr_find' follows non-static declaration
   static struct inotify_inode_mark *inotify_idr_find(struct fsnotify_group *group,
                                     ^
   fs/notify/inotify/inotify_user.c:219:12: note: previous implicit declaration is here
                   i_mark = inotify_idr_find(group, event->wd);
                            ^
   1 warning and 2 errors generated.


vim +/inotify_idr_find +219 fs/notify/inotify/inotify_user.c

   194	
   195	/*
   196	 * Copy an event to user space, returning how much we copied.
   197	 *
   198	 * We already checked that the event size is smaller than the
   199	 * buffer we had in "get_one_event()" above.
   200	 */
   201	static ssize_t copy_event_to_user(struct fsnotify_group *group,
   202					  struct fsnotify_event *fsn_event,
   203					  char __user *buf)
   204	{
   205		struct inotify_event inotify_event;
   206		struct inotify_event_info *event;
   207		struct path event_path;
   208		struct inotify_inode_mark *i_mark;
   209		size_t event_size = sizeof(struct inotify_event);
   210		size_t name_len;
   211		size_t pad_name_len;
   212	
   213		pr_debug("%s: group=%p event=%p\n", __func__, group, fsn_event);
   214	
   215		event = INOTIFY_E(fsn_event);
   216		/* ensure caller has access to view the full path */
   217		if (event->mask & IN_FULL_PATHS && event->mask & IN_MOVE_SELF &&
   218		    kern_path(event->name, 0, &event_path)) {
 > 219			i_mark = inotify_idr_find(group, event->wd);
   220			if (likely(i_mark)) {
   221				fsnotify_destroy_mark(&i_mark->fsn_mark, group);
   222				/* match ref taken by inotify_idr_find */
   223				fsnotify_put_mark(&i_mark->fsn_mark);
   224			}
   225			return -EACCES;
   226		}
   227	
   228		name_len = event->name_len;
   229		/*
   230		 * round up name length so it is a multiple of event_size
   231		 * plus an extra byte for the terminating '\0'.
   232		 */
   233		pad_name_len = round_event_name_len(fsn_event);
   234		inotify_event.len = pad_name_len;
   235		inotify_event.mask = inotify_mask_to_arg(event->mask);
   236		inotify_event.wd = event->wd;
   237		inotify_event.cookie = event->sync_cookie;
   238	
   239		/* send the main event */
   240		if (copy_to_user(buf, &inotify_event, event_size))
   241			return -EFAULT;
   242	
   243		buf += event_size;
   244	
   245		/*
   246		 * fsnotify only stores the pathname, so here we have to send the pathname
   247		 * and then pad that pathname out to a multiple of sizeof(inotify_event)
   248		 * with zeros.
   249		 */
   250		if (pad_name_len) {
   251			/* copy the path name */
   252			if (copy_to_user(buf, event->name, name_len))
   253				return -EFAULT;
   254			buf += name_len;
   255	
   256			/* fill userspace with 0's */
   257			if (clear_user(buf, pad_name_len - name_len))
   258				return -EFAULT;
   259			event_size += pad_name_len;
   260		}
   261	
   262		return event_size;
   263	}
   264	

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

Re: [PATCH 1/1] fs: inotify: Add full paths option to inotify

[Al Viro]

On Mon, Jun 06, 2022 at 11:42:41PM +0100, Oliver Ford wrote:

> @@ -203,6 +204,8 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>  {
>  	struct inotify_event inotify_event;
>  	struct inotify_event_info *event;
> +	struct path event_path;
> +	struct inotify_inode_mark *i_mark;
>  	size_t event_size = sizeof(struct inotify_event);
>  	size_t name_len;
>  	size_t pad_name_len;
> @@ -210,6 +213,18 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>  	pr_debug("%s: group=%p event=%p\n", __func__, group, fsn_event);
>  
>  	event = INOTIFY_E(fsn_event);
> +	/* ensure caller has access to view the full path */
> +	if (event->mask & IN_FULL_PATHS && event->mask & IN_MOVE_SELF &&
> +	    kern_path(event->name, 0, &event_path)) {
> +		i_mark = inotify_idr_find(group, event->wd);
> +		if (likely(i_mark)) {
> +			fsnotify_destroy_mark(&i_mark->fsn_mark, group);
> +			/* match ref taken by inotify_idr_find */
> +			fsnotify_put_mark(&i_mark->fsn_mark);
> +		}
> +		return -EACCES;
> +	}
> +

What.  The.  Hell?

Could you please explain how is it not a massive dentry and mount leak and
just what is being attempted here, anyway?

Incidentally, who said that pathname will be still resolving to whatever
it used to resolve to back when the operation had happened?  Or that
it would make any sense for the read(2) caller, while we are at it...

NAKed-by: Al Viro <viro@zeniv.linux.org.uk>