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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e1dd4dcf-8e2e-4e7b-9d40-533efd123103@linux.microsoft.com>
Date: Tue, 13 Aug 2024 10:54:29 -0700
From: Fan Wu <wufan@...ux.microsoft.com>
To: "Serge E. Hallyn" <serge@...lyn.com>
Cc: corbet@....net, zohar@...ux.ibm.com, jmorris@...ei.org, tytso@....edu,
 ebiggers@...nel.org, axboe@...nel.dk, agk@...hat.com, snitzer@...nel.org,
 mpatocka@...hat.com, eparis@...hat.com, paul@...l-moore.com,
 linux-doc@...r.kernel.org, linux-integrity@...r.kernel.org,
 linux-security-module@...r.kernel.org, fsverity@...ts.linux.dev,
 linux-block@...r.kernel.org, dm-devel@...ts.linux.dev,
 audit@...r.kernel.org, linux-kernel@...r.kernel.org,
 Deven Bowers <deven.desai@...ux.microsoft.com>
Subject: Re: [PATCH v20 02/20] ipe: add policy parser



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