On 3/4/2025 4:04 PM, Jasjiv Singh wrote:
>
>
> 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;
Another issue I was thinking about that is what if memdup works but then
ipe_new_policy fails, then we still need to call kfree but the above code
mentioned by you will not do that.
>>
>>> 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
>>>
>
