| Message ID | 20200511215904.719257-1-aquini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kernel: sysctl: ignore invalid taint bits introduced via kernel.tainted and taint the kernel with TAINT_USER on writes | expand |
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 >
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
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
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 >
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
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
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
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.
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; }
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(-)