On Wed, Mar 5, 2025 at 3:27 PM Jasjiv Singh
<jasjivsingh@xxxxxxxxxxxxxxxxxxx> wrote:
>
>
>
> 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.
>
This looks good to me, make sure to also update the function
documentations in the code.
>
> 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
>
Yep this makes sense to me. We can just add "copy = NULL" and still
use only one out: tag.
-Fan
