On Fri, Feb 28, 2025 at 3:11 PM 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:
>
> * 0: no error
> * -EPERM: Insufficient permission
> * -EEXIST: Same name policy already deployed
> * -EBADMSG: policy is invalid
> * -ENOMEM: out of memory (OOM)
> * -ERANGE: policy version number overflow
> * -EINVAL: policy version parsing error
>
These error codes are not exhaustive. We recently introduced the
secondary keyring and platform keyring to sign policy so the policy
loading could return -ENOKEY or -EKEYREJECT. And also the update
policy can return -ESTALE when the policy version is old.
This is my fault that I forgot we should also update the documentation
of the newly introduced error codes. Could you please go through the
whole loading code and find all possible error codes? Also this is a
good chance to update the current stale function documents.
...
>
> Signed-off-by: Jasjiv Singh <jasjivsingh@xxxxxxxxxxxxxxxxxxx>
> ---
> Documentation/admin-guide/LSM/ipe.rst | 19 ++++++++++++++-----
> security/ipe/audit.c | 15 ++++++++++++---
> security/ipe/fs.c | 16 +++++++++++-----
> security/ipe/policy_fs.c | 18 +++++++++++++-----
> 4 files changed, 50 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/admin-guide/LSM/ipe.rst b/Documentation/admin-guide/LSM/ipe.rst
> index f93a467db628..5dbf54471fab 100644
> --- a/Documentation/admin-guide/LSM/ipe.rst
> +++ b/Documentation/admin-guide/LSM/ipe.rst
> @@ -423,7 +423,7 @@ Field descriptions:
>
> Event Example::
>
> - type=1422 audit(1653425529.927:53): policy_name="boot_verified" policy_version=0.0.0 policy_digest=sha256:820EEA5B40CA42B51F68962354BA083122A20BB846F26765076DD8EED7B8F4DB auid=4294967295 ses=4294967295 lsm=ipe res=1
> + type=1422 audit(1653425529.927:53): policy_name="boot_verified" policy_version=0.0.0 policy_digest=sha256:820EEA5B40CA42B51F68962354BA083122A20BB846F26765076DD8EED7B8F4DB auid=4294967295 ses=4294967295 lsm=ipe res=1 errno=0
> type=1300 audit(1653425529.927:53): arch=c000003e syscall=1 success=yes exit=2567 a0=3 a1=5596fcae1fb0 a2=a07 a3=2 items=0 ppid=184 pid=229 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=4294967295 comm="python3" exe="/usr/bin/python3.10" key=(null)
> type=1327 audit(1653425529.927:53): PROCTITLE proctitle=707974686F6E3300746573742F6D61696E2E7079002D66002E2E
>
> @@ -436,11 +436,11 @@ Field descriptions:
> +----------------+------------+-----------+---------------------------------------------------+
> | Field | Value Type | Optional? | Description of Value |
> +================+============+===========+===================================================+
> -| policy_name | string | No | The policy_name |
> +| policy_name | string | Yes | The policy_name |
> +----------------+------------+-----------+---------------------------------------------------+
> -| policy_version | string | No | The policy_version |
> +| policy_version | string | Yes | The policy_version |
> +----------------+------------+-----------+---------------------------------------------------+
> -| policy_digest | string | No | The policy hash |
> +| policy_digest | string | Yes | The policy hash |
> +----------------+------------+-----------+---------------------------------------------------+
> | auid | integer | No | The login user ID |
> +----------------+------------+-----------+---------------------------------------------------+
> @@ -450,7 +450,16 @@ Field descriptions:
> +----------------+------------+-----------+---------------------------------------------------+
> | res | integer | No | The result of the audited operation(success/fail) |
> +----------------+------------+-----------+---------------------------------------------------+
> -
> +| errno | integer | No | The result of the policy error as follows: |
> +| | | | |
> +| | | | + 0: no error |
> +| | | | + -EPERM: Insufficient permission |
> +| | | | + -EEXIST: Same name policy already deployed |
> +| | | | + -EBADMSG: policy is invalid |
> +| | | | + -ENOMEM: out of memory (OOM) |
> +| | | | + -ERANGE: policy version number overflow |
> +| | | | + -EINVAL: policy version parsing error |
> ++----------------+------------+-----------+---------------------------------------------------+
>
Might be better to create another table to list all potential erronos.
Also please keep the capitalization of sentences consistent.
> 1404 AUDIT_MAC_STATUS
> ^^^^^^^^^^^^^^^^^^^^^
> diff --git a/security/ipe/audit.c b/security/ipe/audit.c
> index f05f0caa4850..8df307bb2bab 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=?"
> #define AUDIT_OLD_ACTIVE_POLICY_FMT "old_active_pol_name=\"%s\" "\
> "old_active_pol_version=%hu.%hu.%hu "\
> "old_policy_digest=" IPE_AUDIT_HASH_ALG ":"
> @@ -254,16 +256,23 @@ void ipe_audit_policy_activation(const struct ipe_policy *const op,
> void ipe_audit_policy_load(const struct ipe_policy *const p)
> {
The documentation of this function should also be updated since it is
also auditing errors now.
> struct audit_buffer *ab;
> + int err = 0;
>
> ab = audit_log_start(audit_context(), GFP_KERNEL,
> AUDIT_IPE_POLICY_LOAD);
> if (!ab)
> return;
>
> - audit_policy(ab, AUDIT_POLICY_LOAD_FMT, p);
> - audit_log_format(ab, " auid=%u ses=%u lsm=ipe res=1",
> + if (!IS_ERR(p)) {
> + audit_policy(ab, AUDIT_POLICY_LOAD_FMT, p);
> + } else {
> + audit_log_format(ab, AUDIT_POLICY_LOAD_FAIL_FMT);
> + err = PTR_ERR(p);
> + }
> +
> + audit_log_format(ab, " auid=%u ses=%u lsm=ipe res=%d errno=%d",
> from_kuid(&init_user_ns, audit_get_loginuid(current)),
> - audit_get_sessionid(current));
> + audit_get_sessionid(current), !err, err);
>
> audit_log_end(ab);
> }
> diff --git a/security/ipe/fs.c b/security/ipe/fs.c
> index 5b6d19fb844a..da51264a1d0f 100644
> --- a/security/ipe/fs.c
> +++ b/security/ipe/fs.c
> @@ -141,12 +141,16 @@ 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);
> + goto out;
> + }
>
> p = ipe_new_policy(NULL, 0, copy, len);
> if (IS_ERR(p)) {
> @@ -161,8 +165,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;
> }
In case of memdup fail, the kfree(copy) will be called with the error
pointer. Also how about refactor the code like
ipe_audit_policy_load(p);
kfree(copy);
return len;
err:
ipe_audit_policy_load(ERR_PTR(rc));
ipe_free_policy(p);
return rc;
> diff --git a/security/ipe/policy_fs.c b/security/ipe/policy_fs.c
> index 3bcd8cbd09df..5f4a8e92bdcf 100644
> --- a/security/ipe/policy_fs.c
> +++ b/security/ipe/policy_fs.c
> @@ -12,6 +12,7 @@
> #include "policy.h"
> #include "eval.h"
> #include "fs.h"
> +#include "audit.h"
>
> #define MAX_VERSION_SIZE ARRAY_SIZE("65535.65535.65535")
>
> @@ -292,21 +293,28 @@ 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);
> + 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;
> + }
>
The above comments also apply to here.
-Fan
> return len;
> }
> --
> 2.34.1
>
