On 8/13/2024 6:53 PM, Paul Moore wrote:

On Tue, Aug 13, 2024 at 1:54 PM Fan Wu [off-list ref] wrote:

quoted

On 8/10/2024 8:50 AM, Serge E. Hallyn wrote:

quoted

On Fri, Aug 02, 2024 at 11:08:16PM -0700, Fan Wu wrote:

quoted

From: Deven Bowers <redacted>

IPE's interpretation of the what the user trusts is accomplished through

nit: "of what the user trusts" (drop the extra 'the')

quoted

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 <redacted>
Signed-off-by: Fan Wu <redacted>

This all looks fine.  Just one comment below.

Thank you for reviewing this!

quoted

quoted

+/**
+ * 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