Re: [PATCH v3] ipe: add errno field to IPE policy load auditing

[Jasjiv Singh <jasjivsingh@linux.microsoft.com>]

On 3/5/2025 1:23 PM, Fan Wu wrote:
> On Tue, Mar 4, 2025 at 4:04 PM Jasjiv Singh
> <jasjivsingh@linux.microsoft.com> wrote:
>>
>>
>>
>> On 3/3/2025 2:11 PM, Fan Wu wrote:
>>> On Fri, Feb 28, 2025 at 3:11 PM Jasjiv Singh
>>> <jasjivsingh@linux.microsoft.com> 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