Re: [PATCH 1/1] IMA event log trimming

[steven chen <chenste@linux.microsoft.com>]

On 12/5/2025 1:30 AM, Roberto Sassu wrote:
> On Tue, 2025-12-02 at 15:28 -0800, steven chen wrote:
>> This patch is for trimming N entries of the IMA event logs as well as
>> cleaning the hash table.
>>
>> It provides a userspace interface ima_trim_log that can be used to input
>> number N to let kernel to trim N entries of IMA event logs. When read
>> this interface, it returns number of entries trimmed last tim.
> High-level comments:
> - It does not offer the possibility to keep the hash table

Hi Roberto,

Should the hash table persist after log removal? Log trimming is 
infrequent, right?

Thanks,

Steven

> - There is no coordination between taking a snapshot and the readers of
>    the measurements list (I think it is necessary, since reading is
>    based on *pos, which contains the entries read until a given point;
>    if there is a truncate in the middle of the read, *pos would still
>    refer to the non-truncated list and the next read will skip some
>    measurement entries)

Hi Roberto,

What’s your suggestion for this? Leave user space handle this (check log 
trimming
happened)?

Thanks!

Steven.

> - While trimming per se is ok, I like more the idea of staging changes
>    and letting the user delete the staged measurements list later

Hi Roberto,

This approach simplifies both the user space agent and the kernel. The 
main difference
lies in whether log purging requires one step or two; I think a single 
step is preferable
since logs must be removed from the kernel regardless.

Thanks,

Steven

