Re: [PATCH v3 02/25] ima: Align ima_post_path_mknod() definition with LSM infrastructure
From: Roberto Sassu
Date: Thu Oct 12 2023 - 09:34:37 EST
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@xxxxxxxxxx>
> > > > > > > >
> > > > > > > > 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@xxxxxxxxxx>
> > > > > > > > ---
> > > > > > > > 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