Re: [PATCH] kernel: sysctl: ignore invalid taint bits introduced via kernel.tainted and taint the kernel with TAINT_USER on writes

[Luis Chamberlain <mcgrof@kernel.org>]

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

> 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