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?
--
paul-moore.com
