On Wed, Aug 14, 2024 at 2:23 PM Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> wrote:
> On 8/13/2024 6:53 PM, Paul Moore wrote:
> > On Tue, Aug 13, 2024 at 1:54 PM Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> wrote:
> >> On 8/10/2024 8:50 AM, Serge E. Hallyn wrote:
> >>> On Fri, Aug 02, 2024 at 11:08:16PM -0700, Fan Wu wrote:
> >>>> From: Deven Bowers <deven.desai@xxxxxxxxxxxxxxxxxxx>
> >>>>
> >>>> IPE's interpretation of the what the user trusts is accomplished through
> >>>
> >>> nit: "of what the user trusts" (drop the extra 'the')
> >>>
> >>>> its policy. IPE's design is to not provide support for a single trust
> >>>> provider, but to support multiple providers to enable the end-user to
> >>>> choose the best one to seek their needs.
> >>>>
> >>>> This requires the policy to be rather flexible and modular so that
> >>>> integrity providers, like fs-verity, dm-verity, or some other system,
> >>>> can plug into the policy with minimal code changes.
> >>>>
> >>>> Signed-off-by: Deven Bowers <deven.desai@xxxxxxxxxxxxxxxxxxx>
> >>>> Signed-off-by: Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx>
> >>>
> >>> This all looks fine. Just one comment below.
> >>>
> >> Thank you for reviewing this!
> >>
> >>>
> >>>> +/**
> >>>> + * parse_rule() - parse a policy rule line.
> >>>> + * @line: Supplies rule line to be parsed.
> >>>> + * @p: Supplies the partial parsed policy.
> >>>> + *
> >>>> + * Return:
> >>>> + * * 0 - Success
> >>>> + * * %-ENOMEM - Out of memory (OOM)
> >>>> + * * %-EBADMSG - Policy syntax error
> >>>> + */
> >>>> +static int parse_rule(char *line, struct ipe_parsed_policy *p)
> >>>> +{
> >>>> + enum ipe_action_type action = IPE_ACTION_INVALID;
> >>>> + enum ipe_op_type op = IPE_OP_INVALID;
> >>>> + bool is_default_rule = false;
> >>>> + struct ipe_rule *r = NULL;
> >>>> + bool first_token = true;
> >>>> + bool op_parsed = false;
> >>>> + int rc = 0;
> >>>> + char *t;
> >>>> +
> >>>> + r = kzalloc(sizeof(*r), GFP_KERNEL);
> >>>> + if (!r)
> >>>> + return -ENOMEM;
> >>>> +
> >>>> + INIT_LIST_HEAD(&r->next);
> >>>> + INIT_LIST_HEAD(&r->props);
> >>>> +
> >>>> + while (t = strsep(&line, IPE_POLICY_DELIM), line) {
> >>>
> >>> If line is passed in as NULL, t will be NULL on the first test. Then
> >>> you'll break out and call parse_action(NULL), which calls
> >>> match_token(NULL, ...), which I do not think is safe.
> >>>
> >>> I realize the current caller won't pass in NULL, but it seems worth
> >>> checking for here in case some future caller is added by someone
> >>> who's unaware.
> >>>
> >>> Or, maybe add 'line must not be null' to the function description.
> >>
> >> Yes, I agree that adding a NULL check would be better. I will include it
> >> in the next version.
> >
> > We're still waiting to hear back from the device-mapper devs, but if
> > this is the only change required to the patchset I can add a NULL
> > check when I merge the patchset as it seems silly to resend the entire
> > patchset for this. Fan, do you want to share the code snippet with
> > the NULL check so Serge can take a look?
>
> Sure, here is the diff.
>
> diff --git a/security/ipe/policy_parser.c b/security/ipe/policy_parser.c
> index 32064262348a..0926b442e32a 100644
> --- a/security/ipe/policy_parser.c
> +++ b/security/ipe/policy_parser.c
> @@ -309,6 +309,9 @@ static int parse_rule(char *line, struct
> ipe_parsed_policy *p)
> int rc = 0;
> char *t;
>
> + if (IS_ERR_OR_NULL(line))
> + return -EBADMSG;
> +
> r = kzalloc(sizeof(*r), GFP_KERNEL);
> if (!r)
> return -ENOMEM;
>
Thanks.
Serge, it looks like this should resolve your concern?
--
paul-moore.com
