On Fri, 11 Oct 2019 13:46:11 -0700
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, Oct 11, 2019 at 1:25 PM Steven Rostedt <rostedt@xxxxxxxxxxx> 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
