Message-ID: <CAKtyLkGUrf1txbKo2QWLn7vJ2jc=Zu3NYQPWiBh9h5fU8FT_nA@mail.gmail.com>
Date: Wed, 5 Mar 2025 16:08:00 -0800
From: Fan Wu <wufan@...nel.org>
To: Jasjiv Singh <jasjivsingh@...ux.microsoft.com>
Cc: Fan Wu <wufan@...nel.org>, audit@...r.kernel.org, corbet@....net, 
	eparis@...hat.com, jmorris@...ei.org, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org, 
	paul@...l-moore.com
Subject: Re: [PATCH v3] ipe: add errno field to IPE policy load auditing

On Wed, Mar 5, 2025 at 3:27 PM Jasjiv Singh
<jasjivsingh@...ux.microsoft.com> wrote:
>
>
>
> On 3/5/2025 1:23 PM, Fan Wu wrote:
> > On Tue, Mar 4, 2025 at 4:04 PM Jasjiv Singh
> > <jasjivsingh@...ux.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@...ux.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.
>

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

Powered by blists - more mailing lists