Message-ID: <CAHC9VhTF646GKPVQjS+bvdws2c8DjdR3jRX4Hc-vUjiF-b0mLA@mail.gmail.com>
Date: Tue, 28 Oct 2025 22:15:49 -0400
From: Paul Moore <paul@...l-moore.com>
To: Isaac Manjarres <isaacmanjarres@...gle.com>
Cc: Eric Paris <eparis@...hat.com>, surenb@...gle.com, aliceryhl@...gle.com, 
	tweek@...gle.com, kernel-team@...roid.com, audit@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] audit: Improve path logging for unlinked files

On Tue, Oct 28, 2025 at 9:22 PM Isaac Manjarres
<isaacmanjarres@...gle.com> wrote:
> On Tue, Oct 28, 2025 at 6:09 PM Paul Moore <paul@...l-moore.com> wrote:
> > On Tue, Oct 28, 2025 at 8:54 PM Isaac J. Manjarres
> > <isaacmanjarres@...gle.com> wrote:
> > >
> > > When logging the path associated with a denial, the path is scanned
> > > to ensure that it does not contain control characters, unprintable
> > > characters, double quote marks, or spaces. If it does, the hexadecimal
> > > representation of the path is emitted.
> > >
> > > This can make debugging difficult in scenarios where the file name that
> > > was given to the file does not contain any of those characters,
> > > but the hexadecimal representation of the path is still emitted when a
> > > denial occurs because the file is unlinked.
> > >
> > > For example, suppose a memfd is created with the name "MemoryHeapBase".
> > > Memfds are unlinked, so when a denial related to that memfd is
> > > encountered, and the the path name for it is obtained via d_path(), the
> > > name will be: "/memfd:MemoryHeapBase (deleted)". Since the name has a
> > > space, the audit logic will just print the hexadecimal representation
> > > instead of the name:
> > >
> > > type=1400 audit(0.0:319): avc:  denied  { read write } for
> > > path=2F6D656D66643A4D656D6F72794865617042617365202864656C6574656429
> > > dev="tmpfs" ino=75 scontext=u:r:audioserver:s0
> > > tcontext=u:object_r:system_server:s0 tclass=memfd_file permissive=0
> > >
> > > To improve debuggability of denials related to unlinked files, check
> > > if the " (deleted)" suffix is detected in a path name and remove it
> > > if so. This allows the actual filename to be validated and emitted
> > > if appropriate, making denials easier to read and more actionable:
> > >
> > > type=1400 audit(0.0:254): avc:  denied  { read write } for
> > > path="/memfd:MemoryHeapBase" dev="tmpfs" ino=67
> > > scontext=u:r:audioserver:s0 tcontext=u:object_r:system_server:s0
> > > tclass=memfd_file permissive=0
> > >
> > > Signed-off-by: Isaac J. Manjarres <isaacmanjarres@...gle.com>
> > > ---
> > >  kernel/audit.c | 16 ++++++++++++++--
> > >  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > I'd prefer not to add any additional string processing to the audit hot path.
>
> Hi Paul,
>
> Thank you for taking the time to look through the patch. Would it help
> if I could
> optimize the logic to scan for the " (deleted)" string from the end of
> the string
> so that the search is bounded by at most 10 characters? I can also switch to
> calling audit_log_n_untrustedstring() instead to avoid another call to strlen().
>
> If not, do you have any other suggestions to help make this improvement?

Since we don't know the length of the path string, I'm not sure
starting the search from the end would be a significant improvement as
you would still need to traverse the string to find the end, and then
do the search.  It is possible that searching from the end may be
slower than searching from the front, although as you point out, some
of this work could be reused through the use of
audit_log_n_untrustedstring().

However, we're still doing string processing/searching in a critical
path.  Audit is already suffering from far too much string processing
and I'm *extremely* hesitant to add any more unless there simply is no
other way.  As this is a "nice to have" sort of thing, and not a
critical bug fix, I'd strongly prefer not to do the extra work here.

As far as other suggestions are concerned, nothing comes immediately
to mind, I'm sorry.

--
paul-moore.com

Powered by blists - more mailing lists