Re: [PATCH v5 06/13] ima: Mediate open/release method of the measurements list

On Wed, 2026-05-20 at 22:07 -0400, Mimi Zohar wrote:
> On Wed, 2026-04-29 at 18:03 +0200, Roberto Sassu wrote:
> > From: Roberto Sassu <[email protected]>
> > 
> > Introduce the ima_measure_users counter, to implement a semaphore-like
> > locking scheme where the binary and ASCII measurements list interfaces can
> > be concurrently open by multiple readers, or alternatively by a single
> > writer.
> > 
> > A semaphore cannot be used because the kernel cannot return to user space
> > with a lock held.
> > 
> > Introduce the ima_measure_lock() and ima_measure_unlock() primitives, to
> > respectively lock/unlock the interfaces (safely with the ima_measure_users
> > counter, without holding a lock).
> > 
> > Finally, introduce _ima_measurements_open() to lock the interface before
> > seq_open(), and call it from ima_measurements_open() and
> > ima_ascii_measurements_open(). And, introduce ima_measurements_release(),
> > to unlock the interface.
> > 
> > Require CAP_SYS_ADMIN if the interface is opened for write (not possible
> > for the current measurements interfaces, since they only have read
> > permission).
> > 
> > No functional changes: multiple readers are allowed as before.
> > 
> > Link: https://github.com/linux-integrity/linux/issues/1
> > Signed-off-by: Roberto Sassu <[email protected]>
> > ---
> >  security/integrity/ima/ima_fs.c | 71 +++++++++++++++++++++++++++++++--
> >  1 file changed, 67 insertions(+), 4 deletions(-)
> > 
> > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> > index 9a8dba14d82a..68edea7139d5 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -25,6 +25,8 @@
> >  #include "ima.h"
> >  
> >  static DEFINE_MUTEX(ima_write_mutex);
> > +static DEFINE_MUTEX(ima_measure_mutex);
> > +static long ima_measure_users;
> 
> long?

The limit pre process can be up to INT_MAX. Two processes could
overflow the counter if it was int.

Since privileged users can bypass the system wide max-file check (see
alloc_empty_file()), I will add an overflow check to be sure.

> >  
> >  bool ima_canonical_fmt;
> >  static int __init default_canonical_fmt_setup(char *str)
> > @@ -209,16 +211,76 @@ static const struct seq_operations ima_measurments_seqops = {
> >  	.show = ima_measurements_show
> >  };
> >  
> > +static int ima_measure_lock(bool write)
> > +{
> > +	mutex_lock(&ima_measure_mutex);
> > +	if ((write && ima_measure_users != 0) ||
> > +	    (!write && ima_measure_users < 0)) {
> > +		mutex_unlock(&ima_measure_mutex);
> > +		return -EBUSY;
> > +	}
> 
> Thanks, Roberto. The code is really clear and well written.  However, it could
> use a comment indicating the different ima_measure_users values as a reminder.
> 
> ima_measure_users:  > 0 open readers
> ima_meaasure_users: == -1 open writer

Ok.

> > +
> > +	if (write)
> > +		ima_measure_users--;
> > +	else
> > +		ima_measure_users++;
> > +	mutex_unlock(&ima_measure_mutex);
> > +	return 0;
> > +}
> > +
> > +static void ima_measure_unlock(bool write)
> > +{
> > +	mutex_lock(&ima_measure_mutex);
> > +	if (write)
> > +		ima_measure_users++;
> 
> There should only be one writer at a time. ima_measure_users could be set to
> zero.

Sure, but I find the code more clear this way.

Roberto

> > +	else
> > +		ima_measure_users--;
> > +	mutex_unlock(&ima_measure_mutex);
> > +}
> > +
> 
> thanks,
> 
> Mimi