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

[Fan Wu <wufan@linux.microsoft.com>]

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