Re: [PATCH] ima: process_measurement() needlessly takes inode_lock() on MAY_READ

[Frederick Lawler]

On Tue, Mar 25, 2025 at 06:01:09PM +0100, Roberto Sassu wrote:
> On Tue, 2025-03-25 at 11:42 -0500, Frederick Lawler wrote:
> > 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@xxxxxxxxxxxxxx>
> > > > ---
> > > >   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.
> 
> Let's keep as it is for now.
> 
> > 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.
> 
> Yes, of course.
> 
> I meant, since in ima_rdwr_violation_check() there is:
> 
> 
> if (mode & FMODE_WRITE)
> ...
> else if (... && must_measure)
> 
> 
> which don't need to be under lock, then I would have modified
> violation_check to:
> 
> 	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK ||
> 			    func == MMAP_CHECK_REQPROT) &&
> 			   (ima_policy_flag & IMA_MEASURE)) &&
> 			   ((action & IMA_MEASURE) || (mask & MAY_WRITE));
>

Sounds good! I'll make the change and submit a v2.

> Roberto
> 
> > 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
>