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

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

On 8/13/2024 6:53 PM, Paul Moore wrote:
> On Tue, Aug 13, 2024 at 1:54 PM Fan Wu <wufan@linux.microsoft.com> 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@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.
> 
> 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;

-Fan