Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 11 May 2020 19:59:14 -0400
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
Message-ID: <20200511235914.GF367616@optiplex-lnx>
References: <20200511215904.719257-1-aquini@redhat.com>
 <20200511231045.GV11244@42.do-not-panic.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200511231045.GV11244@42.do-not-panic.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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:
> > The sysctl knob allows any user with SYS_ADMIN capability to
> > taint the kernel with any arbitrary value, but this might
> > produce an invalid flags bitset being committed to tainted_mask.
> > 
> > This patch introduces a simple way for proc_taint() to ignore
> > any eventual invalid bit coming from the user input before
> > committing those bits to the kernel tainted_mask, as well as
> > it makes clear use of TAINT_USER flag to mark the kernel
> > tainted by user everytime a taint value is written
> > to the kernel.tainted sysctl.
> > 
> > Signed-off-by: Rafael Aquini <aquini@redhat.com>
> > ---
> >  kernel/sysctl.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > 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);
+               }

?
 
> > +
> >  		/*
> >  		 * 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. 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,
this change perfectly communicates that fact without
the need for introducing yet another taint flag.

Cheers!
-- Rafael