Re: [PATCH v1] audit: fix suffixed '/' filename matching in __audit_inode_child()

From: Paul Moore
Date: Wed Nov 13 2024 - 22:24:10 EST


On November 13, 2024 6:04:27 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
On Mon, Nov 11, 2024 at 05:06:43PM -0500, Paul Moore wrote:

This is completely untested, I didn't even compile it, but what about
something like the following? We do add an extra strlen(), but that is
going to be faster than the alloc/copy.

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 470041c49a44..c30c2ee9fb77 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1320,10 +1320,13 @@ int audit_compare_dname_path(const struct qstr *dname, const char *path, int par
return 1;

parentlen = parentlen == AUDIT_NAME_FULL ? parent_len(path) : parentlen;
- if (pathlen - parentlen != dlen)
- return 1;
-
p = path + parentlen;
+ pathlen = strlen(p);

Huh? We have
pathlen = strlen(path);
several lines prior to this. So unless parentlen + path manages to exceed
strlen(path) (in which case your strlen() is really asking for trouble),
this is simply
pathlen -= parentlen;

What am I missing here?

[NOTE: network access is patchy right now so you're getting a phone reply without an opportunity to look more closely at the code]

To be fair, this was a quick example of "do something like this" to demonstrate a different idea than was proposed in the original patch. The bit of code I shared was not a fully baked patch; I thought that would have been clear from the context, if not my comments.

Of course improvements are welcome, but in the future know that unless I'm submitting a proper patch, the code snippets I share during review are going to be *rough* and not fully developed. Additional work by the original author is expected.

And while we are at it,
parentlen = parentlen == AUDIT_NAME_FULL ? parent_len(path) : parentlen;
is a bloody awful way to spell
if (parentlen == AUDIT_NAME_FULL)
parentlen = parent_len(path);
What's more, parent_len(path) starts with *yet* *another* strlen(path),
followed by really awful crap - we trim the trailing slashes (if any),
then search for the last slash before that... is that really worth
the chance to skip that strncmp()?

Pretty much all of the audit code is awkward at best Al, you should know that. We're not going to fix it all in one patch, and considering the nature of this patch effort, I think there is going to be a lot of value in keeping the initial fix patch to a minimum to ease backporting. We can improve on some of those other issues in a second patch or at a later time.

As a reminder to everyone, patches are always welcome. Fixing things is a great way to channel disgust into something much more useful.


+ if (p[pathlen - 1] == '/')
+ pathlen--;
+
+ if (pathlen != dlen)
+ return 1;

return strncmp(p, dname->name, dlen);

... which really should've been memcmp(), at that.

Agreed. See above.

--
paul-moore.com