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

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

[Rafael Aquini]

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;
+
 		/*
 		 * 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);
 	}
 
+
 	return err;
 }
 
-- 
2.25.4

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

[Luis Chamberlain]

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
> 

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

[Rafael Aquini]

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

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

[Luis Chamberlain]

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

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

[Rafael Aquini]

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
> 

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

[Luis Chamberlain]

On Mon, May 11, 2020 at 09:03:13PM -0400, Rafael Aquini wrote:
> 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.

drivers/base/regmap/regmap-debugfs.c requires *manual* code changes
to compile / enable some knob. i915 complains about unsafe module
params such as module_param_cb_unsafe() core_param_unsafe(). Then
drivers/soundwire/cadence_master.c is for when a debugfs dangerous
param was used.

This still doesn't rule out the use of proc_taint() for testing taint,
and that adding it may break some tests. So even though this would
only affect some tests scripts, I can't say that adding this taint won't
cause some headaches to someone. I wouldn't encourage its use on
proc_taint() from what I can see so far.

  Luis

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

[Rafael Aquini]

On Tue, May 12, 2020 at 05:04:05AM +0000, Luis Chamberlain wrote:
> On Mon, May 11, 2020 at 09:03:13PM -0400, Rafael Aquini wrote:
> > 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.
> 
> drivers/base/regmap/regmap-debugfs.c requires *manual* code changes
> to compile / enable some knob. i915 complains about unsafe module
> params such as module_param_cb_unsafe() core_param_unsafe(). Then
> drivers/soundwire/cadence_master.c is for when a debugfs dangerous
> param was used.
> 
> This still doesn't rule out the use of proc_taint() for testing taint,
> and that adding it may break some tests. So even though this would
> only affect some tests scripts, I can't say that adding this taint won't
> cause some headaches to someone. I wouldn't encourage its use on
> proc_taint() from what I can see so far.
>

OK, I´ll repost without the hunk forcing the taint. If we eventually
come to the conclusion that tainting in proc_taint() is the right thing
to do, we can do that part of the change later.

Do you think we should use printk_ratelimited() in the ignore message,
instead? 

Cheers,
-- Rafael

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

[Luis Chamberlain]

On Tue, May 12, 2020 at 10:49:06AM -0400, Rafael Aquini wrote:
> On Tue, May 12, 2020 at 05:04:05AM +0000, Luis Chamberlain wrote:
> > On Mon, May 11, 2020 at 09:03:13PM -0400, Rafael Aquini wrote:
> > > 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.
> > 
> > drivers/base/regmap/regmap-debugfs.c requires *manual* code changes
> > to compile / enable some knob. i915 complains about unsafe module
> > params such as module_param_cb_unsafe() core_param_unsafe(). Then
> > drivers/soundwire/cadence_master.c is for when a debugfs dangerous
> > param was used.
> > 
> > This still doesn't rule out the use of proc_taint() for testing taint,
> > and that adding it may break some tests. So even though this would
> > only affect some tests scripts, I can't say that adding this taint won't
> > cause some headaches to someone. I wouldn't encourage its use on
> > proc_taint() from what I can see so far.
> >
> 
> OK, I´ll repost without the hunk forcing the taint. If we eventually
> come to the conclusion that tainting in proc_taint() is the right thing
> to do, we can do that part of the change later.

Just add another taint, we have 64 bits and according to you we won't
ever run out. TAINT_CUSTOM or whatever.

> Do you think we should use printk_ratelimited() in the ignore message,
> instead? 

No, that's for when there are many prints at the same time, you probably
want pr_warn_once().

  Luis

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

[Rafael Aquini]

On Tue, May 12, 2020 at 03:46:54PM +0000, Luis Chamberlain wrote:
> On Tue, May 12, 2020 at 10:49:06AM -0400, Rafael Aquini wrote:
> > On Tue, May 12, 2020 at 05:04:05AM +0000, Luis Chamberlain wrote:
> > > On Mon, May 11, 2020 at 09:03:13PM -0400, Rafael Aquini wrote:
> > > > 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.
> > > 
> > > drivers/base/regmap/regmap-debugfs.c requires *manual* code changes
> > > to compile / enable some knob. i915 complains about unsafe module
> > > params such as module_param_cb_unsafe() core_param_unsafe(). Then
> > > drivers/soundwire/cadence_master.c is for when a debugfs dangerous
> > > param was used.
> > > 
> > > This still doesn't rule out the use of proc_taint() for testing taint,
> > > and that adding it may break some tests. So even though this would
> > > only affect some tests scripts, I can't say that adding this taint won't
> > > cause some headaches to someone. I wouldn't encourage its use on
> > > proc_taint() from what I can see so far.
> > >
> > 
> > OK, I´ll repost without the hunk forcing the taint. If we eventually
> > come to the conclusion that tainting in proc_taint() is the right thing
> > to do, we can do that part of the change later.
> 
> Just add another taint, we have 64 bits and according to you we won't
> ever run out. TAINT_CUSTOM or whatever.
>

I don't think this deserves a custom taint, and TAINT_USER should be the
one to add here, as it clearly communicates what's being done at this
point. If we cannot compromise on utilizing it, then lets just forget
about forcing any other flag at this point. As per tracking which
taint flags a user has forced via this interface, I do have another
idea that I still have to iron out, but I'll propose it soon enough.
 
> > Do you think we should use printk_ratelimited() in the ignore message,
> > instead? 
> 
> No, that's for when there are many prints at the same time, you probably
> want pr_warn_once().

OK.