>> A mutex ima_trim_list_mutex is provided to allow one trimming request
>> at a time.
>>
>> Signed-off-by: steven chen <chenste@linux.microsoft.com>
>> ---
>>   security/integrity/ima/ima.h       |  2 +
>>   security/integrity/ima/ima_fs.c    | 78 ++++++++++++++++++++++++++++++
>>   security/integrity/ima/ima_queue.c | 42 ++++++++++++++++
>>   3 files changed, 122 insertions(+)
>>
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index e3d71d8d56e3..ab0e30ee25ea 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -246,8 +246,10 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
>>   
>>   #ifdef CONFIG_IMA_KEXEC
>>   void ima_measure_kexec_event(const char *event_name);
>> +long ima_purge_event_log(long number_logs);
>>   #else
>>   static inline void ima_measure_kexec_event(const char *event_name) {}
>> +static inline long ima_purge_event_log(long number_logs) { return 0; }
>>   #endif
>>   
>>   /*
>> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
>> index 87045b09f120..ea93448feedd 100644
>> --- a/security/integrity/ima/ima_fs.c
>> +++ b/security/integrity/ima/ima_fs.c
>> @@ -38,6 +38,11 @@ __setup("ima_canonical_fmt", default_canonical_fmt_setup);
>>   
>>   static int valid_policy = 1;
>>   
>> +#define IMA_LOG_TRIM_REQ_LENGTH 11
>> +static long trimcount;
>> +/* mutex protects atomicity of trimming measurement list requests */
>> +static DEFINE_MUTEX(ima_trim_list_mutex);
>> +
>>   static ssize_t ima_show_htable_value(char __user *buf, size_t count,
>>   				     loff_t *ppos, atomic_long_t *val)
>>   {
>> @@ -289,6 +294,69 @@ static const struct file_operations ima_ascii_measurements_ops = {
>>   	.release = seq_release,
>>   };
>>   
>> +static int ima_log_trim_open(struct inode *inode, struct file *filp)
>> +{
>> +	if (!capable(CAP_SYS_ADMIN))
>> +		return -EPERM;
>> +	return 0;
>> +}
>> +
>> +static ssize_t ima_log_trim_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
>> +{
>> +	char tmpbuf[IMA_LOG_TRIM_REQ_LENGTH];	/* greater than largest 'long' string value */
>> +	ssize_t len;
>> +
>> +	len = scnprintf(tmpbuf, sizeof(tmpbuf), "%li\n", trimcount);
>> +	return simple_read_from_buffer(buf, size, ppos, tmpbuf, len);
>> +}
>> +
>> +static ssize_t ima_log_trim_write(struct file *file,
>> +				  const char __user *buf, size_t datalen, loff_t *ppos)
>> +{
>> +	unsigned char req[IMA_LOG_TRIM_REQ_LENGTH];
>> +	long count, n;
>> +	int ret;
>> +
>> +	mutex_lock(&ima_trim_list_mutex);
>> +
>> +	if (*ppos > 0 || datalen > IMA_LOG_TRIM_REQ_LENGTH || datalen < 2) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	n = (int)datalen;
>> +
>> +	ret = copy_from_user(req, buf, datalen);
>> +	if (ret < 0)
>> +		goto out;
>> +
>> +	count = 0;
>> +	for (int i = 0; i < n; ++i) {
>> +		if (req[i] < '0' || req[i] > '9') {
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +		count = count * 10 + req[i] - '0';
>> +	}
>> +	ret = ima_purge_event_log(count);
>> +
>> +	if (ret < 0)
>> +		goto out;
>> +
>> +	trimcount = ret;
>> +	ret = datalen;
>> +out:
>> +	mutex_unlock(&ima_trim_list_mutex);
>> +	return ret;
>> +}
>> +
>> +static const struct file_operations ima_log_trim_ops = {
>> +	.open = ima_log_trim_open,
>> +	.read = ima_log_trim_read,
>> +	.write = ima_log_trim_write,
>> +	.llseek = generic_file_llseek,
>> +};
>> +
>>   static ssize_t ima_read_policy(char *path)
>>   {
>>   	void *data = NULL;
>> @@ -528,6 +596,16 @@ int __init ima_fs_init(void)
>>   		goto out;
>>   	}
>>   
>> +	dentry = securityfs_create_file("ima_trim_log",
>> +					S_IRUSR | S_IRGRP | S_IWUSR | S_IWGRP,
>> +					ima_dir, NULL, &ima_log_trim_ops);
>> +	if (IS_ERR(dentry)) {
>> +		ret = PTR_ERR(dentry);
>> +		goto out;
>> +	}
>> +
>> +	trimcount = 0;
>> +
>>   	dentry = securityfs_create_file("runtime_measurements_count",
>>   				   S_IRUSR | S_IRGRP, ima_dir, NULL,
>>   				   &ima_measurements_count_ops);
>> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
>> index 590637e81ad1..999cd42c517c 100644
>> --- a/security/integrity/ima/ima_queue.c
>> +++ b/security/integrity/ima/ima_queue.c
>> @@ -220,6 +220,48 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>>   	return result;
>>   }
>>   
>> +/* Delete the IMA event logs */
>> +long ima_purge_event_log(long number_logs)
>> +{
>> +	struct ima_queue_entry *qe;
>> +	long cur = 0;
>> +
>> +	if (number_logs <= 0)
>> +		return number_logs;
>> +
>> +	mutex_lock(&ima_extend_list_mutex);
>> +	rcu_read_lock();
> Sorry, I'm missing why rcu_read_lock() is needed.

Hi Roberto,

I will add "true" parameter in list_for_each_entry_rcu api.

Thanks,

Steven

>> +
>> +	/*
>> +	 * Remove this entry from both hash table and the measurement list
>> +	 * When removing from hash table, decrease the length counter
>> +	 * so that the hash table re-sizing logic works correctly
>> +	 */
>> +	list_for_each_entry_rcu(qe, &ima_measurements, later) {
>> +		int i;
>> +
>> +		/* if CONFIG_IMA_DISABLE_HTABLE is set, the hash table is not used */
>> +		if (!IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE))
>> +			hlist_del_rcu(&qe->hnext);
>> +
>> +		for (i = 0; i < qe->entry->template_desc->num_fields; i++) {
>> +			kfree(qe->entry->template_data[i].data);
>> +			qe->entry->template_data[i].data = NULL;
>> +			qe->entry->template_data[i].len = 0;
>> +		}
>> +
>> +		atomic_long_dec(&ima_htable.len);
>> +		list_del_rcu(&qe->later);
> Missing kfree() of qe->entry and qe?
>
> Thanks
>
> Roberto

Hi Roberto,

I will update in next version.

Thanks,

Steven

>> +		++cur;
>> +		if (cur >= number_logs)
>> +			break;
>> +	}
>> +
>> +	rcu_read_unlock();
>> +	mutex_unlock(&ima_extend_list_mutex);
>> +	return cur;
>> +}
>> +
>>   int ima_restore_measurement_entry(struct ima_template_entry *entry)
>>   {
>>   	int result = 0;