Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Matthew Garrett <matthewgarrett@google.com>,
	James Morris James Morris <jmorris@namei.org>,
	LSM List <linux-security-module@vger.kernel.org>,
	Linux API <linux-api@vger.kernel.org>,
	Ben Hutchings <ben@decadent.org.uk>,
	Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown
Date: Fri, 11 Oct 2019 17:08:33 -0400
Message-ID: <20191011170833.2312857c@gandalf.local.home> (raw)
In-Reply-To: <CAHk-=whC6Ji=fWnjh2+eS4b15TnbsS4VPVtvBOwCy1jjEG_JHQ@mail.gmail.com>

On Fri, 11 Oct 2019 13:46:11 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Oct 11, 2019 at 1:25 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > OK, so I tried this approach, and there's a bit more than just a "few"
> > extra cases that use tracing_open_generic().  
> 
> Yeah, that's more than I would have expected.
> 
> That said, you also did what I consider a somewhat over-done conversion.
> 
> Just do
> 
>    static inline bool tracefs_lockdown_or_disabled(void)
>    { return tracing_disabled || security_locked_down(LOCKDOWN_TRACEFS); }
> 
> and ignore the pointless return value (we know it's EPERM, and we
> really don't care).
> 
> And then several of those conversions just turn into oneliner
> 
> -       if (tracing_disabled)
> +       if (tracefs_lockdown_or_disabled())
>                  return -ENODEV;

OK, so this is similar to what I just sent. But instead I made it into
a function that combines tracing_disabled and the locked_down, plus, it
did the reference update for the trace_array if needed (which in doing
this exercise, I think I found a couple of places that need the ref
count update).

> 
> patches instead.
> 
> I'm also not sure why you bothered with a lot of the files that don't
> currently even have that "tracing_disabled" logic at all. Yeah, they
> show pre-existing tracing info, but even if you locked down _after_
> starting some tracing, that's probably what you want. You just don't
> want people to be able to add new trace events.

I guess just preventing new traces would be good enough (or anything
that records data), as well as seeing that recorded data.

Note, the tracing_disabled was added in case the ring buffer got
corrupted, and we wanted to prevent the system from crashing if someone
read the corrupted ring buffer. In the over 10 years of having that
check, I don't recall a single instance of someone triggering it :-/


> 
> For example, maybe you want to have lockdown after you've booted, but
> still show boot time traces?
> 
> I dunno. I feel like you re-did the original patch, and the original
> patch was designed for "least line impact" rather than for anything
> else.
> 
> I also suspect that if you *actually* do lockdown at early boot (which
> is presumably one common case), then all you need is to do
> 
>     --- a/fs/tracefs/inode.c
>     +++ b/fs/tracefs/inode.c
>     @@ -418,6 +418,9 @@ struct dentry *tracefs_create_file(
>         struct dentry *dentry;
>         struct inode *inode;
> 
>     +   if (security_locked_down(LOCKDOWN_TRACEFS))
>     +           return NULL;
>     +

Sounds reasonable.

>         if (!(mode & S_IFMT))
>                 mode |= S_IFREG;
>         BUG_ON(!S_ISREG(mode));
> 
> and the "open-time check" is purely for "we did lockdown _after_ boot,
> but then you might well want to be able to read the existing trace
> information that you did before you locked down.
> 
> Again - I think trying to emulate exactly what that broken lockdown
> patch did is not really what you should aim for.
> 
> That patch was not only buggy, it really wasn't about what people
> really necessarily _want_, as about "we don't want to deal with
> tracing, so here's a minimal patch that doesn't even work".

OK. My new approach is to:

1) revert the tracefs lockdown patch
2) Add my tracing_check_open_get_tr() patch (and consolidate the calls)
 (fix up anything that should have this too)
3) Add the lockdown to the tracing_check_open_get_tr() routine, and be
   done with it.

-- Steve

  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-11 17:54 Steven Rostedt
2019-10-11 18:20 ` Linus Torvalds
2019-10-11 18:36   ` Steven Rostedt
2019-10-11 19:24     ` Linus Torvalds
2019-10-11 19:50     ` Ben Hutchings
2019-10-11 21:46     ` Florian Weimer
2019-10-11 22:27       ` Steven Rostedt
2019-10-11 20:25   ` Steven Rostedt
2019-10-11 20:46     ` Linus Torvalds
2019-10-11 21:08       ` Steven Rostedt [this message]
2019-10-11 20:54     ` Steven Rostedt
2019-10-11 21:00       ` Linus Torvalds
2019-10-11 21:11         ` Steven Rostedt

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191011170833.2312857c@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=ben@decadent.org.uk \
    --cc=jmorris@namei.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=matthewgarrett@google.com \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git