On 3/5/2025 1:23 PM, Fan Wu wrote:
> On Tue, Mar 4, 2025 at 4:04 PM Jasjiv Singh
> <jasjivsingh@xxxxxxxxxxxxxxxxxxx> 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?
>
> I think the key here is we need to document the errors in the ipe's
> context. For example, ENOKEY means the key used to sign the ipe policy
> is not found in the keyring, EKEYREJECTED means ipe signature
> verification failed. If an error does not have specific meaning for
> ipe then probably we don't need to document it.
>
>>
>> For other functions: I have complied the errno list:
>>
>> * -ENOENT: Policy is not found while updating
>
> This one means policy was deleted while updating, this only happens
> the update happened just after the policy deletion.
>
>> * -EEXIST: Same name policy already deployed
>> * -ERANGE: Policy version number overflow
>> * -EINVAL: Policy version parsing error
>> * -EPERM: Insufficient permission
>> * -ESTALE: Policy version is old
>
> Maybe make this one clearer, how about trying to update an ipe policy
> with an older version policy.
>
> -Fan
Thanks, I have compiled the list based on your comments.
* -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
How does that that look? I will update the documentation with this list
in the next patch along with suggestions you mentioned.
Moving the memdup failure discussion here:
>>> 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.
I think we can just use "copy = NULL;" after recording the rc value from it,
instead of the suggested code. For examples, we can look at selinux.
-Jasjiv
