On 3/3/2025 2:11 PM, Fan Wu wrote:
> 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.
>
> ...
>
So, I looked into error codes when the policy loads. In ipe_new_policy,
the verify_pkcs7_signature can return a lot of errno codes (ex: ENOKEY,
EKEYREJECTED, EBADMSG, etc.) while parsing the pkcs7 and other functions
as well. Also, In ipe_new_policyfs_node used in new_policy(), I see the same
issue with securityfs_create_dir and securityfs_create_file as they
return the errno directly from API to. So, what should we return?
For other functions: I have complied the errno list:
* -ENOENT: Policy is not found while updating
* -EEXIST: Same name policy already deployed
* -ERANGE: Policy version number overflow
* -EINVAL: Policy version parsing error
* -EPERM: Insufficient permission
* -ESTALE: Policy version is old
* -ENOMEM: Out of memory (OOM)
* -EBADMSG: Policy is invalid
- Jasjiv
>>
>> 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
>>
