diff mbox series

[v3,02/25] ima: Align ima_post_path_mknod() definition with LSM infrastructure

Message ID 20230904133415.1799503-3-roberto.sassu@huaweicloud.com (mailing list archive)
State Superseded
Delegated to: Paul Moore
Headers show
Series security: Move IMA and EVM to the LSM infrastructure | expand

Commit Message

Roberto Sassu Sept. 4, 2023, 1:33 p.m. UTC
From: Roberto Sassu <roberto.sassu@huawei.com>

Change ima_post_path_mknod() definition, so that it can be registered as
implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
well.

Also, make sure that ima_post_path_mknod() is executed only if
(mode & S_IFMT) is equal to zero or S_IFREG.

Add this check to take into account the different placement of the
path_post_mknod hook (to be introduced) in do_mknodat(). Since the new hook
will be placed after the switch(), the check ensures that
ima_post_path_mknod() is invoked as originally intended when it is
registered as implementation of path_post_mknod.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 fs/namei.c                        |  9 ++++++---
 include/linux/ima.h               |  7 +++++--
 security/integrity/ima/ima_main.c | 10 +++++++++-
 3 files changed, 20 insertions(+), 6 deletions(-)

Comments

Stefan Berger Sept. 5, 2023, 5:23 p.m. UTC | #1
On 9/4/23 09:33, Roberto Sassu wrote:

> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> Change ima_post_path_mknod() definition, so that it can be registered as
> implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
> mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
> well.
>
> Also, make sure that ima_post_path_mknod() is executed only if
> (mode & S_IFMT) is equal to zero or S_IFREG.
>
> Add this check to take into account the different placement of the
> path_post_mknod hook (to be introduced) in do_mknodat(). Since the new hook
> will be placed after the switch(), the check ensures that
> ima_post_path_mknod() is invoked as originally intended when it is
> registered as implementation of path_post_mknod.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   fs/namei.c                        |  9 ++++++---
>   include/linux/ima.h               |  7 +++++--
>   security/integrity/ima/ima_main.c | 10 +++++++++-
>   3 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index e56ff39a79bc..c5e96f716f98 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4024,6 +4024,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
>   	struct path path;
>   	int error;
>   	unsigned int lookup_flags = 0;
> +	umode_t mode_stripped;
>   
>   	error = may_mknod(mode);
>   	if (error)
> @@ -4034,8 +4035,9 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
>   	if (IS_ERR(dentry))
>   		goto out1;
>   
> -	error = security_path_mknod(&path, dentry,
> -			mode_strip_umask(path.dentry->d_inode, mode), dev);
> +	mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
> +
> +	error = security_path_mknod(&path, dentry, mode_stripped, dev);
>   	if (error)
>   		goto out2;
>   
> @@ -4045,7 +4047,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
>   			error = vfs_create(idmap, path.dentry->d_inode,
>   					   dentry, mode, true);
>   			if (!error)
> -				ima_post_path_mknod(idmap, dentry);
> +				ima_post_path_mknod(idmap, &path, dentry,
> +						    mode_stripped, dev);
>   			break;
>   		case S_IFCHR: case S_IFBLK:
>   			error = vfs_mknod(idmap, path.dentry->d_inode,
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 910a2f11a906..179ce52013b2 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -32,7 +32,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
>   extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
>   			      enum kernel_read_file_id id);
>   extern void ima_post_path_mknod(struct mnt_idmap *idmap,
> -				struct dentry *dentry);
> +				const struct path *dir, struct dentry *dentry,
> +				umode_t mode, unsigned int dev);
>   extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
>   extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
>   extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> @@ -114,7 +115,9 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
>   }
>   
>   static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
> -				       struct dentry *dentry)
> +				       const struct path *dir,
> +				       struct dentry *dentry,
> +				       umode_t mode, unsigned int dev)
>   {
>   	return;
>   }
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 365db0e43d7c..76eba92d7f10 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -696,18 +696,26 @@ void ima_post_create_tmpfile(struct mnt_idmap *idmap,
>   /**
>    * ima_post_path_mknod - mark as a new inode
>    * @idmap: idmap of the mount the inode was found from
> + * @dir: path structure of parent of the new file
>    * @dentry: newly created dentry
> + * @mode: mode of the new file
> + * @dev: undecoded device number
>    *
>    * Mark files created via the mknodat syscall as new, so that the
>    * file data can be written later.
>    */
>   void ima_post_path_mknod(struct mnt_idmap *idmap,
> -			 struct dentry *dentry)
> +			 const struct path *dir, struct dentry *dentry,
> +			 umode_t mode, unsigned int dev)
>   {
>   	struct integrity_iint_cache *iint;
>   	struct inode *inode = dentry->d_inode;
>   	int must_appraise;
>   
> +	/* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
> +	if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
> +		return;
> +

These checks are only needed later (16/25) but IMO ok to introduce now.

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Mimi Zohar Oct. 11, 2023, 2:38 p.m. UTC | #2
On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Change ima_post_path_mknod() definition, so that it can be registered as
> implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
> mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
> well.
> Also, make sure that ima_post_path_mknod() is executed only if
> (mode & S_IFMT) is equal to zero or S_IFREG.
> 
> Add this check to take into account the different placement of the
> path_post_mknod hook (to be introduced) in do_mknodat().

Move "(to be introduced)" to when it is first mentioned.

> Since the new hook
> will be placed after the switch(), the check ensures that
> ima_post_path_mknod() is invoked as originally intended when it is
> registered as implementation of path_post_mknod.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  fs/namei.c                        |  9 ++++++---
>  include/linux/ima.h               |  7 +++++--
>  security/integrity/ima/ima_main.c | 10 +++++++++-
>  3 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index e56ff39a79bc..c5e96f716f98 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4024,6 +4024,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
>  	struct path path;
>  	int error;
>  	unsigned int lookup_flags = 0;
> +	umode_t mode_stripped;
>  
>  	error = may_mknod(mode);
>  	if (error)
> @@ -4034,8 +4035,9 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
>  	if (IS_ERR(dentry))
>  		goto out1;
>  
> -	error = security_path_mknod(&path, dentry,
> -			mode_strip_umask(path.dentry->d_inode, mode), dev);
> +	mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
> +
> +	error = security_path_mknod(&path, dentry, mode_stripped, dev);
>  	if (error)
>  		goto out2;
>  
> @@ -4045,7 +4047,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
>  			error = vfs_create(idmap, path.dentry->d_inode,
>  					   dentry, mode, true);
>  			if (!error)
> -				ima_post_path_mknod(idmap, dentry);
> +				ima_post_path_mknod(idmap, &path, dentry,
> +						    mode_stripped, dev);
>  			break;
>  		case S_IFCHR: case S_IFBLK:
>  			error = vfs_mknod(idmap, path.dentry->d_inode,
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 910a2f11a906..179ce52013b2 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -32,7 +32,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
>  extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
>  			      enum kernel_read_file_id id);
>  extern void ima_post_path_mknod(struct mnt_idmap *idmap,
> -				struct dentry *dentry);
> +				const struct path *dir, struct dentry *dentry,
> +				umode_t mode, unsigned int dev);
>  extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
>  extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
>  extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> @@ -114,7 +115,9 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
>  }
>  
>  static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
> -				       struct dentry *dentry)
> +				       const struct path *dir,
> +				       struct dentry *dentry,
> +				       umode_t mode, unsigned int dev)
>  {
>  	return;
>  }
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 365db0e43d7c..76eba92d7f10 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -696,18 +696,26 @@ void ima_post_create_tmpfile(struct mnt_idmap *idmap,
>  /**
>   * ima_post_path_mknod - mark as a new inode
>   * @idmap: idmap of the mount the inode was found from
> + * @dir: path structure of parent of the new file
>   * @dentry: newly created dentry
> + * @mode: mode of the new file
> + * @dev: undecoded device number
>   *
>   * Mark files created via the mknodat syscall as new, so that the
>   * file data can be written later.
>   */
>  void ima_post_path_mknod(struct mnt_idmap *idmap,
> -			 struct dentry *dentry)
> +			 const struct path *dir, struct dentry *dentry,
> +			 umode_t mode, unsigned int dev)
>  {
>  	struct integrity_iint_cache *iint;
>  	struct inode *inode = dentry->d_inode;
>  	int must_appraise;
>  
> +	/* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
> +	if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
> +		return;
> +

There's already a check below to make sure that this is a regular file.
Are both needed?

>  	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
>  		return;
>
Roberto Sassu Oct. 11, 2023, 4:02 p.m. UTC | #3
On Wed, 2023-10-11 at 10:38 -0400, Mimi Zohar wrote:
> On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Change ima_post_path_mknod() definition, so that it can be registered as
> > implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
> > mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
> > well.
> > Also, make sure that ima_post_path_mknod() is executed only if
> > (mode & S_IFMT) is equal to zero or S_IFREG.
> > 
> > Add this check to take into account the different placement of the
> > path_post_mknod hook (to be introduced) in do_mknodat().
> 
> Move "(to be introduced)" to when it is first mentioned.
> 
> > Since the new hook
> > will be placed after the switch(), the check ensures that
> > ima_post_path_mknod() is invoked as originally intended when it is
> > registered as implementation of path_post_mknod.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  fs/namei.c                        |  9 ++++++---
> >  include/linux/ima.h               |  7 +++++--
> >  security/integrity/ima/ima_main.c | 10 +++++++++-
> >  3 files changed, 20 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index e56ff39a79bc..c5e96f716f98 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -4024,6 +4024,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> >  	struct path path;
> >  	int error;
> >  	unsigned int lookup_flags = 0;
> > +	umode_t mode_stripped;
> >  
> >  	error = may_mknod(mode);
> >  	if (error)
> > @@ -4034,8 +4035,9 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> >  	if (IS_ERR(dentry))
> >  		goto out1;
> >  
> > -	error = security_path_mknod(&path, dentry,
> > -			mode_strip_umask(path.dentry->d_inode, mode), dev);
> > +	mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
> > +
> > +	error = security_path_mknod(&path, dentry, mode_stripped, dev);
> >  	if (error)
> >  		goto out2;
> >  
> > @@ -4045,7 +4047,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> >  			error = vfs_create(idmap, path.dentry->d_inode,
> >  					   dentry, mode, true);
> >  			if (!error)
> > -				ima_post_path_mknod(idmap, dentry);
> > +				ima_post_path_mknod(idmap, &path, dentry,
> > +						    mode_stripped, dev);
> >  			break;
> >  		case S_IFCHR: case S_IFBLK:
> >  			error = vfs_mknod(idmap, path.dentry->d_inode,
> > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > index 910a2f11a906..179ce52013b2 100644
> > --- a/include/linux/ima.h
> > +++ b/include/linux/ima.h
> > @@ -32,7 +32,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
> >  extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> >  			      enum kernel_read_file_id id);
> >  extern void ima_post_path_mknod(struct mnt_idmap *idmap,
> > -				struct dentry *dentry);
> > +				const struct path *dir, struct dentry *dentry,
> > +				umode_t mode, unsigned int dev);
> >  extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> >  extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
> >  extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> > @@ -114,7 +115,9 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
> >  }
> >  
> >  static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
> > -				       struct dentry *dentry)
> > +				       const struct path *dir,
> > +				       struct dentry *dentry,
> > +				       umode_t mode, unsigned int dev)
> >  {
> >  	return;
> >  }
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 365db0e43d7c..76eba92d7f10 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -696,18 +696,26 @@ void ima_post_create_tmpfile(struct mnt_idmap *idmap,
> >  /**
> >   * ima_post_path_mknod - mark as a new inode
> >   * @idmap: idmap of the mount the inode was found from
> > + * @dir: path structure of parent of the new file
> >   * @dentry: newly created dentry
> > + * @mode: mode of the new file
> > + * @dev: undecoded device number
> >   *
> >   * Mark files created via the mknodat syscall as new, so that the
> >   * file data can be written later.
> >   */
> >  void ima_post_path_mknod(struct mnt_idmap *idmap,
> > -			 struct dentry *dentry)
> > +			 const struct path *dir, struct dentry *dentry,
> > +			 umode_t mode, unsigned int dev)
> >  {
> >  	struct integrity_iint_cache *iint;
> >  	struct inode *inode = dentry->d_inode;
> >  	int must_appraise;
> >  
> > +	/* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
> > +	if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
> > +		return;
> > +
> 
> There's already a check below to make sure that this is a regular file.
> Are both needed?

You are right, I can remove the first check.

Thanks

Roberto

> >  	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> >  		return;
> >
Mimi Zohar Oct. 11, 2023, 7:01 p.m. UTC | #4
On Wed, 2023-10-11 at 18:02 +0200, Roberto Sassu wrote:
> On Wed, 2023-10-11 at 10:38 -0400, Mimi Zohar wrote:
> > On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > 
> > > Change ima_post_path_mknod() definition, so that it can be registered as
> > > implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
> > > mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
> > > well.
> > > Also, make sure that ima_post_path_mknod() is executed only if
> > > (mode & S_IFMT) is equal to zero or S_IFREG.
> > > 
> > > Add this check to take into account the different placement of the
> > > path_post_mknod hook (to be introduced) in do_mknodat().
> > 
> > Move "(to be introduced)" to when it is first mentioned.
> > 
> > > Since the new hook
> > > will be placed after the switch(), the check ensures that
> > > ima_post_path_mknod() is invoked as originally intended when it is
> > > registered as implementation of path_post_mknod.
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >  fs/namei.c                        |  9 ++++++---
> > >  include/linux/ima.h               |  7 +++++--
> > >  security/integrity/ima/ima_main.c | 10 +++++++++-
> > >  3 files changed, 20 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index e56ff39a79bc..c5e96f716f98 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -4024,6 +4024,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > >  	struct path path;
> > >  	int error;
> > >  	unsigned int lookup_flags = 0;
> > > +	umode_t mode_stripped;
> > >  
> > >  	error = may_mknod(mode);
> > >  	if (error)
> > > @@ -4034,8 +4035,9 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > >  	if (IS_ERR(dentry))
> > >  		goto out1;
> > >  
> > > -	error = security_path_mknod(&path, dentry,
> > > -			mode_strip_umask(path.dentry->d_inode, mode), dev);
> > > +	mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
> > > +
> > > +	error = security_path_mknod(&path, dentry, mode_stripped, dev);
> > >  	if (error)
> > >  		goto out2;
> > >  
> > > @@ -4045,7 +4047,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > >  			error = vfs_create(idmap, path.dentry->d_inode,
> > >  					   dentry, mode, true);
> > >  			if (!error)
> > > -				ima_post_path_mknod(idmap, dentry);
> > > +				ima_post_path_mknod(idmap, &path, dentry,
> > > +						    mode_stripped, dev);
> > >  			break;
> > >  		case S_IFCHR: case S_IFBLK:
> > >  			error = vfs_mknod(idmap, path.dentry->d_inode,
> > > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > > index 910a2f11a906..179ce52013b2 100644
> > > --- a/include/linux/ima.h
> > > +++ b/include/linux/ima.h
> > > @@ -32,7 +32,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
> > >  extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > >  			      enum kernel_read_file_id id);
> > >  extern void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > -				struct dentry *dentry);
> > > +				const struct path *dir, struct dentry *dentry,
> > > +				umode_t mode, unsigned int dev);
> > >  extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> > >  extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
> > >  extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> > > @@ -114,7 +115,9 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > >  }
> > >  
> > >  static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > -				       struct dentry *dentry)
> > > +				       const struct path *dir,
> > > +				       struct dentry *dentry,
> > > +				       umode_t mode, unsigned int dev)
> > >  {
> > >  	return;
> > >  }
> > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > index 365db0e43d7c..76eba92d7f10 100644
> > > --- a/security/integrity/ima/ima_main.c
> > > +++ b/security/integrity/ima/ima_main.c
> > > @@ -696,18 +696,26 @@ void ima_post_create_tmpfile(struct mnt_idmap *idmap,
> > >  /**
> > >   * ima_post_path_mknod - mark as a new inode
> > >   * @idmap: idmap of the mount the inode was found from
> > > + * @dir: path structure of parent of the new file
> > >   * @dentry: newly created dentry
> > > + * @mode: mode of the new file
> > > + * @dev: undecoded device number
> > >   *
> > >   * Mark files created via the mknodat syscall as new, so that the
> > >   * file data can be written later.
> > >   */
> > >  void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > -			 struct dentry *dentry)
> > > +			 const struct path *dir, struct dentry *dentry,
> > > +			 umode_t mode, unsigned int dev)
> > >  {
> > >  	struct integrity_iint_cache *iint;
> > >  	struct inode *inode = dentry->d_inode;
> > >  	int must_appraise;
> > >  
> > > +	/* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
> > > +	if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
> > > +		return;
> > > +
> > 
> > There's already a check below to make sure that this is a regular file.
> > Are both needed?
> 
> You are right, I can remove the first check.

The question then becomes why modify hook the arguments?   

> 
> > >  	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> > >  		return;
> > >  
>
Roberto Sassu Oct. 12, 2023, 7:29 a.m. UTC | #5
On Wed, 2023-10-11 at 15:01 -0400, Mimi Zohar wrote:
> On Wed, 2023-10-11 at 18:02 +0200, Roberto Sassu wrote:
> > On Wed, 2023-10-11 at 10:38 -0400, Mimi Zohar wrote:
> > > On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > 
> > > > Change ima_post_path_mknod() definition, so that it can be registered as
> > > > implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
> > > > mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
> > > > well.
> > > > Also, make sure that ima_post_path_mknod() is executed only if
> > > > (mode & S_IFMT) is equal to zero or S_IFREG.
> > > > 
> > > > Add this check to take into account the different placement of the
> > > > path_post_mknod hook (to be introduced) in do_mknodat().
> > > 
> > > Move "(to be introduced)" to when it is first mentioned.
> > > 
> > > > Since the new hook
> > > > will be placed after the switch(), the check ensures that
> > > > ima_post_path_mknod() is invoked as originally intended when it is
> > > > registered as implementation of path_post_mknod.
> > > > 
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > ---
> > > >  fs/namei.c                        |  9 ++++++---
> > > >  include/linux/ima.h               |  7 +++++--
> > > >  security/integrity/ima/ima_main.c | 10 +++++++++-
> > > >  3 files changed, 20 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/fs/namei.c b/fs/namei.c
> > > > index e56ff39a79bc..c5e96f716f98 100644
> > > > --- a/fs/namei.c
> > > > +++ b/fs/namei.c
> > > > @@ -4024,6 +4024,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > >  	struct path path;
> > > >  	int error;
> > > >  	unsigned int lookup_flags = 0;
> > > > +	umode_t mode_stripped;
> > > >  
> > > >  	error = may_mknod(mode);
> > > >  	if (error)
> > > > @@ -4034,8 +4035,9 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > >  	if (IS_ERR(dentry))
> > > >  		goto out1;
> > > >  
> > > > -	error = security_path_mknod(&path, dentry,
> > > > -			mode_strip_umask(path.dentry->d_inode, mode), dev);
> > > > +	mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
> > > > +
> > > > +	error = security_path_mknod(&path, dentry, mode_stripped, dev);
> > > >  	if (error)
> > > >  		goto out2;
> > > >  
> > > > @@ -4045,7 +4047,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > >  			error = vfs_create(idmap, path.dentry->d_inode,
> > > >  					   dentry, mode, true);
> > > >  			if (!error)
> > > > -				ima_post_path_mknod(idmap, dentry);
> > > > +				ima_post_path_mknod(idmap, &path, dentry,
> > > > +						    mode_stripped, dev);
> > > >  			break;
> > > >  		case S_IFCHR: case S_IFBLK:
> > > >  			error = vfs_mknod(idmap, path.dentry->d_inode,
> > > > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > > > index 910a2f11a906..179ce52013b2 100644
> > > > --- a/include/linux/ima.h
> > > > +++ b/include/linux/ima.h
> > > > @@ -32,7 +32,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
> > > >  extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > >  			      enum kernel_read_file_id id);
> > > >  extern void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > -				struct dentry *dentry);
> > > > +				const struct path *dir, struct dentry *dentry,
> > > > +				umode_t mode, unsigned int dev);
> > > >  extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> > > >  extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
> > > >  extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> > > > @@ -114,7 +115,9 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > >  }
> > > >  
> > > >  static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > -				       struct dentry *dentry)
> > > > +				       const struct path *dir,
> > > > +				       struct dentry *dentry,
> > > > +				       umode_t mode, unsigned int dev)
> > > >  {
> > > >  	return;
> > > >  }
> > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > > index 365db0e43d7c..76eba92d7f10 100644
> > > > --- a/security/integrity/ima/ima_main.c
> > > > +++ b/security/integrity/ima/ima_main.c
> > > > @@ -696,18 +696,26 @@ void ima_post_create_tmpfile(struct mnt_idmap *idmap,
> > > >  /**
> > > >   * ima_post_path_mknod - mark as a new inode
> > > >   * @idmap: idmap of the mount the inode was found from
> > > > + * @dir: path structure of parent of the new file
> > > >   * @dentry: newly created dentry
> > > > + * @mode: mode of the new file
> > > > + * @dev: undecoded device number
> > > >   *
> > > >   * Mark files created via the mknodat syscall as new, so that the
> > > >   * file data can be written later.
> > > >   */
> > > >  void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > -			 struct dentry *dentry)
> > > > +			 const struct path *dir, struct dentry *dentry,
> > > > +			 umode_t mode, unsigned int dev)
> > > >  {
> > > >  	struct integrity_iint_cache *iint;
> > > >  	struct inode *inode = dentry->d_inode;
> > > >  	int must_appraise;
> > > >  
> > > > +	/* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
> > > > +	if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
> > > > +		return;
> > > > +
> > > 
> > > There's already a check below to make sure that this is a regular file.
> > > Are both needed?
> > 
> > You are right, I can remove the first check.
> 
> The question then becomes why modify hook the arguments?   

We need to make sure that ima_post_path_mknod() has the same parameters
as the LSM hook at the time we register it to the LSM infrastructure.

Thanks

Roberto

> > 
> > > >  	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> > > >  		return;
> > > >  
> > 
>
Mimi Zohar Oct. 12, 2023, 11:42 a.m. UTC | #6
On Thu, 2023-10-12 at 09:29 +0200, Roberto Sassu wrote:
> On Wed, 2023-10-11 at 15:01 -0400, Mimi Zohar wrote:
> > On Wed, 2023-10-11 at 18:02 +0200, Roberto Sassu wrote:
> > > On Wed, 2023-10-11 at 10:38 -0400, Mimi Zohar wrote:
> > > > On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > 
> > > > > Change ima_post_path_mknod() definition, so that it can be registered as
> > > > > implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
> > > > > mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
> > > > > well.
> > > > > Also, make sure that ima_post_path_mknod() is executed only if
> > > > > (mode & S_IFMT) is equal to zero or S_IFREG.
> > > > > 
> > > > > Add this check to take into account the different placement of the
> > > > > path_post_mknod hook (to be introduced) in do_mknodat().
> > > > 
> > > > Move "(to be introduced)" to when it is first mentioned.
> > > > 
> > > > > Since the new hook
> > > > > will be placed after the switch(), the check ensures that
> > > > > ima_post_path_mknod() is invoked as originally intended when it is
> > > > > registered as implementation of path_post_mknod.
> > > > > 
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > ---
> > > > >  fs/namei.c                        |  9 ++++++---
> > > > >  include/linux/ima.h               |  7 +++++--
> > > > >  security/integrity/ima/ima_main.c | 10 +++++++++-
> > > > >  3 files changed, 20 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/fs/namei.c b/fs/namei.c
> > > > > index e56ff39a79bc..c5e96f716f98 100644
> > > > > --- a/fs/namei.c
> > > > > +++ b/fs/namei.c
> > > > > @@ -4024,6 +4024,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > >  	struct path path;
> > > > >  	int error;
> > > > >  	unsigned int lookup_flags = 0;
> > > > > +	umode_t mode_stripped;
> > > > >  
> > > > >  	error = may_mknod(mode);
> > > > >  	if (error)
> > > > > @@ -4034,8 +4035,9 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > >  	if (IS_ERR(dentry))
> > > > >  		goto out1;
> > > > >  
> > > > > -	error = security_path_mknod(&path, dentry,
> > > > > -			mode_strip_umask(path.dentry->d_inode, mode), dev);
> > > > > +	mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
> > > > > +
> > > > > +	error = security_path_mknod(&path, dentry, mode_stripped, dev);
> > > > >  	if (error)
> > > > >  		goto out2;
> > > > >  
> > > > > @@ -4045,7 +4047,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > >  			error = vfs_create(idmap, path.dentry->d_inode,
> > > > >  					   dentry, mode, true);
> > > > >  			if (!error)
> > > > > -				ima_post_path_mknod(idmap, dentry);
> > > > > +				ima_post_path_mknod(idmap, &path, dentry,
> > > > > +						    mode_stripped, dev);
> > > > >  			break;
> > > > >  		case S_IFCHR: case S_IFBLK:
> > > > >  			error = vfs_mknod(idmap, path.dentry->d_inode,
> > > > > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > > > > index 910a2f11a906..179ce52013b2 100644
> > > > > --- a/include/linux/ima.h
> > > > > +++ b/include/linux/ima.h
> > > > > @@ -32,7 +32,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
> > > > >  extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > > >  			      enum kernel_read_file_id id);
> > > > >  extern void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > -				struct dentry *dentry);
> > > > > +				const struct path *dir, struct dentry *dentry,
> > > > > +				umode_t mode, unsigned int dev);
> > > > >  extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> > > > >  extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
> > > > >  extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> > > > > @@ -114,7 +115,9 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > > >  }
> > > > >  
> > > > >  static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > -				       struct dentry *dentry)
> > > > > +				       const struct path *dir,
> > > > > +				       struct dentry *dentry,
> > > > > +				       umode_t mode, unsigned int dev)
> > > > >  {
> > > > >  	return;
> > > > >  }
> > > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > > > index 365db0e43d7c..76eba92d7f10 100644
> > > > > --- a/security/integrity/ima/ima_main.c
> > > > > +++ b/security/integrity/ima/ima_main.c
> > > > > @@ -696,18 +696,26 @@ void ima_post_create_tmpfile(struct mnt_idmap *idmap,
> > > > >  /**
> > > > >   * ima_post_path_mknod - mark as a new inode
> > > > >   * @idmap: idmap of the mount the inode was found from
> > > > > + * @dir: path structure of parent of the new file
> > > > >   * @dentry: newly created dentry
> > > > > + * @mode: mode of the new file
> > > > > + * @dev: undecoded device number
> > > > >   *
> > > > >   * Mark files created via the mknodat syscall as new, so that the
> > > > >   * file data can be written later.
> > > > >   */
> > > > >  void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > -			 struct dentry *dentry)
> > > > > +			 const struct path *dir, struct dentry *dentry,
> > > > > +			 umode_t mode, unsigned int dev)
> > > > >  {
> > > > >  	struct integrity_iint_cache *iint;
> > > > >  	struct inode *inode = dentry->d_inode;
> > > > >  	int must_appraise;
> > > > >  
> > > > > +	/* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
> > > > > +	if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
> > > > > +		return;
> > > > > +
> > > > 
> > > > There's already a check below to make sure that this is a regular file.
> > > > Are both needed?
> > > 
> > > You are right, I can remove the first check.
> > 
> > The question then becomes why modify hook the arguments?   
> 
> We need to make sure that ima_post_path_mknod() has the same parameters
> as the LSM hook at the time we register it to the LSM infrastructure.

I'm trying to understand why the pre hook parameters and the missing
IMA parameter are used, as opposed to just defining the new
post_path_mknod hook like IMA.

thanks,

Mimi

> 
> > > 
> > > > >  	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> > > > >  		return;
> > > > >  
> > > 
> > 
>
Roberto Sassu Oct. 12, 2023, 12:19 p.m. UTC | #7
On Thu, 2023-10-12 at 07:42 -0400, Mimi Zohar wrote:
> On Thu, 2023-10-12 at 09:29 +0200, Roberto Sassu wrote:
> > On Wed, 2023-10-11 at 15:01 -0400, Mimi Zohar wrote:
> > > On Wed, 2023-10-11 at 18:02 +0200, Roberto Sassu wrote:
> > > > On Wed, 2023-10-11 at 10:38 -0400, Mimi Zohar wrote:
> > > > > On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> > > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > 
> > > > > > Change ima_post_path_mknod() definition, so that it can be registered as
> > > > > > implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
> > > > > > mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
> > > > > > well.
> > > > > > Also, make sure that ima_post_path_mknod() is executed only if
> > > > > > (mode & S_IFMT) is equal to zero or S_IFREG.
> > > > > > 
> > > > > > Add this check to take into account the different placement of the
> > > > > > path_post_mknod hook (to be introduced) in do_mknodat().
> > > > > 
> > > > > Move "(to be introduced)" to when it is first mentioned.
> > > > > 
> > > > > > Since the new hook
> > > > > > will be placed after the switch(), the check ensures that
> > > > > > ima_post_path_mknod() is invoked as originally intended when it is
> > > > > > registered as implementation of path_post_mknod.
> > > > > > 
> > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > ---
> > > > > >  fs/namei.c                        |  9 ++++++---
> > > > > >  include/linux/ima.h               |  7 +++++--
> > > > > >  security/integrity/ima/ima_main.c | 10 +++++++++-
> > > > > >  3 files changed, 20 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/namei.c b/fs/namei.c
> > > > > > index e56ff39a79bc..c5e96f716f98 100644
> > > > > > --- a/fs/namei.c
> > > > > > +++ b/fs/namei.c
> > > > > > @@ -4024,6 +4024,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > >  	struct path path;
> > > > > >  	int error;
> > > > > >  	unsigned int lookup_flags = 0;
> > > > > > +	umode_t mode_stripped;
> > > > > >  
> > > > > >  	error = may_mknod(mode);
> > > > > >  	if (error)
> > > > > > @@ -4034,8 +4035,9 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > >  	if (IS_ERR(dentry))
> > > > > >  		goto out1;
> > > > > >  
> > > > > > -	error = security_path_mknod(&path, dentry,
> > > > > > -			mode_strip_umask(path.dentry->d_inode, mode), dev);
> > > > > > +	mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
> > > > > > +
> > > > > > +	error = security_path_mknod(&path, dentry, mode_stripped, dev);
> > > > > >  	if (error)
> > > > > >  		goto out2;
> > > > > >  
> > > > > > @@ -4045,7 +4047,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > >  			error = vfs_create(idmap, path.dentry->d_inode,
> > > > > >  					   dentry, mode, true);
> > > > > >  			if (!error)
> > > > > > -				ima_post_path_mknod(idmap, dentry);
> > > > > > +				ima_post_path_mknod(idmap, &path, dentry,
> > > > > > +						    mode_stripped, dev);
> > > > > >  			break;
> > > > > >  		case S_IFCHR: case S_IFBLK:
> > > > > >  			error = vfs_mknod(idmap, path.dentry->d_inode,
> > > > > > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > > > > > index 910a2f11a906..179ce52013b2 100644
> > > > > > --- a/include/linux/ima.h
> > > > > > +++ b/include/linux/ima.h
> > > > > > @@ -32,7 +32,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
> > > > > >  extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > > > >  			      enum kernel_read_file_id id);
> > > > > >  extern void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > > -				struct dentry *dentry);
> > > > > > +				const struct path *dir, struct dentry *dentry,
> > > > > > +				umode_t mode, unsigned int dev);
> > > > > >  extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> > > > > >  extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
> > > > > >  extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> > > > > > @@ -114,7 +115,9 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > > > >  }
> > > > > >  
> > > > > >  static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > > -				       struct dentry *dentry)
> > > > > > +				       const struct path *dir,
> > > > > > +				       struct dentry *dentry,
> > > > > > +				       umode_t mode, unsigned int dev)
> > > > > >  {
> > > > > >  	return;
> > > > > >  }
> > > > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > > > > index 365db0e43d7c..76eba92d7f10 100644
> > > > > > --- a/security/integrity/ima/ima_main.c
> > > > > > +++ b/security/integrity/ima/ima_main.c
> > > > > > @@ -696,18 +696,26 @@ void ima_post_create_tmpfile(struct mnt_idmap *idmap,
> > > > > >  /**
> > > > > >   * ima_post_path_mknod - mark as a new inode
> > > > > >   * @idmap: idmap of the mount the inode was found from
> > > > > > + * @dir: path structure of parent of the new file
> > > > > >   * @dentry: newly created dentry
> > > > > > + * @mode: mode of the new file
> > > > > > + * @dev: undecoded device number
> > > > > >   *
> > > > > >   * Mark files created via the mknodat syscall as new, so that the
> > > > > >   * file data can be written later.
> > > > > >   */
> > > > > >  void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > > -			 struct dentry *dentry)
> > > > > > +			 const struct path *dir, struct dentry *dentry,
> > > > > > +			 umode_t mode, unsigned int dev)
> > > > > >  {
> > > > > >  	struct integrity_iint_cache *iint;
> > > > > >  	struct inode *inode = dentry->d_inode;
> > > > > >  	int must_appraise;
> > > > > >  
> > > > > > +	/* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
> > > > > > +	if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
> > > > > > +		return;
> > > > > > +
> > > > > 
> > > > > There's already a check below to make sure that this is a regular file.
> > > > > Are both needed?
> > > > 
> > > > You are right, I can remove the first check.
> > > 
> > > The question then becomes why modify hook the arguments?   
> > 
> > We need to make sure that ima_post_path_mknod() has the same parameters
> > as the LSM hook at the time we register it to the LSM infrastructure.
> 
> I'm trying to understand why the pre hook parameters and the missing
> IMA parameter are used, as opposed to just defining the new
> post_path_mknod hook like IMA.

As an empyrical rule, I pass the same parameters as the corresponding
pre hook (plus idmap, in this case). This is similar to the
inode_setxattr hook. But I can be wrong, if desired I can reduce.

Thanks

Roberto

> thanks,
> 
> Mimi
> 
> > 
> > > > 
> > > > > >  	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> > > > > >  		return;
> > > > > >  
> > > > 
> > > 
> > 
>
Mimi Zohar Oct. 12, 2023, 1:25 p.m. UTC | #8
On Thu, 2023-10-12 at 14:19 +0200, Roberto Sassu wrote:
> On Thu, 2023-10-12 at 07:42 -0400, Mimi Zohar wrote:
> > On Thu, 2023-10-12 at 09:29 +0200, Roberto Sassu wrote:
> > > On Wed, 2023-10-11 at 15:01 -0400, Mimi Zohar wrote:
> > > > On Wed, 2023-10-11 at 18:02 +0200, Roberto Sassu wrote:
> > > > > On Wed, 2023-10-11 at 10:38 -0400, Mimi Zohar wrote:
> > > > > > On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > 
> > > > > > > Change ima_post_path_mknod() definition, so that it can be registered as
> > > > > > > implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
> > > > > > > mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
> > > > > > > well.
> > > > > > > Also, make sure that ima_post_path_mknod() is executed only if
> > > > > > > (mode & S_IFMT) is equal to zero or S_IFREG.
> > > > > > > 
> > > > > > > Add this check to take into account the different placement of the
> > > > > > > path_post_mknod hook (to be introduced) in do_mknodat().
> > > > > > 
> > > > > > Move "(to be introduced)" to when it is first mentioned.
> > > > > > 
> > > > > > > Since the new hook
> > > > > > > will be placed after the switch(), the check ensures that
> > > > > > > ima_post_path_mknod() is invoked as originally intended when it is
> > > > > > > registered as implementation of path_post_mknod.
> > > > > > > 
> > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > ---
> > > > > > >  fs/namei.c                        |  9 ++++++---
> > > > > > >  include/linux/ima.h               |  7 +++++--
> > > > > > >  security/integrity/ima/ima_main.c | 10 +++++++++-
> > > > > > >  3 files changed, 20 insertions(+), 6 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/fs/namei.c b/fs/namei.c
> > > > > > > index e56ff39a79bc..c5e96f716f98 100644
> > > > > > > --- a/fs/namei.c
> > > > > > > +++ b/fs/namei.c
> > > > > > > @@ -4024,6 +4024,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > > >  	struct path path;
> > > > > > >  	int error;
> > > > > > >  	unsigned int lookup_flags = 0;
> > > > > > > +	umode_t mode_stripped;
> > > > > > >  
> > > > > > >  	error = may_mknod(mode);
> > > > > > >  	if (error)
> > > > > > > @@ -4034,8 +4035,9 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > > >  	if (IS_ERR(dentry))
> > > > > > >  		goto out1;
> > > > > > >  
> > > > > > > -	error = security_path_mknod(&path, dentry,
> > > > > > > -			mode_strip_umask(path.dentry->d_inode, mode), dev);
> > > > > > > +	mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
> > > > > > > +
> > > > > > > +	error = security_path_mknod(&path, dentry, mode_stripped, dev);
> > > > > > >  	if (error)
> > > > > > >  		goto out2;
> > > > > > >  
> > > > > > > @@ -4045,7 +4047,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > > >  			error = vfs_create(idmap, path.dentry->d_inode,
> > > > > > >  					   dentry, mode, true);
> > > > > > >  			if (!error)
> > > > > > > -				ima_post_path_mknod(idmap, dentry);
> > > > > > > +				ima_post_path_mknod(idmap, &path, dentry,
> > > > > > > +						    mode_stripped, dev);
> > > > > > >  			break;
> > > > > > >  		case S_IFCHR: case S_IFBLK:
> > > > > > >  			error = vfs_mknod(idmap, path.dentry->d_inode,
> > > > > > > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > > > > > > index 910a2f11a906..179ce52013b2 100644
> > > > > > > --- a/include/linux/ima.h
> > > > > > > +++ b/include/linux/ima.h
> > > > > > > @@ -32,7 +32,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
> > > > > > >  extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > > > > >  			      enum kernel_read_file_id id);
> > > > > > >  extern void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > > > -				struct dentry *dentry);
> > > > > > > +				const struct path *dir, struct dentry *dentry,
> > > > > > > +				umode_t mode, unsigned int dev);
> > > > > > >  extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> > > > > > >  extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
> > > > > > >  extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> > > > > > > @@ -114,7 +115,9 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > > > > >  }
> > > > > > >  
> > > > > > >  static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > > > -				       struct dentry *dentry)
> > > > > > > +				       const struct path *dir,
> > > > > > > +				       struct dentry *dentry,
> > > > > > > +				       umode_t mode, unsigned int dev)
> > > > > > >  {
> > > > > > >  	return;
> > > > > > >  }
> > > > > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > > > > > index 365db0e43d7c..76eba92d7f10 100644
> > > > > > > --- a/security/integrity/ima/ima_main.c
> > > > > > > +++ b/security/integrity/ima/ima_main.c
> > > > > > > @@ -696,18 +696,26 @@ void ima_post_create_tmpfile(struct mnt_idmap *idmap,
> > > > > > >  /**
> > > > > > >   * ima_post_path_mknod - mark as a new inode
> > > > > > >   * @idmap: idmap of the mount the inode was found from
> > > > > > > + * @dir: path structure of parent of the new file
> > > > > > >   * @dentry: newly created dentry
> > > > > > > + * @mode: mode of the new file
> > > > > > > + * @dev: undecoded device number
> > > > > > >   *
> > > > > > >   * Mark files created via the mknodat syscall as new, so that the
> > > > > > >   * file data can be written later.
> > > > > > >   */
> > > > > > >  void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > > > -			 struct dentry *dentry)
> > > > > > > +			 const struct path *dir, struct dentry *dentry,
> > > > > > > +			 umode_t mode, unsigned int dev)
> > > > > > >  {
> > > > > > >  	struct integrity_iint_cache *iint;
> > > > > > >  	struct inode *inode = dentry->d_inode;
> > > > > > >  	int must_appraise;
> > > > > > >  
> > > > > > > +	/* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
> > > > > > > +	if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
> > > > > > > +		return;
> > > > > > > +
> > > > > > 
> > > > > > There's already a check below to make sure that this is a regular file.
> > > > > > Are both needed?
> > > > > 
> > > > > You are right, I can remove the first check.
> > > > 
> > > > The question then becomes why modify hook the arguments?   
> > > 
> > > We need to make sure that ima_post_path_mknod() has the same parameters
> > > as the LSM hook at the time we register it to the LSM infrastructure.
> > 
> > I'm trying to understand why the pre hook parameters and the missing
> > IMA parameter are used, as opposed to just defining the new
> > post_path_mknod hook like IMA.
> 
> As an empyrical rule, I pass the same parameters as the corresponding
> pre hook (plus idmap, in this case). This is similar to the
> inode_setxattr hook. But I can be wrong, if desired I can reduce.

The inode_setxattr hook change example is legitimate, as EVM includes
idmap, while IMA doesn't. 

Unless there is a good reason for the additional parameters, I'm not
sure that adding them makes sense.  Not modifying the parameter list
will reduce the size of this patch set.
Roberto Sassu Oct. 12, 2023, 1:33 p.m. UTC | #9
On Thu, 2023-10-12 at 09:25 -0400, Mimi Zohar wrote:
> On Thu, 2023-10-12 at 14:19 +0200, Roberto Sassu wrote:
> > On Thu, 2023-10-12 at 07:42 -0400, Mimi Zohar wrote:
> > > On Thu, 2023-10-12 at 09:29 +0200, Roberto Sassu wrote:
> > > > On Wed, 2023-10-11 at 15:01 -0400, Mimi Zohar wrote:
> > > > > On Wed, 2023-10-11 at 18:02 +0200, Roberto Sassu wrote:
> > > > > > On Wed, 2023-10-11 at 10:38 -0400, Mimi Zohar wrote:
> > > > > > > On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> > > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > > 
> > > > > > > > Change ima_post_path_mknod() definition, so that it can be registered as
> > > > > > > > implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
> > > > > > > > mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
> > > > > > > > well.
> > > > > > > > Also, make sure that ima_post_path_mknod() is executed only if
> > > > > > > > (mode & S_IFMT) is equal to zero or S_IFREG.
> > > > > > > > 
> > > > > > > > Add this check to take into account the different placement of the
> > > > > > > > path_post_mknod hook (to be introduced) in do_mknodat().
> > > > > > > 
> > > > > > > Move "(to be introduced)" to when it is first mentioned.
> > > > > > > 
> > > > > > > > Since the new hook
> > > > > > > > will be placed after the switch(), the check ensures that
> > > > > > > > ima_post_path_mknod() is invoked as originally intended when it is
> > > > > > > > registered as implementation of path_post_mknod.
> > > > > > > > 
> > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > > ---
> > > > > > > >  fs/namei.c                        |  9 ++++++---
> > > > > > > >  include/linux/ima.h               |  7 +++++--
> > > > > > > >  security/integrity/ima/ima_main.c | 10 +++++++++-
> > > > > > > >  3 files changed, 20 insertions(+), 6 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/fs/namei.c b/fs/namei.c
> > > > > > > > index e56ff39a79bc..c5e96f716f98 100644
> > > > > > > > --- a/fs/namei.c
> > > > > > > > +++ b/fs/namei.c
> > > > > > > > @@ -4024,6 +4024,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > > > >  	struct path path;
> > > > > > > >  	int error;
> > > > > > > >  	unsigned int lookup_flags = 0;
> > > > > > > > +	umode_t mode_stripped;
> > > > > > > >  
> > > > > > > >  	error = may_mknod(mode);
> > > > > > > >  	if (error)
> > > > > > > > @@ -4034,8 +4035,9 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > > > >  	if (IS_ERR(dentry))
> > > > > > > >  		goto out1;
> > > > > > > >  
> > > > > > > > -	error = security_path_mknod(&path, dentry,
> > > > > > > > -			mode_strip_umask(path.dentry->d_inode, mode), dev);
> > > > > > > > +	mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
> > > > > > > > +
> > > > > > > > +	error = security_path_mknod(&path, dentry, mode_stripped, dev);
> > > > > > > >  	if (error)
> > > > > > > >  		goto out2;
> > > > > > > >  
> > > > > > > > @@ -4045,7 +4047,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > > > >  			error = vfs_create(idmap, path.dentry->d_inode,
> > > > > > > >  					   dentry, mode, true);
> > > > > > > >  			if (!error)
> > > > > > > > -				ima_post_path_mknod(idmap, dentry);
> > > > > > > > +				ima_post_path_mknod(idmap, &path, dentry,
> > > > > > > > +						    mode_stripped, dev);
> > > > > > > >  			break;
> > > > > > > >  		case S_IFCHR: case S_IFBLK:
> > > > > > > >  			error = vfs_mknod(idmap, path.dentry->d_inode,
> > > > > > > > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > > > > > > > index 910a2f11a906..179ce52013b2 100644
> > > > > > > > --- a/include/linux/ima.h
> > > > > > > > +++ b/include/linux/ima.h
> > > > > > > > @@ -32,7 +32,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
> > > > > > > >  extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > > > > > >  			      enum kernel_read_file_id id);
> > > > > > > >  extern void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > > > > -				struct dentry *dentry);
> > > > > > > > +				const struct path *dir, struct dentry *dentry,
> > > > > > > > +				umode_t mode, unsigned int dev);
> > > > > > > >  extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> > > > > > > >  extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
> > > > > > > >  extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> > > > > > > > @@ -114,7 +115,9 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > > > > > >  }
> > > > > > > >  
> > > > > > > >  static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > > > > -				       struct dentry *dentry)
> > > > > > > > +				       const struct path *dir,
> > > > > > > > +				       struct dentry *dentry,
> > > > > > > > +				       umode_t mode, unsigned int dev)
> > > > > > > >  {
> > > > > > > >  	return;
> > > > > > > >  }
> > > > > > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > > > > > > index 365db0e43d7c..76eba92d7f10 100644
> > > > > > > > --- a/security/integrity/ima/ima_main.c
> > > > > > > > +++ b/security/integrity/ima/ima_main.c
> > > > > > > > @@ -696,18 +696,26 @@ void ima_post_create_tmpfile(struct mnt_idmap *idmap,
> > > > > > > >  /**
> > > > > > > >   * ima_post_path_mknod - mark as a new inode
> > > > > > > >   * @idmap: idmap of the mount the inode was found from
> > > > > > > > + * @dir: path structure of parent of the new file
> > > > > > > >   * @dentry: newly created dentry
> > > > > > > > + * @mode: mode of the new file
> > > > > > > > + * @dev: undecoded device number
> > > > > > > >   *
> > > > > > > >   * Mark files created via the mknodat syscall as new, so that the
> > > > > > > >   * file data can be written later.
> > > > > > > >   */
> > > > > > > >  void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > > > > -			 struct dentry *dentry)
> > > > > > > > +			 const struct path *dir, struct dentry *dentry,
> > > > > > > > +			 umode_t mode, unsigned int dev)
> > > > > > > >  {
> > > > > > > >  	struct integrity_iint_cache *iint;
> > > > > > > >  	struct inode *inode = dentry->d_inode;
> > > > > > > >  	int must_appraise;
> > > > > > > >  
> > > > > > > > +	/* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
> > > > > > > > +	if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
> > > > > > > > +		return;
> > > > > > > > +
> > > > > > > 
> > > > > > > There's already a check below to make sure that this is a regular file.
> > > > > > > Are both needed?
> > > > > > 
> > > > > > You are right, I can remove the first check.
> > > > > 
> > > > > The question then becomes why modify hook the arguments?   
> > > > 
> > > > We need to make sure that ima_post_path_mknod() has the same parameters
> > > > as the LSM hook at the time we register it to the LSM infrastructure.
> > > 
> > > I'm trying to understand why the pre hook parameters and the missing
> > > IMA parameter are used, as opposed to just defining the new
> > > post_path_mknod hook like IMA.
> > 
> > As an empyrical rule, I pass the same parameters as the corresponding
> > pre hook (plus idmap, in this case). This is similar to the
> > inode_setxattr hook. But I can be wrong, if desired I can reduce.
> 
> The inode_setxattr hook change example is legitimate, as EVM includes
> idmap, while IMA doesn't. 
> 
> Unless there is a good reason for the additional parameters, I'm not
> sure that adding them makes sense.  Not modifying the parameter list
> will reduce the size of this patch set.

The hook is going to be used by any LSM. Without knowing all the
possible use cases, maybe it is better to include more information now,
than modifying the hook and respective implementations later.

(again, no problem to reduce)

Thanks

Roberto
Mimi Zohar Oct. 12, 2023, 5:10 p.m. UTC | #10
> > > > > We need to make sure that ima_post_path_mknod() has the same parameters
> > > > > as the LSM hook at the time we register it to the LSM infrastructure.
> > > > 
> > > > I'm trying to understand why the pre hook parameters and the missing
> > > > IMA parameter are used, as opposed to just defining the new
> > > > post_path_mknod hook like IMA.
> > > 
> > > As an empyrical rule, I pass the same parameters as the corresponding
> > > pre hook (plus idmap, in this case). This is similar to the
> > > inode_setxattr hook. But I can be wrong, if desired I can reduce.
> > 
> > The inode_setxattr hook change example is legitimate, as EVM includes
> > idmap, while IMA doesn't. 
> > 
> > Unless there is a good reason for the additional parameters, I'm not
> > sure that adding them makes sense.  Not modifying the parameter list
> > will reduce the size of this patch set.
> 
> The hook is going to be used by any LSM. Without knowing all the
> possible use cases, maybe it is better to include more information now,
> than modifying the hook and respective implementations later.
> 
> (again, no problem to reduce)

Unless there is a known use case for a specific parameter, please
minimize them.   Additional parameters can be added later as needed.
Roberto Sassu Oct. 13, 2023, 7:38 a.m. UTC | #11
On Thu, 2023-10-12 at 13:10 -0400, Mimi Zohar wrote:
> > > > > > We need to make sure that ima_post_path_mknod() has the
> > > > > > same parameters
> > > > > > as the LSM hook at the time we register it to the LSM
> > > > > > infrastructure.
> > > > > 
> > > > > I'm trying to understand why the pre hook parameters and the
> > > > > missing
> > > > > IMA parameter are used, as opposed to just defining the new
> > > > > post_path_mknod hook like IMA.
> > > > 
> > > > As an empyrical rule, I pass the same parameters as the
> > > > corresponding
> > > > pre hook (plus idmap, in this case). This is similar to the
> > > > inode_setxattr hook. But I can be wrong, if desired I can
> > > > reduce.
> > > 
> > > The inode_setxattr hook change example is legitimate, as EVM
> > > includes
> > > idmap, while IMA doesn't. 
> > > 
> > > Unless there is a good reason for the additional parameters, I'm
> > > not
> > > sure that adding them makes sense.  Not modifying the parameter
> > > list
> > > will reduce the size of this patch set.
> > 
> > The hook is going to be used by any LSM. Without knowing all the
> > possible use cases, maybe it is better to include more information
> > now,
> > than modifying the hook and respective implementations later.
> > 
> > (again, no problem to reduce)
> 
> Unless there is a known use case for a specific parameter, please
> minimize them.   Additional parameters can be added later as needed. 

Ok. I did the same for inode_post_create_tmpfile.

Thanks

Roberto
diff mbox series

Patch

diff --git a/fs/namei.c b/fs/namei.c
index e56ff39a79bc..c5e96f716f98 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4024,6 +4024,7 @@  static int do_mknodat(int dfd, struct filename *name, umode_t mode,
 	struct path path;
 	int error;
 	unsigned int lookup_flags = 0;
+	umode_t mode_stripped;
 
 	error = may_mknod(mode);
 	if (error)
@@ -4034,8 +4035,9 @@  static int do_mknodat(int dfd, struct filename *name, umode_t mode,
 	if (IS_ERR(dentry))
 		goto out1;
 
-	error = security_path_mknod(&path, dentry,
-			mode_strip_umask(path.dentry->d_inode, mode), dev);
+	mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
+
+	error = security_path_mknod(&path, dentry, mode_stripped, dev);
 	if (error)
 		goto out2;
 
@@ -4045,7 +4047,8 @@  static int do_mknodat(int dfd, struct filename *name, umode_t mode,
 			error = vfs_create(idmap, path.dentry->d_inode,
 					   dentry, mode, true);
 			if (!error)
-				ima_post_path_mknod(idmap, dentry);
+				ima_post_path_mknod(idmap, &path, dentry,
+						    mode_stripped, dev);
 			break;
 		case S_IFCHR: case S_IFBLK:
 			error = vfs_mknod(idmap, path.dentry->d_inode,
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 910a2f11a906..179ce52013b2 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -32,7 +32,8 @@  extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
 extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 			      enum kernel_read_file_id id);
 extern void ima_post_path_mknod(struct mnt_idmap *idmap,
-				struct dentry *dentry);
+				const struct path *dir, struct dentry *dentry,
+				umode_t mode, unsigned int dev);
 extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
 extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
 extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
@@ -114,7 +115,9 @@  static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
 }
 
 static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
-				       struct dentry *dentry)
+				       const struct path *dir,
+				       struct dentry *dentry,
+				       umode_t mode, unsigned int dev)
 {
 	return;
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 365db0e43d7c..76eba92d7f10 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -696,18 +696,26 @@  void ima_post_create_tmpfile(struct mnt_idmap *idmap,
 /**
  * ima_post_path_mknod - mark as a new inode
  * @idmap: idmap of the mount the inode was found from
+ * @dir: path structure of parent of the new file
  * @dentry: newly created dentry
+ * @mode: mode of the new file
+ * @dev: undecoded device number
  *
  * Mark files created via the mknodat syscall as new, so that the
  * file data can be written later.
  */
 void ima_post_path_mknod(struct mnt_idmap *idmap,
-			 struct dentry *dentry)
+			 const struct path *dir, struct dentry *dentry,
+			 umode_t mode, unsigned int dev)
 {
 	struct integrity_iint_cache *iint;
 	struct inode *inode = dentry->d_inode;
 	int must_appraise;
 
+	/* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
+	if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
+		return;
+
 	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
 		return;