[PATCH] fsnotify: add generic perm check for unlink/rmdir

[PATCH] fsnotify: add generic perm check for unlink/rmdir

[Guowei Du]

From: duguowei <duguowei@xiaomi.com>

For now, there have been open/access/open_exec perms for file operation,
so we add new perms check with unlink/rmdir syscall. if one app deletes
any file/dir within pubic area, fsnotify can sends fsnotify_event to
listener to deny that, even if the app have right dac/mac permissions.

Signed-off-by: duguowei <duguowei@xiaomi.com>
---
 fs/notify/fsnotify.c             |  2 +-
 include/linux/fs.h               |  2 ++
 include/linux/fsnotify.h         | 16 ++++++++++++++++
 include/linux/fsnotify_backend.h |  6 +++++-
 security/security.c              | 12 ++++++++++--
 security/selinux/hooks.c         |  4 ++++
 6 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 70a8516b78bc..9c03a5f84be0 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -581,7 +581,7 @@ static __init int fsnotify_init(void)
 {
 	int ret;
 
-	BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 25);
+	BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 27);
 
 	ret = init_srcu_struct(&fsnotify_mark_srcu);
 	if (ret)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bbde95387a23..9c661584db7d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -100,6 +100,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 #define MAY_CHDIR		0x00000040
 /* called from RCU mode, don't block */
 #define MAY_NOT_BLOCK		0x00000080
+#define MAY_UNLINK		0x00000100
+#define MAY_RMDIR		0x00000200
 
 /*
  * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must correspond
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index bb8467cd11ae..68f5d4aaf1ae 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -80,6 +80,22 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
 	return fsnotify(mask, data, data_type, NULL, NULL, inode, 0);
 }
 
+static inline int fsnotify_path_perm(struct path *path, struct dentry *dentry, __u32 mask)
+{
+	__u32 fsnotify_mask = 0;
+
+	if (!(mask & (MAY_UNLINK | MAY_RMDIR)))
+		return 0;
+
+	if (mask & MAY_UNLINK)
+		fsnotify_mask |= FS_UNLINK_PERM;
+
+	if (mask & MAY_RMDIR)
+		fsnotify_mask |= FS_RMDIR_PERM;
+
+	return fsnotify_parent(dentry, fsnotify_mask, path, FSNOTIFY_EVENT_PATH);
+}
+
 /*
  * Simple wrappers to consolidate calls to fsnotify_parent() when an event
  * is on a file/dentry.
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 0805b74cae44..0e2e240e8234 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -54,6 +54,8 @@
 #define FS_OPEN_PERM		0x00010000	/* open event in an permission hook */
 #define FS_ACCESS_PERM		0x00020000	/* access event in a permissions hook */
 #define FS_OPEN_EXEC_PERM	0x00040000	/* open/exec event in a permission hook */
