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 23:10:45 +0000
From:   Luis Chamberlain <mcgrof@kernel.org>
To:     Rafael Aquini <aquini@redhat.com>, Tso Ted <tytso@mit.edu>
Cc:     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: <20200511231045.GV11244@42.do-not-panic.com>
References: <20200511215904.719257-1-aquini@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200511215904.719257-1-aquini@redhat.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

> +
>  		/*
>  		 * 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.

   Luis

>  	}
>  
> +
>  	return err;
>  }
>  
> -- 
> 2.25.4
>