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
- 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)
- While trimming per se is ok, I like more the idea of staging changes
  and letting the user delete the staged measurements list later

> A mutex ima_trim_list_mutex is provided to allow one trimming request
> at a time.
> 
> Signed-off-by: steven chen <[email protected]>
> ---
>  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.

> +
> +     /*
> +      * 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

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


Reply via email to