linux-kernel - Re: [PATCH 2/2] tracefs: Only clobber mode/uid/gid on remount if asked

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YxkEfzVusTXj9JST@google.com>
Date:   Wed, 7 Sep 2022 13:52:15 -0700
From:   Brian Norris <briannorris@...omium.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] tracefs: Only clobber mode/uid/gid on remount if
 asked

Hi Steven,

On Wed, Sep 07, 2022 at 07:44:43AM -0400, Steven Rostedt wrote:
> On Fri, 26 Aug 2022 17:44:17 -0700
> Brian Norris <briannorris@...omium.org> wrote:
> 
> > Users may have explicitly configured their tracefs permissions; we
> > shouldn't overwrite those just because a second mount appeared.
> > 
> > Only clobber if the options were provided at mount time.
> > 
> > Note: the previous behavior was especially surprising in the presence of
> > automounted /sys/kernel/debug/tracing/.
> > 
> >   # Don't change /sys/kernel/tracing/ permissions on automount.
> >   umount /sys/kernel/debug/tracing/
> >   stat /sys/kernel/debug/tracing/.
> > 
> >   # Don't change /sys/kernel/tracing/ permissions.
> >   mount -t tracefs none /mnt/foo
> > 
> >   # Change /sys/kernel/tracing/ mode and uid, but not gid.
> >   mount -t tracefs -o uid=bar,mode=0750 none /mnt/baz
> > 
> 
> The above text doesn't make sense. Is the comments what you are doing or
> what the system is doing? If it is what the system is doing, please show
> the output of the stat command and how it is doing something unexpected.

Sorry, I do see how the text as-is is unclear. The intention is to
describe the new, intended behavior, and I only left the existing
behavior as implied.

> Can you show the example of what is wrong, and what you expected to happen.
> The above is just a bunch of commands, but does not display anything that
> is incorrect.

Sure, here's a narrower description of old (unexpected) and new
(expected) behavior, in case you were wanting to update the changelog on
your own:

---

Existing behavior:

  ## Pre-existing status: tracefs is 0755.
  # stat -c '%A' /sys/kernel/tracing/
  drwxr-xr-x

  ## (Re)trigger the automount.
  # umount /sys/kernel/debug/tracing
  # stat -c '%A' /sys/kernel/debug/tracing/.
  drwx------

  ## Unexpected: the automount changed mode for other mount instances.
  # stat -c '%A' /sys/kernel/tracing/
  drwx------

New behavior:

  ## Pre-existing status: tracefs is 0755.
  # stat -c '%A' /sys/kernel/tracing/
  drwxr-xr-x

  ## (Re)trigger the automount.
  # umount /sys/kernel/debug/tracing
  # stat -c '%A' /sys/kernel/debug/tracing/.
  drwxr-xr-x

  ## Expected: the automount does not change other mount instances.
  # stat -c '%A' /sys/kernel/tracing/
  drwxr-xr-x

---

There are other variations of old/new behavior (e.g., if using various
mount options), but those are not the main reason for this patch.

> > Signed-off-by: Brian Norris <briannorris@...omium.org>
> > ---
> > I'm open to writing an LTP test case for this, if that seems like a good
> > idea.
> 
> Yes, please add a test :-)

Sure, I'm dusting off my LTP VM now. But in case you'd like the patch
as-is, feel free to splice the above into the commit description.
Otherwise, I may resend as Greg requested for patch 1, after I've got a
test patch going.

Brian