linux-kernel - Re: [PATCH v2 1/7] fs: ima: Remove S_IMA and IS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241128114059.h7hrq6sma2ckyrjn@quack3>
Date: Thu, 28 Nov 2024 12:40:59 +0100
From: Jan Kara <jack@...e.cz>
To: Roberto Sassu <roberto.sassu@...weicloud.com>
Cc: viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz,
	zohar@...ux.ibm.com, dmitry.kasatkin@...il.com,
	eric.snowberg@...cle.com, paul@...l-moore.com, jmorris@...ei.org,
	serge@...lyn.com, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-integrity@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	Roberto Sassu <roberto.sassu@...wei.com>,
	Shu Han <ebpqwerty472123@...il.com>
Subject: Re: [PATCH v2 1/7] fs: ima: Remove S_IMA and IS_IMA()

On Thu 28-11-24 11:06:14, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@...wei.com>
> 
> Commit 196f518128d2e ("IMA: explicit IMA i_flag to remove global lock on
> inode_delete") introduced the new S_IMA inode flag to determine whether or
> not an inode was processed by IMA. In that way, it was not necessary to
> take the global lock on inode delete.
> 
> Since commit 4de2f084fbff ("ima: Make it independent from 'integrity'
> LSM"), the pointer of the inode integrity metadata managed by IMA has been
> moved to the inode security blob, from the rb-tree. The pointer is not NULL
> only if the inode has been processed by IMA, i.e. ima_inode_get() has been
> called for that inode.
> 
> Thus, since the IS_IMA() check can be now implemented by trivially testing
> whether or not the pointer of inode integrity metadata is NULL, remove the
> S_IMA definition in include/linux/fs.h and also the IS_IMA() macro.
> 
> Remove also the IS_IMA() invocation in ima_rdwr_violation_check(), since
> whether the inode was processed by IMA will be anyway detected by a
> subsequent call to ima_iint_find(). It does not have an additional overhead
> since the decision can be made in constant time, as opposed to logarithm
> when the inode integrity metadata was stored in the rb-tree.
> 
> Suggested-by: Shu Han <ebpqwerty472123@...il.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
> ---
>  include/linux/fs.h                | 3 +--
>  security/integrity/ima/ima_iint.c | 5 -----
>  security/integrity/ima/ima_main.c | 2 +-
>  3 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3559446279c1..b33363becbdd 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2261,7 +2261,7 @@ struct super_operations {
>  #define S_NOCMTIME	(1 << 7)  /* Do not update file c/mtime */
>  #define S_SWAPFILE	(1 << 8)  /* Do not truncate: swapon got its bmaps */
>  #define S_PRIVATE	(1 << 9)  /* Inode is fs-internal */
> -#define S_IMA		(1 << 10) /* Inode has an associated IMA struct */
> +/* #define S_IMA	(1 << 10) Inode has an associated IMA struct (unused) */

Well, I guess you can just delete this line. These are internal kernel
flags so we can do whatever we want with them. Otherwise the patch looks
good. Feel free to add:

Acked-by: Jan Kara <jack@...e.cz>

								Honza

>  #define S_AUTOMOUNT	(1 << 11) /* Automount/referral quasi-directory */
>  #define S_NOSEC		(1 << 12) /* no suid or xattr security attributes */
>  #ifdef CONFIG_FS_DAX
> @@ -2319,7 +2319,6 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags
>  #endif
>  
>  #define IS_PRIVATE(inode)	((inode)->i_flags & S_PRIVATE)
> -#define IS_IMA(inode)		((inode)->i_flags & S_IMA)
>  #define IS_AUTOMOUNT(inode)	((inode)->i_flags & S_AUTOMOUNT)
>  #define IS_NOSEC(inode)		((inode)->i_flags & S_NOSEC)
>  #define IS_DAX(inode)		((inode)->i_flags & S_DAX)
> diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
> index 00b249101f98..9d9fc7a911ad 100644
> --- a/security/integrity/ima/ima_iint.c
> +++ b/security/integrity/ima/ima_iint.c
> @@ -26,9 +26,6 @@ static struct kmem_cache *ima_iint_cache __ro_after_init;
>   */
>  struct ima_iint_cache *ima_iint_find(struct inode *inode)
>  {
> -	if (!IS_IMA(inode))
> -		return NULL;
> -
>  	return ima_inode_get_iint(inode);
>  }
>  
> @@ -102,7 +99,6 @@ struct ima_iint_cache *ima_inode_get(struct inode *inode)
>  
>  	ima_iint_init_always(iint, inode);
>  
> -	inode->i_flags |= S_IMA;
>  	ima_inode_set_iint(inode, iint);
>  
>  	return iint;
> @@ -118,7 +114,6 @@ void ima_inode_free_rcu(void *inode_security)
>  {
>  	struct ima_iint_cache **iint_p = inode_security + ima_blob_sizes.lbs_inode;
>  
> -	/* *iint_p should be NULL if !IS_IMA(inode) */
>  	if (*iint_p)
>  		ima_iint_free(*iint_p);
>  }
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 06132cf47016..cea0afbbc28d 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -126,7 +126,7 @@ static void ima_rdwr_violation_check(struct file *file,
>  	bool send_tomtou = false, send_writers = false;
>  
>  	if (mode & FMODE_WRITE) {
> -		if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) {
> +		if (atomic_read(&inode->i_readcount)) {
>  			if (!iint)
>  				iint = ima_iint_find(inode);
>  			/* IMA_MEASURE is set from reader side */
> -- 
> 2.47.0.118.gfd3785337b
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR