Re: [PATCH v20 02/20] ipe: add policy parser

Fan Wu
2024-08-13T17:54:36Z

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@linux.microsoft.com>
>>
>> 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@linux.microsoft.com>
>> Signed-off-by: Fan Wu <wufan@linux.microsoft.com>
>
> 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.

-Fan