From: Rafael Aquini <aquini@redhat.com>
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: Tso Ted <tytso@mit.edu>,
linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
keescook@chromium.org, yzaikin@google.com
Subject: Re: [PATCH] kernel: sysctl: ignore invalid taint bits introduced via kernel.tainted and taint the kernel with TAINT_USER on writes
Date: Mon, 11 May 2020 21:03:13 -0400 [thread overview]
Message-ID: <20200512010313.GA725253@optiplex-lnx> (raw)
In-Reply-To: <20200512001702.GW11244@42.do-not-panic.com>
On Tue, May 12, 2020 at 12:17:03AM +0000, Luis Chamberlain wrote:
> On Mon, May 11, 2020 at 07:59:14PM -0400, Rafael Aquini wrote:
> > On Mon, May 11, 2020 at 11:10:45PM +0000, Luis Chamberlain wrote:
> > > On Mon, May 11, 2020 at 05:59:04PM -0400, Rafael Aquini wrote:
> > > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > > > index 8a176d8727a3..f0a4fb38ac62 100644
> > > > --- a/kernel/sysctl.c
> > > > +++ b/kernel/sysctl.c
> > > > @@ -2623,17 +2623,32 @@ static int proc_taint(struct ctl_table *table, int write,
> > > > return err;
> > > >
> > > > if (write) {
> > > > + int i;
> > > > +
> > > > + /*
> > > > + * Ignore user input that would make us committing
> > > > + * arbitrary invalid TAINT flags in the loop below.
> > > > + */
> > > > + tmptaint &= (1UL << TAINT_FLAGS_COUNT) - 1;
> > >
> > > This looks good but we don't pr_warn() of information lost on intention.
> > >
> >
> > Are you thinking in sth like:
> >
> > + if (tmptaint > TAINT_FLAGS_MAX) {
> > + tmptaint &= TAINT_FLAGS_MAX;
> > + pr_warn("proc_taint: out-of-range invalid input ignored"
> > + " tainted_mask adjusted to 0x%x\n", tmptaint);
> > + }
> > ?
>
> Sure that would clarify this.
>
> > > > +
> > > > /*
> > > > * Poor man's atomic or. Not worth adding a primitive
> > > > * to everyone's atomic.h for this
> > > > */
> > > > - int i;
> > > > for (i = 0; i < BITS_PER_LONG && tmptaint >> i; i++) {
> > > > if ((tmptaint >> i) & 1)
> > > > add_taint(i, LOCKDEP_STILL_OK);
> > > > }
> > > > +
> > > > + /*
> > > > + * Users with SYS_ADMIN capability can include any arbitrary
> > > > + * taint flag by writing to this interface. If that's the case,
> > > > + * we also need to mark the kernel "tainted by user".
> > > > + */
> > > > + add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> > >
> > > I'm in favor of this however I'd like to hear from Ted on if it meets
> > > the original intention. I would think he had a good reason not to add
> > > it here.
> > >
> >
> > Fair enough. The impression I got by reading Ted's original commit
> > message is that the intent was to have TAINT_USER as the flag set
> > via this interface, even though the code was allowing for any
> > arbitrary value.
>
> That wasn't my reading, it was that the user did something very odd
> with user input which we don't like as kernel developers, and it gives
> us a way to prove: hey you did something stupid, sorry but I cannot
> support your kernel panic.
>
> > I think it's OK to let the user fiddle with
> > the flags, as it's been allowed since the introduction of
> > this interface, but we need to reflect that fact in the
> > tainting itself. Since TAINT_USER is not used anywhere,
>
> I see users of TAINT_USER sprinkled around
>
I meant in the original commit that introduced it
(commit 34f5a39899f3f3e815da64f48ddb72942d86c366). Sorry I
miscomunicated that.
In its current usage, it seems that the other places adding TAINT_USER
match with what is being proposed here: To signal when we have user
fiddling with kernel / module parameters.
> > this change perfectly communicates that fact without
> > the need for introducing yet another taint flag.
>
> I'd be happy if we don't have introduce yet-anothe flag as well.
> But since Ted introduced it, without using the flag on the proc_taint()
> I'd like confirmation we won't screw things up with existing test cases
> which assume proc_taint() won't set this up. We'd therefore regress
> userspace.
>
> This is why I'd like for us to be careful with this flag.
>
> Luis
>
next prev parent reply other threads:[~2020-05-12 1:03 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-11 21:59 [PATCH] kernel: sysctl: ignore invalid taint bits introduced via kernel.tainted and taint the kernel with TAINT_USER on writes Rafael Aquini
2020-05-11 23:10 ` Luis Chamberlain
2020-05-11 23:59 ` Rafael Aquini
2020-05-12 0:17 ` Luis Chamberlain
2020-05-12 1:03 ` Rafael Aquini [this message]
2020-05-12 5:04 ` Luis Chamberlain
2020-05-12 14:49 ` Rafael Aquini
2020-05-12 15:46 ` Luis Chamberlain
2020-05-12 16:01 ` Rafael Aquini
Reply instructions:
You may reply publicly 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=20200512010313.GA725253@optiplex-lnx \
--to=aquini@redhat.com \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=tytso@mit.edu \
--cc=yzaikin@google.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).