+#define FS_UNLINK_PERM		0x00080000	/* unlink event in a permission hook */
+#define FS_RMDIR_PERM		0x00100000	/* rmdir event in a permission hook */
 
 #define FS_EXCL_UNLINK		0x04000000	/* do not send events if object is unlinked */
 /*
@@ -79,7 +81,9 @@
 #define ALL_FSNOTIFY_DIRENT_EVENTS (FS_CREATE | FS_DELETE | FS_MOVE | FS_RENAME)
 
 #define ALL_FSNOTIFY_PERM_EVENTS (FS_OPEN_PERM | FS_ACCESS_PERM | \
-				  FS_OPEN_EXEC_PERM)
+				  FS_OPEN_EXEC_PERM | \
+				  FS_UNLINK_PERM | \
+				  FS_RMDIR_PERM)
 
 /*
  * This is a list of all events that may get sent to a parent that is watching
diff --git a/security/security.c b/security/security.c
index b7cf5cbfdc67..8efc00ec02ed 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1160,16 +1160,24 @@ EXPORT_SYMBOL(security_path_mkdir);
 
 int security_path_rmdir(const struct path *dir, struct dentry *dentry)
 {
+	int ret;
 	if (unlikely(IS_PRIVATE(d_backing_inode(dir->dentry))))
 		return 0;
-	return call_int_hook(path_rmdir, 0, dir, dentry);
+	ret = call_int_hook(path_rmdir, 0, dir, dentry);
+	if (ret)
+		return ret;
+	return fsnotify_path_perm(dir, dentry, MAY_RMDIR);
 }
 
 int security_path_unlink(const struct path *dir, struct dentry *dentry)
 {
+	int ret;
 	if (unlikely(IS_PRIVATE(d_backing_inode(dir->dentry))))
 		return 0;
-	return call_int_hook(path_unlink, 0, dir, dentry);
+	ret = call_int_hook(path_unlink, 0, dir, dentry);
+	if (ret)
+		return ret;
+	return fsnotify_path_perm(dir, dentry, MAY_UNLINK);
 }
 EXPORT_SYMBOL(security_path_unlink);
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e9e959343de9..f0780f0eb903 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1801,8 +1801,12 @@ static int may_create(struct inode *dir,
 }
 
 #define MAY_LINK	0
+#ifndef MAY_UNLINK
 #define MAY_UNLINK	1
+#endif
+#ifndef MAY_RMDIR
 #define MAY_RMDIR	2
+#endif
 
 /* Check whether a task can link, unlink, or rmdir a file/directory. */
 static int may_link(struct inode *dir,
-- 
2.17.1

Re: [PATCH] fsnotify: add generic perm check for unlink/rmdir

[Jan Kara]

On Wed 04-05-22 02:37:50, Guowei Du wrote:
> From: duguowei <duguowei@xiaomi.com>
> 
> For now, there have been open/access/open_exec perms for file operation,
> so we add new perms check with unlink/rmdir syscall. if one app deletes
> any file/dir within pubic area, fsnotify can sends fsnotify_event to
> listener to deny that, even if the app have right dac/mac permissions.
> 
> Signed-off-by: duguowei <duguowei@xiaomi.com>

Before we go into technical details of implementation can you tell me more
details about the usecase? Why do you need to check specifically for unlink
/ delete?

Also on the design side of things: Do you realize these permission events
will not be usable together with other permission events like
FAN_OPEN_PERM? Because these require notification group returning file
descriptors while your events will return file handles... I guess we should
somehow fix that.


								Honza
> ---
>  fs/notify/fsnotify.c             |  2 +-
>  include/linux/fs.h               |  2 ++
>  include/linux/fsnotify.h         | 16 ++++++++++++++++
>  include/linux/fsnotify_backend.h |  6 +++++-
>  security/security.c              | 12 ++++++++++--
>  security/selinux/hooks.c         |  4 ++++
>  6 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 70a8516b78bc..9c03a5f84be0 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -581,7 +581,7 @@ static __init int fsnotify_init(void)
>  {
>  	int ret;
>  
> -	BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 25);
> +	BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 27);
>  
>  	ret = init_srcu_struct(&fsnotify_mark_srcu);
>  	if (ret)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index bbde95387a23..9c661584db7d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -100,6 +100,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
>  #define MAY_CHDIR		0x00000040
>  /* called from RCU mode, don't block */
>  #define MAY_NOT_BLOCK		0x00000080
> +#define MAY_UNLINK		0x00000100
> +#define MAY_RMDIR		0x00000200
>  
>  /*
>   * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must correspond
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index bb8467cd11ae..68f5d4aaf1ae 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -80,6 +80,22 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
>  	return fsnotify(mask, data, data_type, NULL, NULL, inode, 0);
>  }
>  
> +static inline int fsnotify_path_perm(struct path *path, struct dentry *dentry, __u32 mask)
> +{
> +	__u32 fsnotify_mask = 0;
> +
> +	if (!(mask & (MAY_UNLINK | MAY_RMDIR)))
> +		return 0;
> +
> +	if (mask & MAY_UNLINK)
> +		fsnotify_mask |= FS_UNLINK_PERM;
> +
> +	if (mask & MAY_RMDIR)
> +		fsnotify_mask |= FS_RMDIR_PERM;
> +
> +	return fsnotify_parent(dentry, fsnotify_mask, path, FSNOTIFY_EVENT_PATH);
> +}
> +
>  /*
>   * Simple wrappers to consolidate calls to fsnotify_parent() when an event
>   * is on a file/dentry.
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 0805b74cae44..0e2e240e8234 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -54,6 +54,8 @@
>  #define FS_OPEN_PERM		0x00010000	/* open event in an permission hook */
>  #define FS_ACCESS_PERM		0x00020000	/* access event in a permissions hook */
>  #define FS_OPEN_EXEC_PERM	0x00040000	/* open/exec event in a permission hook */
> +#define FS_UNLINK_PERM		0x00080000	/* unlink event in a permission hook */
> +#define FS_RMDIR_PERM		0x00100000	/* rmdir event in a permission hook */
>  
>  #define FS_EXCL_UNLINK		0x04000000	/* do not send events if object is unlinked */
>  /*
> @@ -79,7 +81,9 @@
>  #define ALL_FSNOTIFY_DIRENT_EVENTS (FS_CREATE | FS_DELETE | FS_MOVE | FS_RENAME)
>  
>  #define ALL_FSNOTIFY_PERM_EVENTS (FS_OPEN_PERM | FS_ACCESS_PERM | \
> -				  FS_OPEN_EXEC_PERM)
> +				  FS_OPEN_EXEC_PERM | \
> +				  FS_UNLINK_PERM | \
> +				  FS_RMDIR_PERM)
>  
>  /*
>   * This is a list of all events that may get sent to a parent that is watching
> diff --git a/security/security.c b/security/security.c
> index b7cf5cbfdc67..8efc00ec02ed 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1160,16 +1160,24 @@ EXPORT_SYMBOL(security_path_mkdir);
>  
>  int security_path_rmdir(const struct path *dir, struct dentry *dentry)
>  {
> +	int ret;
>  	if (unlikely(IS_PRIVATE(d_backing_inode(dir->dentry))))
>  		return 0;
> -	return call_int_hook(path_rmdir, 0, dir, dentry);
> +	ret = call_int_hook(path_rmdir, 0, dir, dentry);
> +	if (ret)
> +		return ret;
> +	return fsnotify_path_perm(dir, dentry, MAY_RMDIR);
>  }
>  
>  int security_path_unlink(const struct path *dir, struct dentry *dentry)
>  {
> +	int ret;
>  	if (unlikely(IS_PRIVATE(d_backing_inode(dir->dentry))))
>  		return 0;
> -	return call_int_hook(path_unlink, 0, dir, dentry);
> +	ret = call_int_hook(path_unlink, 0, dir, dentry);
> +	if (ret)
> +		return ret;
> +	return fsnotify_path_perm(dir, dentry, MAY_UNLINK);
>  }
>  EXPORT_SYMBOL(security_path_unlink);
>  
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e9e959343de9..f0780f0eb903 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1801,8 +1801,12 @@ static int may_create(struct inode *dir,
>  }
>  
>  #define MAY_LINK	0
> +#ifndef MAY_UNLINK
>  #define MAY_UNLINK	1
> +#endif
> +#ifndef MAY_RMDIR
>  #define MAY_RMDIR	2
> +#endif
>  
>  /* Check whether a task can link, unlink, or rmdir a file/directory. */
>  static int may_link(struct inode *dir,
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fsnotify: add generic perm check for unlink/rmdir

[kernel test robot]

Hi Guowei,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pcmoore-selinux/next]
[also build test ERROR on linus/master v5.18-rc5]
[cannot apply to jack-fs/fsnotify next-20220503]
[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/Guowei-Du/fsnotify-add-generic-perm-check-for-unlink-rmdir/20220504-024310
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next
config: hexagon-randconfig-r041-20220501 (https://download.01.org/0day-ci/archive/20220504/202205040959.SAV6vlzH-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 363b3a645a1e30011cc8da624f13dac5fd915628)
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/6f635019bbd2ab22a64e03164c8812a46531966e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Guowei-Du/fsnotify-add-generic-perm-check-for-unlink-rmdir/20220504-024310
        git checkout 6f635019bbd2ab22a64e03164c8812a46531966e
        # 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

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

All errors (new ones prefixed by >>):

>> security/security.c:1169:28: error: passing 'const struct path *' to parameter of type 'struct path *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
           return fsnotify_path_perm(dir, dentry, MAY_RMDIR);
                                     ^~~
   include/linux/fsnotify.h:83:51: note: passing argument to parameter 'path' here
   static inline int fsnotify_path_perm(struct path *path, struct dentry *dentry, __u32 mask)
                                                     ^
   security/security.c:1180:28: error: passing 'const struct path *' to parameter of type 'struct path *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
           return fsnotify_path_perm(dir, dentry, MAY_UNLINK);
                                     ^~~
   include/linux/fsnotify.h:83:51: note: passing argument to parameter 'path' here
   static inline int fsnotify_path_perm(struct path *path, struct dentry *dentry, __u32 mask)
                                                     ^
   2 errors generated.


vim +1169 security/security.c

  1160	
  1161	int security_path_rmdir(const struct path *dir, struct dentry *dentry)
  1162	{
  1163		int ret;
  1164		if (unlikely(IS_PRIVATE(d_backing_inode(dir->dentry))))
  1165			return 0;
  1166		ret = call_int_hook(path_rmdir, 0, dir, dentry);
  1167		if (ret)
  1168			return ret;
> 1169		return fsnotify_path_perm(dir, dentry, MAY_RMDIR);
  1170	}
  1171	

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

Re: [PATCH] fsnotify: add generic perm check for unlink/rmdir

[kernel test robot]

Hi Guowei,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pcmoore-selinux/next]
[also build test WARNING on linus/master jmorris-security/next-testing v5.18-rc5]
[cannot apply to jack-fs/fsnotify next-20220503]
[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/Guowei-Du/fsnotify-add-generic-perm-check-for-unlink-rmdir/20220504-024310
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next
config: h8300-randconfig-s032-20220501 (https://download.01.org/0day-ci/archive/20220504/202205041421.bHwZBEFK-lkp@intel.com/config)
compiler: h8300-linux-gcc (GCC) 11.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/intel-lab-lkp/linux/commit/6f635019bbd2ab22a64e03164c8812a46531966e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Guowei-Du/fsnotify-add-generic-perm-check-for-unlink-rmdir/20220504-024310
        git checkout 6f635019bbd2ab22a64e03164c8812a46531966e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=h8300 SHELL=/bin/bash

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


sparse warnings: (new ones prefixed by >>)
   security/security.c:358:25: sparse: sparse: cast removes address space '__rcu' of expression
>> security/security.c:1169:35: sparse: sparse: incorrect type in argument 1 (different modifiers) @@     expected struct path *path @@     got struct path const *dir @@
   security/security.c:1169:35: sparse:     expected struct path *path
   security/security.c:1169:35: sparse:     got struct path const *dir
   security/security.c:1180:35: sparse: sparse: incorrect type in argument 1 (different modifiers) @@     expected struct path *path @@     got struct path const *dir @@
   security/security.c:1180:35: sparse:     expected struct path *path
   security/security.c:1180:35: sparse:     got struct path const *dir

vim +1169 security/security.c

  1160	
  1161	int security_path_rmdir(const struct path *dir, struct dentry *dentry)
  1162	{
  1163		int ret;
  1164		if (unlikely(IS_PRIVATE(d_backing_inode(dir->dentry))))
  1165			return 0;
  1166		ret = call_int_hook(path_rmdir, 0, dir, dentry);
  1167		if (ret)
  1168			return ret;
> 1169		return fsnotify_path_perm(dir, dentry, MAY_RMDIR);
  1170	}
  1171	

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

Re: [PATCH] fsnotify: add generic perm check for unlink/rmdir

[kernel test robot]

Hi Guowei,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pcmoore-selinux/next]
[also build test WARNING on linus/master jmorris-security/next-testing v5.18-rc5]
[cannot apply to jack-fs/fsnotify next-20220503]
[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/Guowei-Du/fsnotify-add-generic-perm-check-for-unlink-rmdir/20220504-024310
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next
config: openrisc-buildonly-randconfig-r003-20220501 (https://download.01.org/0day-ci/archive/20220504/202205042136.nn1xy0Ae-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 11.3.0
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/6f635019bbd2ab22a64e03164c8812a46531966e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Guowei-Du/fsnotify-add-generic-perm-check-for-unlink-rmdir/20220504-024310
        git checkout 6f635019bbd2ab22a64e03164c8812a46531966e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=openrisc SHELL=/bin/bash

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

All warnings (new ones prefixed by >>):

   security/security.c: In function 'security_path_rmdir':
>> security/security.c:1169:35: warning: passing argument 1 of 'fsnotify_path_perm' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    1169 |         return fsnotify_path_perm(dir, dentry, MAY_RMDIR);
         |                                   ^~~
   In file included from security/security.c:24:
   include/linux/fsnotify.h:83:51: note: expected 'struct path *' but argument is of type 'const struct path *'
      83 | static inline int fsnotify_path_perm(struct path *path, struct dentry *dentry, __u32 mask)
         |                                      ~~~~~~~~~~~~~^~~~
   security/security.c: In function 'security_path_unlink':
   security/security.c:1180:35: warning: passing argument 1 of 'fsnotify_path_perm' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    1180 |         return fsnotify_path_perm(dir, dentry, MAY_UNLINK);
         |                                   ^~~
   In file included from security/security.c:24:
   include/linux/fsnotify.h:83:51: note: expected 'struct path *' but argument is of type 'const struct path *'
      83 | static inline int fsnotify_path_perm(struct path *path, struct dentry *dentry, __u32 mask)
         |                                      ~~~~~~~~~~~~~^~~~


vim +1169 security/security.c

  1160	
  1161	int security_path_rmdir(const struct path *dir, struct dentry *dentry)
  1162	{
  1163		int ret;
  1164		if (unlikely(IS_PRIVATE(d_backing_inode(dir->dentry))))
  1165			return 0;
  1166		ret = call_int_hook(path_rmdir, 0, dir, dentry);
  1167		if (ret)
  1168			return ret;
> 1169		return fsnotify_path_perm(dir, dentry, MAY_RMDIR);
  1170	}
  1171	

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

Re: [PATCH] fsnotify: add generic perm check for unlink/rmdir

[Amir Goldstein]

On Tue, May 3, 2022 at 10:49 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 04-05-22 02:37:50, Guowei Du wrote:
> > From: duguowei <duguowei@xiaomi.com>
> >
> > For now, there have been open/access/open_exec perms for file operation,
> > so we add new perms check with unlink/rmdir syscall. if one app deletes
> > any file/dir within pubic area, fsnotify can sends fsnotify_event to
> > listener to deny that, even if the app have right dac/mac permissions.
> >
> > Signed-off-by: duguowei <duguowei@xiaomi.com>
>
> Before we go into technical details of implementation can you tell me more
> details about the usecase? Why do you need to check specifically for unlink
> / delete?
>
> Also on the design side of things: Do you realize these permission events
> will not be usable together with other permission events like
> FAN_OPEN_PERM? Because these require notification group returning file
> descriptors while your events will return file handles... I guess we should
> somehow fix that.
>

IMO, regardless of file descriptions vs. file handles, blocking events have
no business with async events in the same group at all.
What is the use case for that?
Sure, we have the legacy permission event, but if we do add new blocking
events to UAPI, IMO they should be added to a group that was initialized with a
different class to indicate "blocking events only".

And if we do that, we will not need to pollute the event mask namespace
for every permission event.
When users request to get FAN_UNLINK/FAN_RMDIR events in a
FAN_CLASS_PERMISSION group, internally, that only captures
events reported from fsnotify_perm()/fsnotify_path_perm().

FYI, I do intend to try and upload "pre-modify events" [1].
I had no intention to expose those in fanotify and my implementation
does not have the granularity of UNLINK/RMDIR, but we do need
to think about not duplicating too much code with those overlapping
features.

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/fsnotify_pre_modify

Re: [PATCH] fsnotify: add generic perm check for unlink/rmdir

[Jan Kara]

On Wed 04-05-22 17:12:16, Amir Goldstein wrote:
> On Tue, May 3, 2022 at 10:49 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 04-05-22 02:37:50, Guowei Du wrote:
> > > From: duguowei <duguowei@xiaomi.com>
> > >
> > > For now, there have been open/access/open_exec perms for file operation,
> > > so we add new perms check with unlink/rmdir syscall. if one app deletes
> > > any file/dir within pubic area, fsnotify can sends fsnotify_event to
> > > listener to deny that, even if the app have right dac/mac permissions.
> > >
> > > Signed-off-by: duguowei <duguowei@xiaomi.com>
> >
> > Before we go into technical details of implementation can you tell me more
> > details about the usecase? Why do you need to check specifically for unlink
> > / delete?
> >
> > Also on the design side of things: Do you realize these permission events
> > will not be usable together with other permission events like
> > FAN_OPEN_PERM? Because these require notification group returning file
> > descriptors while your events will return file handles... I guess we should
> > somehow fix that.
> >
> 
> IMO, regardless of file descriptions vs. file handles, blocking events have
> no business with async events in the same group at all.
> What is the use case for that?
> Sure, we have the legacy permission event, but if we do add new blocking
> events to UAPI, IMO they should be added to a group that was initialized with a
> different class to indicate "blocking events only".
> 
> And if we do that, we will not need to pollute the event mask namespace
> for every permission event.

That's an interesting idea. I agree mixing of permission and normal events
is not very useful and separating event mask for permission and other
events looks like a compelling reason to really forbid that :). It's a pity
nobody had this idea when proposing fanotify permission events.

> When users request to get FAN_UNLINK/FAN_RMDIR events in a
> FAN_CLASS_PERMISSION group, internally, that only captures
> events reported from fsnotify_perm()/fsnotify_path_perm().
> 
> FYI, I do intend to try and upload "pre-modify events" [1].
> I had no intention to expose those in fanotify and my implementation
> does not have the granularity of UNLINK/RMDIR, but we do need
> to think about not duplicating too much code with those overlapping
> features.

Definitely.

								Honza

> [1] https://github.com/amir73il/linux/commits/fsnotify_pre_modify
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fsnotify: add generic perm check for unlink/rmdir

[Jan Kara]

Hello!

On Wed 04-05-22 11:42:23, guowei du wrote:
>           for the first issue,one usecase is ,for the shared storage with
> android device,shared storage is public to all apps which gained whole
> storage rwx permission,
>           and computer could also read/write the storage by usb cable
> connected.
>          so ,we need to protect some resources such as photoes or videos or
> some secure documents in the shared storage.
>          in other words,we want to subdivide permissions of that area  for
> open/read/unlink and so on.

I see but I thought that MTP protocol was there exactly so that the phone
can control the access from computer to the shared storage. So it is
probably not the case that you'd need this fanotify feature to control MTP
client access but you want to say block image removal while the file is
being transfered over MTP? Do I get this right?

>          for the second issue. every FANOTIFY_EVENT_TYPE_PATH event will
> 'dentry_open' a new file with FMODE_NONOTIFY,then bind to a new unused fd,
> so could tell me the reason?

Yes, this is just how fanotify was designed. And it was designed in this
way because it was created for use by antivirus scanners which wanted to
read the file contents and based on that decide whether the file could be
accessed or not.

>         and next step ,i will go on to fix the related issue such as
> fanotify module.

I have realized that you do propagate struct path to fsnotify with your new
RMDIR_PERM and UNLINK_PERM events (unlike standard DELETE fsnotify events)
so things should work in the same way as say for OPEN_PERM events.

								Honza

> On Wed, May 4, 2022 at 3:49 AM Jan Kara <jack@suse.cz> wrote:
> 
> > On Wed 04-05-22 02:37:50, Guowei Du wrote:
> > > From: duguowei <duguowei@xiaomi.com>
> > >
> > > For now, there have been open/access/open_exec perms for file operation,
> > > so we add new perms check with unlink/rmdir syscall. if one app deletes
> > > any file/dir within pubic area, fsnotify can sends fsnotify_event to
> > > listener to deny that, even if the app have right dac/mac permissions.
> > >
> > > Signed-off-by: duguowei <duguowei@xiaomi.com>
> >
> > Before we go into technical details of implementation can you tell me more
> > details about the usecase? Why do you need to check specifically for unlink
> > / delete?
> >
> > Also on the design side of things: Do you realize these permission events
> > will not be usable together with other permission events like
> > FAN_OPEN_PERM? Because these require notification group returning file
> > descriptors while your events will return file handles... I guess we should
> > somehow fix that.
> >
> >
> >                                                                 Honza
> > > ---
> > >  fs/notify/fsnotify.c             |  2 +-
> > >  include/linux/fs.h               |  2 ++
> > >  include/linux/fsnotify.h         | 16 ++++++++++++++++
> > >  include/linux/fsnotify_backend.h |  6 +++++-
> > >  security/security.c              | 12 ++++++++++--
> > >  security/selinux/hooks.c         |  4 ++++
> > >  6 files changed, 38 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> > > index 70a8516b78bc..9c03a5f84be0 100644
> > > --- a/fs/notify/fsnotify.c
> > > +++ b/fs/notify/fsnotify.c
> > > @@ -581,7 +581,7 @@ static __init int fsnotify_init(void)
> > >  {
> > >       int ret;
> > >
> > > -     BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 25);
> > > +     BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 27);
> > >
> > >       ret = init_srcu_struct(&fsnotify_mark_srcu);
> > >       if (ret)
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index bbde95387a23..9c661584db7d 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -100,6 +100,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb,
> > loff_t offset,
> > >  #define MAY_CHDIR            0x00000040
> > >  /* called from RCU mode, don't block */
> > >  #define MAY_NOT_BLOCK                0x00000080
> > > +#define MAY_UNLINK           0x00000100
> > > +#define MAY_RMDIR            0x00000200
> > >
> > >  /*
> > >   * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must
> > correspond
> > > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> > > index bb8467cd11ae..68f5d4aaf1ae 100644
> > > --- a/include/linux/fsnotify.h
> > > +++ b/include/linux/fsnotify.h
> > > @@ -80,6 +80,22 @@ static inline int fsnotify_parent(struct dentry
> > *dentry, __u32 mask,
> > >       return fsnotify(mask, data, data_type, NULL, NULL, inode, 0);
> > >  }
> > >
> > > +static inline int fsnotify_path_perm(struct path *path, struct dentry
> > *dentry, __u32 mask)
> > > +{
> > > +     __u32 fsnotify_mask = 0;
> > > +
> > > +     if (!(mask & (MAY_UNLINK | MAY_RMDIR)))
> > > +             return 0;
> > > +
> > > +     if (mask & MAY_UNLINK)
> > > +             fsnotify_mask |= FS_UNLINK_PERM;
> > > +
> > > +     if (mask & MAY_RMDIR)
> > > +             fsnotify_mask |= FS_RMDIR_PERM;
> > > +
> > > +     return fsnotify_parent(dentry, fsnotify_mask, path,
> > FSNOTIFY_EVENT_PATH);
> > > +}
> > > +
> > >  /*
> > >   * Simple wrappers to consolidate calls to fsnotify_parent() when an
> > event
> > >   * is on a file/dentry.
> > > diff --git a/include/linux/fsnotify_backend.h
> > b/include/linux/fsnotify_backend.h
> > > index 0805b74cae44..0e2e240e8234 100644
> > > --- a/include/linux/fsnotify_backend.h
> > > +++ b/include/linux/fsnotify_backend.h
> > > @@ -54,6 +54,8 @@
> > >  #define FS_OPEN_PERM         0x00010000      /* open event in an
> > permission hook */
> > >  #define FS_ACCESS_PERM               0x00020000      /* access event in
> > a permissions hook */
> > >  #define FS_OPEN_EXEC_PERM    0x00040000      /* open/exec event in a
> > permission hook */
> > > +#define FS_UNLINK_PERM               0x00080000      /* unlink event in
> > a permission hook */
> > > +#define FS_RMDIR_PERM                0x00100000      /* rmdir event in
> > a permission hook */
> > >
> > >  #define FS_EXCL_UNLINK               0x04000000      /* do not send
> > events if object is unlinked */
> > >  /*
> > > @@ -79,7 +81,9 @@
> > >  #define ALL_FSNOTIFY_DIRENT_EVENTS (FS_CREATE | FS_DELETE | FS_MOVE |
> > FS_RENAME)
> > >
> > >  #define ALL_FSNOTIFY_PERM_EVENTS (FS_OPEN_PERM | FS_ACCESS_PERM | \
> > > -                               FS_OPEN_EXEC_PERM)
> > > +                               FS_OPEN_EXEC_PERM | \
> > > +                               FS_UNLINK_PERM | \
> > > +                               FS_RMDIR_PERM)
> > >
> > >  /*
> > >   * This is a list of all events that may get sent to a parent that is
> > watching
> > > diff --git a/security/security.c b/security/security.c
> > > index b7cf5cbfdc67..8efc00ec02ed 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -1160,16 +1160,24 @@ EXPORT_SYMBOL(security_path_mkdir);
> > >
> > >  int security_path_rmdir(const struct path *dir, struct dentry *dentry)
> > >  {
> > > +     int ret;
> > >       if (unlikely(IS_PRIVATE(d_backing_inode(dir->dentry))))
> > >               return 0;
> > > -     return call_int_hook(path_rmdir, 0, dir, dentry);
> > > +     ret = call_int_hook(path_rmdir, 0, dir, dentry);
> > > +     if (ret)
> > > +             return ret;
> > > +     return fsnotify_path_perm(dir, dentry, MAY_RMDIR);
> > >  }
> > >
> > >  int security_path_unlink(const struct path *dir, struct dentry *dentry)
> > >  {
> > > +     int ret;
> > >       if (unlikely(IS_PRIVATE(d_backing_inode(dir->dentry))))
> > >               return 0;
> > > -     return call_int_hook(path_unlink, 0, dir, dentry);
> > > +     ret = call_int_hook(path_unlink, 0, dir, dentry);
> > > +     if (ret)
> > > +             return ret;
> > > +     return fsnotify_path_perm(dir, dentry, MAY_UNLINK);
> > >  }
> > >  EXPORT_SYMBOL(security_path_unlink);
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index e9e959343de9..f0780f0eb903 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -1801,8 +1801,12 @@ static int may_create(struct inode *dir,
> > >  }
> > >
> > >  #define MAY_LINK     0
> > > +#ifndef MAY_UNLINK
> > >  #define MAY_UNLINK   1
> > > +#endif
> > > +#ifndef MAY_RMDIR
> > >  #define MAY_RMDIR    2
> > > +#endif
> > >
> > >  /* Check whether a task can link, unlink, or rmdir a file/directory. */
> > >  static int may_link(struct inode *dir,
> > > --
> > > 2.17.1
> > >
> > --
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
> >
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fsnotify: add generic perm check for unlink/rmdir

[Amir Goldstein]

On Thu, May 5, 2022 at 6:28 AM guowei du <duguoweisz@gmail.com> wrote:
>
> thanks very much for your replay.
>
> I am a starter for kernel filesystem study, i see the newest plan with the 'pre modify events',
> I think the plan is great and meaningful,I am looking forward to it.

Welcome. Since you are new let me start with some basics.
I don't know what generated the long list of CC that you used,
I suppose it was get_maintainer.pl - this list is way too long and irrelevant
I cut it down to the list and maintainers listed in the MAINTAINERS file.

>
> for the legacy modify events, i mean to implement most blocking events which are not yet
> done for now, maybe the permission model is old or legacy, and,sure ,expending the
> blocking events such as unlink/rmdir/rename will do pollute EVENTS namespace in part.
> but, it is only a little ,maybe all usual blocking events are  open/access/rmdir/unlink/rename,
> they cover some usecases,and other modify events can be only audited or notified.

Sorry, I don't understand what you mean.

>
> With the fanotify, open/access/rmdir/unlink/rename need to occupy a fsnotify bit to explicitly
> separate from others.if Blocking event is denied,then there will be no normal events to notify.
>

Sorry, I don't understand what you mean.
What I meant is that adding different bits for FAN_OPEN and FAN_OPEN_PERM
was a mistake that was done a long time ago and we need to live with it.
We do NOT need to repeat the same mistake again.

We need to initialize fanotify with class FAN_CLASS_PERM and then when
setting events FAN_UNLINK|FAN_RMDIR in mask they will be blocking events
which reader will need to allow/deny.

Here is my old example implementation of dir modify permission events that use
just one more bit in mask:
https://github.com/amir73il/linux/commits/fsnotify_dirent_perm

This was never designed to be exported to users via fanotify, but it could
be extended. I also did not think yet how this could be combined with pre-modify
events that have different implementations.
I am just giving that as an example of how only a single new bit can be used
to describe blocking events for all the legacy notification events.

> By now ,fanotify is a way that userspace could make choice to allow or deny some events,so
> I expand fsnotify and use fanotify to do some work.
>
> so,that is why I expand the fsnotify blocking events.

Since you are new, I will try to be very clear about expectations.
In order to get the requested changes into the kernel you will have to
address the comments that we gave you.
If you do not understand the comments, please ask for clarifications.
These will not be the last comments that you will get.
Other people may also have more comments on your patches.
You will be asked to write tests.

Thanks,
Amir.

Re: [PATCH] fsnotify: add generic perm check for unlink/rmdir

[Amir Goldstein]

On Thu, May 5, 2022 at 10:22 AM guowei du <duguoweisz@gmail.com> wrote:
>
> Ok, I understand.
>
> All replies are very important to me,I need to make some changes for my mistakes.
> I should not do the same wrong things for now.

One more newbie mistake to correct - no "top posting" please.
Answers below the questions.

See one more important comment below.

>
>
> thanks very much.
>
> On Thu, May 5, 2022 at 1:23 PM Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> On Thu, May 5, 2022 at 6:28 AM guowei du <duguoweisz@gmail.com> wrote:
>> >
>> > thanks very much for your replay.
>> >
>> > I am a starter for kernel filesystem study, i see the newest plan with the 'pre modify events',
>> > I think the plan is great and meaningful,I am looking forward to it.
>>
>> Welcome. Since you are new let me start with some basics.
>> I don't know what generated the long list of CC that you used,
>> I suppose it was get_maintainer.pl - this list is way too long and irrelevant
>> I cut it down to the list and maintainers listed in the MAINTAINERS file.
>>
>> >
>> > for the legacy modify events, i mean to implement most blocking events which are not yet
>> > done for now, maybe the permission model is old or legacy, and,sure ,expending the
>> > blocking events such as unlink/rmdir/rename will do pollute EVENTS namespace in part.
>> > but, it is only a little ,maybe all usual blocking events are  open/access/rmdir/unlink/rename,
>> > they cover some usecases,and other modify events can be only audited or notified.
>>
>> Sorry, I don't understand what you mean.
>>
>> >
>> > With the fanotify, open/access/rmdir/unlink/rename need to occupy a fsnotify bit to explicitly
>> > separate from others.if Blocking event is denied,then there will be no normal events to notify.
>> >
>>
>> Sorry, I don't understand what you mean.
>> What I meant is that adding different bits for FAN_OPEN and FAN_OPEN_PERM
>> was a mistake that was done a long time ago and we need to live with it.
>> We do NOT need to repeat the same mistake again.
>>
>> We need to initialize fanotify with class FAN_CLASS_PERM and then when
>> setting events FAN_UNLINK|FAN_RMDIR in mask they will be blocking events
>> which reader will need to allow/deny.
>>
>> Here is my old example implementation of dir modify permission events that use
>> just one more bit in mask:
>> https://github.com/amir73il/linux/commits/fsnotify_dirent_perm
>>

As you can see in the branch above, this is similar to your patch
but more generic.

However, I did not like this approach especially if exporting blocking events
to userspace because the hooks are called with directory inode locked.

That means that as long as the monitoring program does not approve
an unlink, creation of files in that directory are blocked as well.
This could also result in deadlock because userland is not aware of
directory locking order.

So IF we are going to support those blocking events in userspace
and be aware that it is not at all certina that we will, then those events
better be called without the inode lock held as in my pre_modify
patches. I can split
#define FS_NAME_INTENT         0x02000000      /* Subfile about to be
linked/unlinked */
To FS_LINK_INTENT and FS_UNLINK_INTENT if needed
that is not a problem, but separate events for RMDIR and UNLINK is
an overkill. You will be able to use FAN_ONDIR to request or ignore
FS_UNLINK_INTENT events on directories.

I expect to post these patches within month or two.

Thanks,
Amir.

Re: [PATCH] fsnotify: add generic perm check for unlink/rmdir

[Paul Moore]

On Tue, May 3, 2022 at 2:38 PM Guowei Du <duguoweisz@gmail.com> wrote:
>
> From: duguowei <duguowei@xiaomi.com>
>
> For now, there have been open/access/open_exec perms for file operation,
> so we add new perms check with unlink/rmdir syscall. if one app deletes
> any file/dir within pubic area, fsnotify can sends fsnotify_event to
> listener to deny that, even if the app have right dac/mac permissions.
>
> Signed-off-by: duguowei <duguowei@xiaomi.com>
> ---
>  fs/notify/fsnotify.c             |  2 +-
>  include/linux/fs.h               |  2 ++
>  include/linux/fsnotify.h         | 16 ++++++++++++++++
>  include/linux/fsnotify_backend.h |  6 +++++-
>  security/security.c              | 12 ++++++++++--
>  security/selinux/hooks.c         |  4 ++++
>  6 files changed, 38 insertions(+), 4 deletions(-)

...

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e9e959343de9..f0780f0eb903 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1801,8 +1801,12 @@ static int may_create(struct inode *dir,
>  }
>
>  #define MAY_LINK       0
> +#ifndef MAY_UNLINK
>  #define MAY_UNLINK     1
> +#endif
> +#ifndef MAY_RMDIR
>  #define MAY_RMDIR      2
> +#endif

In the future if you run into a symbol collision here I would prefer
if you renamed the SELinux constants to something like SEL_MAY_LINK,
etc.

-- 
paul-moore.com