| Message ID | 20191016131506.17193-1-chrubis@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | lib: tst_taint: Ignore WARN taint flag if already set | expand |
----- Original Message ----- > This commit changes the library so that it ignores the taint warn flag > if it was set prior to the test run. It turns out that the warn taint > flag is not well defined and could be easily set on a freshly booted > kernel for example when buggy BIOS is detected. > > Signed-off-by: Cyril Hrubis <chrubis@suse.cz> > CC: Jan Stancek <jstancek@redhat.com> > CC: Chang Yin <cyin@redhat.com> > --- > > I haven't tested this, since I don't have a system where the flag is set > at hand, but it's simple enough so that it should work as expected. > > lib/tst_taint.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/lib/tst_taint.c b/lib/tst_taint.c > index a5dbf77d2..3de6d72f4 100644 > --- a/lib/tst_taint.c > +++ b/lib/tst_taint.c > @@ -82,8 +82,13 @@ void tst_taint_init(unsigned int mask) > tst_res(TCONF, "Kernel is too old for requested mask"); > > taint_mask = mask; > - > taint = tst_taint_read(); > + > + if (mask & TST_TAINT_W && taint & TST_TAINT_W) { > + tst_res(TCONF, "Ignoring already set kernel warning taint"); > + mask &= ~TST_TAINT_W; Shouldn't this rather mask it from taint_mask? Otherwise, won't you still get an error in tst_taint_check()? > + } > + > if ((taint & mask) != 0) > tst_brk(TBROK, "Kernel is already tainted: %u", taint); > } > -- > 2.21.0 > >
Hi, On 16.10.19 15:15, Cyril Hrubis wrote: > This commit changes the library so that it ignores the taint warn flag > if it was set prior to the test run. It turns out that the warn taint > flag is not well defined and could be easily set on a freshly booted > kernel for example when buggy BIOS is detected.> > Signed-off-by: Cyril Hrubis <chrubis@suse.cz> > CC: Jan Stancek <jstancek@redhat.com> > CC: Chang Yin <cyin@redhat.com> > --- > > I haven't tested this, since I don't have a system where the flag is set > at hand, but it's simple enough so that it should work as expected. > > lib/tst_taint.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/lib/tst_taint.c b/lib/tst_taint.c > index a5dbf77d2..3de6d72f4 100644 > --- a/lib/tst_taint.c > +++ b/lib/tst_taint.c > @@ -82,8 +82,13 @@ void tst_taint_init(unsigned int mask) > tst_res(TCONF, "Kernel is too old for requested mask"); > > taint_mask = mask; > - > taint = tst_taint_read(); > + > + if (mask & TST_TAINT_W && taint & TST_TAINT_W) { > + tst_res(TCONF, "Ignoring already set kernel warning taint"); > + mask &= ~TST_TAINT_W; > + } > + > if ((taint & mask) != 0) > tst_brk(TBROK, "Kernel is already tainted: %u", taint); > } > Tests may rely on TAINT_W to decide the result. If we need TAINT_W, we can only run the test when it was not set before. For example, testcases/cve/cve-2017-17053.c relies on this. This may render this testcase unusable, or do I get this wrong?
----- Original Message ----- > Hi, > > On 16.10.19 15:15, Cyril Hrubis wrote: > > This commit changes the library so that it ignores the taint warn flag > > if it was set prior to the test run. It turns out that the warn taint > > flag is not well defined and could be easily set on a freshly booted > > kernel for example when buggy BIOS is detected.> > > Signed-off-by: Cyril Hrubis <chrubis@suse.cz> > > CC: Jan Stancek <jstancek@redhat.com> > > CC: Chang Yin <cyin@redhat.com> > > --- > > > > I haven't tested this, since I don't have a system where the flag is set > > at hand, but it's simple enough so that it should work as expected. > > > > lib/tst_taint.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/lib/tst_taint.c b/lib/tst_taint.c > > index a5dbf77d2..3de6d72f4 100644 > > --- a/lib/tst_taint.c > > +++ b/lib/tst_taint.c > > @@ -82,8 +82,13 @@ void tst_taint_init(unsigned int mask) > > tst_res(TCONF, "Kernel is too old for requested mask"); > > > > taint_mask = mask; > > - > > taint = tst_taint_read(); > > + > > + if (mask & TST_TAINT_W && taint & TST_TAINT_W) { > > + tst_res(TCONF, "Ignoring already set kernel warning taint"); > > + mask &= ~TST_TAINT_W; > > + } > > + > > if ((taint & mask) != 0) > > tst_brk(TBROK, "Kernel is already tainted: %u", taint); > > } > > > > Tests may rely on TAINT_W to decide the result. > If we need TAINT_W, we can only run the test when it was not set before. For example, > testcases/cve/cve-2017-17053.c relies on this. > This may render this testcase unusable, or do I get this wrong? Patch masks TAINT_W if that taint flag was set before test started. In such case, test can't determine result accurately anyway. I wonder if there is more specific way for cve-2017-17053 to determine error condition. For example by inspecting new messages in dmesg.
Hi! > > + > > + if (mask & TST_TAINT_W && taint & TST_TAINT_W) { > > + tst_res(TCONF, "Ignoring already set kernel warning taint"); > > + mask &= ~TST_TAINT_W; > > Shouldn't this rather mask it from taint_mask? > Otherwise, won't you still get an error in tst_taint_check()? Of course. > > + } > > + > > if ((taint & mask) != 0) And this has to be changed so that we check the taint againts the taint_mask . > > tst_brk(TBROK, "Kernel is already tainted: %u", taint); > > } > > -- > > 2.21.0 > > > >
Hi! > > diff --git a/lib/tst_taint.c b/lib/tst_taint.c > > index a5dbf77d2..3de6d72f4 100644 > > --- a/lib/tst_taint.c > > +++ b/lib/tst_taint.c > > @@ -82,8 +82,13 @@ void tst_taint_init(unsigned int mask) > > tst_res(TCONF, "Kernel is too old for requested mask"); > > > > taint_mask = mask; > > - > > taint = tst_taint_read(); > > + > > + if (mask & TST_TAINT_W && taint & TST_TAINT_W) { > > + tst_res(TCONF, "Ignoring already set kernel warning taint"); > > + mask &= ~TST_TAINT_W; > > + } > > + > > if ((taint & mask) != 0) > > tst_brk(TBROK, "Kernel is already tainted: %u", taint); > > } > > > > Tests may rely on TAINT_W to decide the result. If we need TAINT_W, we > can only run the test when it was not set before. For example, > testcases/cve/cve-2017-17053.c relies on this. > This may render this testcase unusable, or do I get this wrong? Only on systems where something set the flag already. Ideally that will not happen on many systems.
Hi! > I wonder if there is more specific way for cve-2017-17053 to determine > error condition. For example by inspecting new messages in dmesg. We wanted to avoid that because the taint flags looked like a nice and simple solution...
Hi Cyril, Per the discussion in history, I'm asking do you plan to draft patch v2 for it? Or, do we have new thoughts on this?
diff --git a/lib/tst_taint.c b/lib/tst_taint.c index a5dbf77d2..3de6d72f4 100644 --- a/lib/tst_taint.c +++ b/lib/tst_taint.c @@ -82,8 +82,13 @@ void tst_taint_init(unsigned int mask) tst_res(TCONF, "Kernel is too old for requested mask"); taint_mask = mask; - taint = tst_taint_read(); + + if (mask & TST_TAINT_W && taint & TST_TAINT_W) { + tst_res(TCONF, "Ignoring already set kernel warning taint"); + mask &= ~TST_TAINT_W; + } + if ((taint & mask) != 0) tst_brk(TBROK, "Kernel is already tainted: %u", taint); }
This commit changes the library so that it ignores the taint warn flag if it was set prior to the test run. It turns out that the warn taint flag is not well defined and could be easily set on a freshly booted kernel for example when buggy BIOS is detected. Signed-off-by: Cyril Hrubis <chrubis@suse.cz> CC: Jan Stancek <jstancek@redhat.com> CC: Chang Yin <cyin@redhat.com> --- I haven't tested this, since I don't have a system where the flag is set at hand, but it's simple enough so that it should work as expected. lib/tst_taint.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)