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

[Mimi Zohar <zohar@linux.ibm.com>]

On Wed, 2026-04-29 at 18:03 +0200, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> 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 <roberto.sassu@huawei.com>
> ---
>  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?

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

> +
> +	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.

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

thanks,

Mimi