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