Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

On Thu, Aug 2, 2018 at 7:45 AM Ondrej Mosnacek <omosnace(a)redhat.com&gt; wrote: > > When a relative path has just a single component and we want to emit a > nametype=PARENT record, the current implementation just reports the full > CWD path (which is alrady available in the audit context). > > This is wrong for three reasons: > 1. Wasting log space for redundant data (CWD path is already in the CWD > record). > 2. Inconsistency with other PATH records (if a relative PARENT directory > path contains at least one component, only the verbatim relative path > is logged). > 3. In some syscalls (e.g. openat(2)) the relative path may not even be > relative to the CWD, but to another directory specified as a file > descriptor. In that case the logged path is simply plain wrong. > > This patch modifies this behavior to simply report "." in the > aforementioned case, which is equivalent to an "empty" directory path > and can be concatenated with the actual base directory path (CWD or > dirfd from openat(2)-like syscall) once support for its logging is added > later. In the meantime, defaulting to CWD as base directory on relative > paths (as already done by the userspace tools) will be enough to achieve > results equivalent to the current behavior. > > See: https://github.com/linux-audit/audit-kernel/issues/95 > > Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events") > Signed-off-by: Ondrej Mosnacek <omosnace(a)redhat.com&gt; > --- > kernel/audit.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/kernel/audit.c b/kernel/audit.c > index 2a8058764aa6..4f18bd48eb4b 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -2127,28 +2127,27 @@ void audit_log_name(struct audit_context *context, struct audit_names *n, > > audit_log_format(ab, "item=%d", record_num); > > + audit_log_format(ab, " name="); > if (path) > - audit_log_d_path(ab, " name=", path); > + audit_log_d_path(ab, NULL, path); > else if (n->name) { > switch (n->name_len) { > case AUDIT_NAME_FULL: > /* log the full path */ > - audit_log_format(ab, " name="); > audit_log_untrustedstring(ab, n->name->name); > break; > case 0: > /* name was specified as a relative path and the > * directory component is the cwd */ > - audit_log_d_path(ab, " name=", &context->pwd); > + audit_log_untrustedstring(ab, "."); This isn't a comprehensive review, I only gave this a quick look, but considering you are only logging "." above we can safely use audit_log_string() and safe a few cycles.

Honestly, looking at the rest of this function, why are we using audit_log_format() in the case where we are simply writing a string literal? While I haven't tested it, simple code inspection would seem to indicate that audit_log_string() should be much quicker than audit_log_format()? I realize this isn't strictly a problem from this patch, but we probably shouldn't be propagating this mistake any further.