| Message ID | 20230920154447.3165-1-chrubis@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | lib: lockdown: Report lockdown as disabled on missing sysfs | expand |
On 20. 09. 23 17:44, Cyril Hrubis wrote: > We currently report -1 when secure boot sysfs file is not present which > is later interpreted as secure boot enabled. This causes regression in > *_module sycall tests executed on systems when secureboot is not > compiled-in or supported at all. That's incorrect usage then. The tests should check tst_secureboot_enabled() > 0 instead. I think it will be useful to know whether the function found that secureboot is disabled, or could not check at all. We should just document it better. > > Signed-off-by: Cyril Hrubis <chrubis@suse.cz> > --- > lib/tst_lockdown.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/tst_lockdown.c b/lib/tst_lockdown.c > index 38d830886..7613092ec 100644 > --- a/lib/tst_lockdown.c > +++ b/lib/tst_lockdown.c > @@ -29,7 +29,7 @@ int tst_secureboot_enabled(void) > > if (access(SECUREBOOT_VAR, F_OK)) { > tst_res(TINFO, "SecureBoot sysfs file not available"); > - return -1; > + return 0; > } > > fd = open(SECUREBOOT_VAR, O_RDONLY);
Hi! > > We currently report -1 when secure boot sysfs file is not present which > > is later interpreted as secure boot enabled. This causes regression in > > *_module sycall tests executed on systems when secureboot is not > > compiled-in or supported at all. > > That's incorrect usage then. The tests should check > tst_secureboot_enabled() > 0 instead. I think it will be useful to know > whether the function found that secureboot is disabled, or could not > check at all. We should just document it better. Yes, the functions do not seem to have any documentation. So I guess that we need: diff --git a/lib/tst_test.c b/lib/tst_test.c index 2e58cad33..e2c195645 100644 --- a/lib/tst_test.c +++ b/lib/tst_test.c @@ -1163,10 +1163,10 @@ static void do_setup(int argc, char *argv[]) if (tst_test->supported_archs && !tst_is_on_arch(tst_test->supported_archs)) tst_brk(TCONF, "This arch '%s' is not supported for test!", tst_arch.name); - if (tst_test->skip_in_lockdown && tst_lockdown_enabled()) + if (tst_test->skip_in_lockdown && tst_lockdown_enabled() > 0) tst_brk(TCONF, "Kernel is locked down, skipping test"); - if (tst_test->skip_in_secureboot && tst_secureboot_enabled()) + if (tst_test->skip_in_secureboot && tst_secureboot_enabled() > 0) tst_brk(TCONF, "SecureBoot enabled, skipping test"); if (tst_test->skip_in_compat && TST_ABI != tst_kernel_bits())
Hi, On 20. 09. 23 20:17, Cyril Hrubis wrote: > Hi! >>> We currently report -1 when secure boot sysfs file is not present which >>> is later interpreted as secure boot enabled. This causes regression in >>> *_module sycall tests executed on systems when secureboot is not >>> compiled-in or supported at all. >> >> That's incorrect usage then. The tests should check >> tst_secureboot_enabled() > 0 instead. I think it will be useful to know >> whether the function found that secureboot is disabled, or could not >> check at all. We should just document it better. > > Yes, the functions do not seem to have any documentation. > > So I guess that we need: > > diff --git a/lib/tst_test.c b/lib/tst_test.c > index 2e58cad33..e2c195645 100644 > --- a/lib/tst_test.c > +++ b/lib/tst_test.c > @@ -1163,10 +1163,10 @@ static void do_setup(int argc, char *argv[]) > if (tst_test->supported_archs && !tst_is_on_arch(tst_test->supported_archs)) > tst_brk(TCONF, "This arch '%s' is not supported for test!", tst_arch.name); > > - if (tst_test->skip_in_lockdown && tst_lockdown_enabled()) > + if (tst_test->skip_in_lockdown && tst_lockdown_enabled() > 0) > tst_brk(TCONF, "Kernel is locked down, skipping test"); > > - if (tst_test->skip_in_secureboot && tst_secureboot_enabled()) > + if (tst_test->skip_in_secureboot && tst_secureboot_enabled() > 0) > tst_brk(TCONF, "SecureBoot enabled, skipping test"); > > if (tst_test->skip_in_compat && TST_ABI != tst_kernel_bits()) > Yes, this is the correct fix.
diff --git a/lib/tst_lockdown.c b/lib/tst_lockdown.c index 38d830886..7613092ec 100644 --- a/lib/tst_lockdown.c +++ b/lib/tst_lockdown.c @@ -29,7 +29,7 @@ int tst_secureboot_enabled(void) if (access(SECUREBOOT_VAR, F_OK)) { tst_res(TINFO, "SecureBoot sysfs file not available"); - return -1; + return 0; } fd = open(SECUREBOOT_VAR, O_RDONLY);
We currently report -1 when secure boot sysfs file is not present which is later interpreted as secure boot enabled. This causes regression in *_module sycall tests executed on systems when secureboot is not compiled-in or supported at all. Signed-off-by: Cyril Hrubis <chrubis@suse.cz> --- lib/tst_lockdown.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)