linux-kernel - Re: [PATCH] ima: process_measurement() needlessly takes inode_lock() on MAY

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z-Lc5WxW7NRA6AiT@CMGLRV3>
Date: Tue, 25 Mar 2025 11:42:13 -0500
From: Frederick Lawler <fred@...udflare.com>
To: Roberto Sassu <roberto.sassu@...weicloud.com>
Cc: Mimi Zohar <zohar@...ux.ibm.com>,
	Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
	Roberto Sassu <roberto.sassu@...wei.com>,
	Eric Snowberg <eric.snowberg@...cle.com>,
	James Morris <james.l.morris@...cle.com>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	linux-ima-devel@...ts.sourceforge.net,
	linux-ima-user@...ts.sourceforge.net,
	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-team@...udflare.com
Subject: Re: [PATCH] ima: process_measurement() needlessly takes inode_lock()
 on MAY_READ

On Tue, Mar 25, 2025 at 05:30:32PM +0100, Roberto Sassu wrote:
> On 3/25/2025 4:58 PM, Frederick Lawler wrote:
> > On IMA policy update, if a measure rule exists in the policy,
> > IMA_MEASURE is set for ima_policy_flags which makes the violation_check
> > variable always true. Coupled with a no-action on MAY_READ for a
> > FILE_CHECK call, we're always taking the inode_lock().
> > 
> > This becomes a performance problem for extremely heavy read-only workloads.
> > Therefore, prevent this only in the case there's no action to be taken.
> > 
> > Signed-off-by: Frederick Lawler <fred@...udflare.com>
> > ---
> >   security/integrity/ima/ima_main.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 2aebb7984437..78921e69ee14 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -181,7 +181,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> >   	action = ima_get_action(inode, mask, func, &pcr);
> >   	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
> >   			   (ima_policy_flag & IMA_MEASURE));
> > -	if (!action && !violation_check)
> > +	if (!action && (mask == MAY_READ || !violation_check))
> >   		return 0;
> 

Hi Roberto,

> Hi Frederick
> 
> thanks, nice catch!
> 
> Thinking... in fact you are saying that there are conditions for which
> ima_rdwr_violation_check() does nothing.
> 
> For better clarity, I would add the conditions for which we are doing a
> violation check in violation_check directly. So that, one can just go to the
> function and see that in fact nothing special is done other than doing the
> same checks in advance before taking the lock (the conditions you are
> checking on are immutable, so it is fine).
> 
> So, it is not a write, and the file is not being measured (this would be a
> bit redundant given that we are checking anyway !action).
> 
> Thanks
>

The ima_rdwr_violation_check() call takes a action & IMA_MEASURE
argument anyway.

My initial thought was to replace ima_flag_policy & IMA_MEASURE with
action & IMA_MEASURE there, but I wasn't sure if there was a race
problem that the ima_rdwr_violation_check() is trying to catch for the non
FILE_CHECK cases.

Otherwise, I think the checks in the ima_rdwr_violation_check() demand the lock,
and therefore we can't just move them out to that violation_check
variable--unless I'm missing something. As for other conditions, I think
it's _just_ the MAY_READ we care about.

Is what you're suggesting to move the check mask == MAY_READ to instead be in
that violation_check variable than the branch?

> Roberto
> 
> >   	must_appraise = action & IMA_APPRAISE;
> 

Thanks,
Fred