Re: [PATCH RFC v4 1/1] ipe: add errno field to IPE policy load auditing

[Paul Moore]

On Mar  7, 2025 Jasjiv Singh <jasjivsingh@xxxxxxxxxxxxxxxxxxx> wrote:
> 
> Users of IPE require a way to identify when and why an operation fails,
> allowing them to both respond to violations of policy and be notified
> of potentially malicious actions on their systems with respect to IPE.
> 
> This patch introduces a new error field to the AUDIT_IPE_POLICY_LOAD event
> to log policy loading failures. Currently, IPE only logs successful policy
> loads, but not failures. Tracking failures is crucial to detect malicious
> attempts and ensure a complete audit trail for security events.
> 
> The new error field will capture the following error codes:
> 
> * -ENOKEY: Key used to sign the IPE policy not found in the keyring
> * -ESTALE: Attempting to update an IPE policy with an older version
> * -EKEYREJECTED: IPE signature verification failed
> * -ENOENT: Policy was deleted while updating
> * -EEXIST: Same name policy already deployed
> * -ERANGE: Policy version number overflow
> * -EINVAL: Policy version parsing error
> * -EPERM: Insufficient permission
> * -ENOMEM: Out of memory (OOM)
> * -EBADMSG: Policy is invalid
> 
> Here are some examples of the updated audit record types:
> 
> AUDIT_IPE_POLICY_LOAD(1422):
> audit:  AUDIT1422 policy_name="Test_Policy" policy_version=0.0.1
> policy_digest=sha256:84EFBA8FA71E62AE0A537FAB962F8A2BD1053964C4299DCA
> 92BFFF4DB82E86D3 auid=1000 ses=3 lsm=ipe res=1 errno=0
> 
> The above record shows a new policy has been successfully loaded into
> the kernel with the policy name, version, and hash with the errno=0.
> 
> AUDIT_IPE_POLICY_LOAD(1422) with error:
> 
> audit: AUDIT1422 policy_name=? policy_version=? policy_digest=?
> auid=1000 ses=3 lsm=ipe res=0 errno=-74
> 
> The above record shows a policy load failure due to an invalid policy
> (-EBADMSG).
> 
> Signed-off-by: Jasjiv Singh <jasjivsingh@xxxxxxxxxxxxxxxxxxx>
> ---
>  Documentation/admin-guide/LSM/ipe.rst | 69 +++++++++++++++++++--------
>  security/ipe/audit.c                  | 21 ++++++--
>  security/ipe/fs.c                     | 19 ++++++--
>  security/ipe/policy.c                 | 11 ++++-
>  security/ipe/policy_fs.c              | 29 ++++++++---
>  5 files changed, 111 insertions(+), 38 deletions(-)

...

> diff --git a/security/ipe/audit.c b/security/ipe/audit.c
> index f05f0caa4850..ac9d68b68b8b 100644
> --- a/security/ipe/audit.c
> +++ b/security/ipe/audit.c
> @@ -21,6 +21,8 @@
>  
>  #define AUDIT_POLICY_LOAD_FMT "policy_name=\"%s\" policy_version=%hu.%hu.%hu "\
>  			      "policy_digest=" IPE_AUDIT_HASH_ALG ":"
> +#define AUDIT_POLICY_LOAD_FAIL_FMT "policy_name=? policy_version=? "\
> +				   "policy_digest=?"

This should probably be AUDIT_POLICY_LOAD_NULL_FMT to be consistent with
the other IPE audit format macros, e.g. AUDIT_OLD_ACTIVE_POLICY_NULL_FMT.

> diff --git a/security/ipe/fs.c b/security/ipe/fs.c
> index 5b6d19fb844a..db18636470bf 100644
> --- a/security/ipe/fs.c
> +++ b/security/ipe/fs.c
> @@ -133,6 +133,8 @@ static ssize_t getenforce(struct file *f, char __user *data,
>   * * %-ERANGE			- Policy version number overflow
>   * * %-EINVAL			- Policy version parsing error
>   * * %-EEXIST			- Same name policy already deployed
> + * * %-ENOKEY			- Key used to sign the IPE policy not found in the keyring
> + * * %-EKEYREJECTED		- IPE signature verification failed
>   */
>  static ssize_t new_policy(struct file *f, const char __user *data,
>  			  size_t len, loff_t *offset)
> @@ -141,12 +143,17 @@ static ssize_t new_policy(struct file *f, const char __user *data,
>  	char *copy = NULL;
>  	int rc = 0;
>  
> -	if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN))
> -		return -EPERM;
> +	if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) {
> +		rc = -EPERM;
> +		goto out;
> +	}
>  
>  	copy = memdup_user_nul(data, len);
> -	if (IS_ERR(copy))
> -		return PTR_ERR(copy);
> +	if (IS_ERR(copy)) {
> +		rc = PTR_ERR(copy);
> +		copy = NULL;
> +		goto out;
> +	}
>  
>  	p = ipe_new_policy(NULL, 0, copy, len);
>  	if (IS_ERR(p)) {
> @@ -161,8 +168,10 @@ static ssize_t new_policy(struct file *f, const char __user *data,
>  	ipe_audit_policy_load(p);
>  
>  out:
> -	if (rc < 0)
> +	if (rc < 0) {
>  		ipe_free_policy(p);
> +		ipe_audit_policy_load(ERR_PTR(rc));
> +	}
>  	kfree(copy);
>  	return (rc < 0) ? rc : len;
>  }

I'm going to suggest putting the audit calls closer together to help
ease maintainence, e.g.:

  out:
    if (rc) {
	    ipe_audit_policy_load(ERR_PTR(rc));
	    ipe_free_policy(p);
    } else
	    ipe_audit_policy_load(p);

> diff --git a/security/ipe/policy_fs.c b/security/ipe/policy_fs.c
> index 3bcd8cbd09df..b70d2518b182 100644
> --- a/security/ipe/policy_fs.c
> +++ b/security/ipe/policy_fs.c
> @@ -292,21 +299,29 @@ static ssize_t update_policy(struct file *f, const char __user *data,
>  	char *copy = NULL;
>  	int rc = 0;
>  
> -	if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN))
> -		return -EPERM;
> +	if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN)) {
> +		rc = -EPERM;
> +		goto out;
> +	}
>  
>  	copy = memdup_user(data, len);
> -	if (IS_ERR(copy))
> -		return PTR_ERR(copy);
> +	if (IS_ERR(copy)) {
> +		rc = PTR_ERR(copy);
> +		copy = NULL;
> +		goto out;
> +	}
>  
>  	root = d_inode(f->f_path.dentry->d_parent);
>  	inode_lock(root);
>  	rc = ipe_update_policy(root, NULL, 0, copy, len);
>  	inode_unlock(root);
>  
> +out:
>  	kfree(copy);
> -	if (rc)
> +	if (rc) {
> +		ipe_audit_policy_load(ERR_PTR(rc));
>  		return rc;
> +	}
>  
>  	return len;
>  }

I don't really like how your auditing failure in one function and
success in a different function, that looks fragile.  Unfortunately,
I don't see a quick/easy fix for that right now so I guess this is
okay, but something to keep in mind for the future.

--
paul-moore.com