| Message ID | 20201109164605.25965-2-mdoucha@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] Add tst_secureboot_enabled() helper function | expand |
On Tue, Nov 10, 2020 at 12:46 AM Martin Doucha <mdoucha@suse.cz> wrote: > ... > > include $(top_srcdir)/include/mk/testcases.mk > > +CFLAGS += $(EFIVAR_CFLAGS) > +LDLIBS += $(EFIVAR_LIBS) > Where can we get the value of these two variables? Shouldn't we add AC_SUBST() in the m4 file? > --- a/testcases/kernel/syscalls/ioperm/ioperm02.c > +++ b/testcases/kernel/syscalls/ioperm/ioperm02.c > @@ -22,6 +22,7 @@ > #include <pwd.h> > #include "tst_test.h" > #include "tst_safe_macros.h" > +#include "tst_secureboot.h" > > #if defined __i386__ || defined(__x86_64__) > #include <sys/io.h> > @@ -45,6 +46,10 @@ static struct tcase_t { > > static void setup(void) > { > + /* ioperm() is restricted under kernel lockdown. */ > + if (tst_lockdown_enabled() || tst_secureboot_enabled() > 0) > + tst_brk(TCONF, "Kernel is locked down, skip this test"); > The ioperm02 is an error test for ioperm(), it doesn't matter without the lockdown/secure-boot status. Better to remove this from setup(). iopl02 as well.
Hi! > > ... > > > > include $(top_srcdir)/include/mk/testcases.mk > > > > +CFLAGS += $(EFIVAR_CFLAGS) > > +LDLIBS += $(EFIVAR_LIBS) > > > > Where can we get the value of these two variables? Shouldn't we > add AC_SUBST() in the m4 file? These are exported by the PKG_CHECK_MODULES() pkgconfig autotools macro. > > --- a/testcases/kernel/syscalls/ioperm/ioperm02.c > > +++ b/testcases/kernel/syscalls/ioperm/ioperm02.c > > @@ -22,6 +22,7 @@ > > #include <pwd.h> > > #include "tst_test.h" > > #include "tst_safe_macros.h" > > +#include "tst_secureboot.h" > > > > #if defined __i386__ || defined(__x86_64__) > > #include <sys/io.h> > > @@ -45,6 +46,10 @@ static struct tcase_t { > > > > static void setup(void) > > { > > + /* ioperm() is restricted under kernel lockdown. */ > > + if (tst_lockdown_enabled() || tst_secureboot_enabled() > 0) > > + tst_brk(TCONF, "Kernel is locked down, skip this test"); > > > > The ioperm02 is an error test for ioperm(), it doesn't matter without the > lockdown/secure-boot status. Better to remove this from setup(). > > iopl02 as well. Actually I think that this is correct, since there is no imposed order on the checks in kernel, so we may not get the errors we expect to get. What we are actually missing are tests that iopl() and ioperm() does fail with EPERM when either of lockdown or secureboot are enabled.
On Tue, Nov 10, 2020 at 4:51 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > > ... > > > > > > include $(top_srcdir)/include/mk/testcases.mk > > > > > > +CFLAGS += $(EFIVAR_CFLAGS) > > > +LDLIBS += $(EFIVAR_LIBS) > > > > > > > Where can we get the value of these two variables? Shouldn't we > > add AC_SUBST() in the m4 file? > > These are exported by the PKG_CHECK_MODULES() pkgconfig autotools macro. > Good to know this. > > > > --- a/testcases/kernel/syscalls/ioperm/ioperm02.c > > > +++ b/testcases/kernel/syscalls/ioperm/ioperm02.c > > > @@ -22,6 +22,7 @@ > > > #include <pwd.h> > > > #include "tst_test.h" > > > #include "tst_safe_macros.h" > > > +#include "tst_secureboot.h" > > > > > > #if defined __i386__ || defined(__x86_64__) > > > #include <sys/io.h> > > > @@ -45,6 +46,10 @@ static struct tcase_t { > > > > > > static void setup(void) > > > { > > > + /* ioperm() is restricted under kernel lockdown. */ > > > + if (tst_lockdown_enabled() || tst_secureboot_enabled() > 0) > > > + tst_brk(TCONF, "Kernel is locked down, skip this > test"); > > > > > > > The ioperm02 is an error test for ioperm(), it doesn't matter without the > > lockdown/secure-boot status. Better to remove this from setup(). > > > > iopl02 as well. > > Actually I think that this is correct, since there is no imposed order > on the checks in kernel, so we may not get the errors we expect to get. > > > What we are actually missing are tests that iopl() and ioperm() does > fail with EPERM when either of lockdown or secureboot are enabled. > I remember they(ioperm02, iopl02) works well with secure-boot enabled/disabled. (I did that test when reviewing Erico's tst_lockdown.c patch) Okay, but I agree that it's safer to add this check because it may change in the newer kernel someday. Feel free to add: Reviewed-by: Li Wang <liwang@redhat.com>
Hi, ... > > > include $(top_srcdir)/include/mk/testcases.mk > > > +CFLAGS += $(EFIVAR_CFLAGS) > > > +LDLIBS += $(EFIVAR_LIBS) > > Where can we get the value of these two variables? Shouldn't we > > add AC_SUBST() in the m4 file? > These are exported by the PKG_CHECK_MODULES() pkgconfig autotools macro. FYI: I added a fix for old pkg-config (< 0.24) into m4/ltp-tirpc.m4 (the first m4 file which started to use pkg-config) https://autotools.io/pkgconfig/pkg_check_modules.html But 0.24 is probably old enough (2010; 0.23 was released 2008), thus we should probably remove it. Kind regards, Petr
Hi, > SecureBoot implies integrity lockdown even if tst_lockdown_enabled() cannot > check lockdown status directly. Udpate skip condition in ioperm() and iopl() > tests. Reviewed-by: Petr Vorel <pvorel@suse.cz> ... > + /* ioperm() is restricted under kernel lockdown. */ > + if (tst_lockdown_enabled() || tst_secureboot_enabled() > 0) > + tst_brk(TCONF, "Kernel is locked down, skip this test"); > + I wonder if this could be interesting for docparse to have this functionality exposed as a struct tst_test flag: .tst_lockdown_restricted = 1 Kind regards, Petr
Hi! > I wonder if this could be interesting for docparse to have this functionality > exposed as a struct tst_test flag: > .tst_lockdown_restricted = 1 Well if we happen to have more than two testcases in the future it would make sense to move the checks to the library like this instead.
diff --git a/testcases/kernel/syscalls/ioperm/Makefile b/testcases/kernel/syscalls/ioperm/Makefile index 044619fb8..8624e2c99 100644 --- a/testcases/kernel/syscalls/ioperm/Makefile +++ b/testcases/kernel/syscalls/ioperm/Makefile @@ -5,4 +5,7 @@ top_srcdir ?= ../../../.. include $(top_srcdir)/include/mk/testcases.mk +CFLAGS += $(EFIVAR_CFLAGS) +LDLIBS += $(EFIVAR_LIBS) + include $(top_srcdir)/include/mk/generic_leaf_target.mk diff --git a/testcases/kernel/syscalls/ioperm/ioperm01.c b/testcases/kernel/syscalls/ioperm/ioperm01.c index fc5754be9..01f83aefe 100644 --- a/testcases/kernel/syscalls/ioperm/ioperm01.c +++ b/testcases/kernel/syscalls/ioperm/ioperm01.c @@ -15,6 +15,7 @@ #include <unistd.h> #include "tst_test.h" +#include "tst_secureboot.h" #if defined __i386__ || defined(__x86_64__) #include <sys/io.h> @@ -43,7 +44,7 @@ static void verify_ioperm(void) static void setup(void) { /* ioperm() is restricted under kernel lockdown. */ - if (tst_lockdown_enabled()) + if (tst_lockdown_enabled() || tst_secureboot_enabled() > 0) tst_brk(TCONF, "Kernel is locked down, skip this test"); /* diff --git a/testcases/kernel/syscalls/ioperm/ioperm02.c b/testcases/kernel/syscalls/ioperm/ioperm02.c index 1808191bf..129ca265c 100644 --- a/testcases/kernel/syscalls/ioperm/ioperm02.c +++ b/testcases/kernel/syscalls/ioperm/ioperm02.c @@ -22,6 +22,7 @@ #include <pwd.h> #include "tst_test.h" #include "tst_safe_macros.h" +#include "tst_secureboot.h" #if defined __i386__ || defined(__x86_64__) #include <sys/io.h> @@ -45,6 +46,10 @@ static struct tcase_t { static void setup(void) { + /* ioperm() is restricted under kernel lockdown. */ + if (tst_lockdown_enabled() || tst_secureboot_enabled() > 0) + tst_brk(TCONF, "Kernel is locked down, skip this test"); + /* * The value of IO_BITMAP_BITS (include/asm-i386/processor.h) changed * from kernel 2.6.8 to permit 16-bits (65536) ioperm diff --git a/testcases/kernel/syscalls/iopl/Makefile b/testcases/kernel/syscalls/iopl/Makefile index 044619fb8..8624e2c99 100644 --- a/testcases/kernel/syscalls/iopl/Makefile +++ b/testcases/kernel/syscalls/iopl/Makefile @@ -5,4 +5,7 @@ top_srcdir ?= ../../../.. include $(top_srcdir)/include/mk/testcases.mk +CFLAGS += $(EFIVAR_CFLAGS) +LDLIBS += $(EFIVAR_LIBS) + include $(top_srcdir)/include/mk/generic_leaf_target.mk diff --git a/testcases/kernel/syscalls/iopl/iopl01.c b/testcases/kernel/syscalls/iopl/iopl01.c index dcf2cc406..60fc529e8 100644 --- a/testcases/kernel/syscalls/iopl/iopl01.c +++ b/testcases/kernel/syscalls/iopl/iopl01.c @@ -18,6 +18,7 @@ #include <unistd.h> #include "tst_test.h" +#include "tst_secureboot.h" #if defined __i386__ || defined(__x86_64__) #include <sys/io.h> @@ -45,7 +46,7 @@ static void verify_iopl(void) static void setup(void) { /* iopl() is restricted under kernel lockdown. */ - if (tst_lockdown_enabled()) + if (tst_lockdown_enabled() || tst_secureboot_enabled() > 0) tst_brk(TCONF, "Kernel is locked down, skip this test"); } diff --git a/testcases/kernel/syscalls/iopl/iopl02.c b/testcases/kernel/syscalls/iopl/iopl02.c index 6a817cf2d..f27cfd098 100644 --- a/testcases/kernel/syscalls/iopl/iopl02.c +++ b/testcases/kernel/syscalls/iopl/iopl02.c @@ -21,6 +21,7 @@ #include <pwd.h> #include "tst_test.h" #include "tst_safe_macros.h" +#include "tst_secureboot.h" #if defined __i386__ || defined(__x86_64__) #include <sys/io.h> @@ -52,6 +53,11 @@ static void verify_iopl(unsigned int i) static void setup(void) { struct passwd *pw; + + /* ioperm() is restricted under kernel lockdown. */ + if (tst_lockdown_enabled() || tst_secureboot_enabled() > 0) + tst_brk(TCONF, "Kernel is locked down, skip this test"); + pw = SAFE_GETPWNAM("nobody"); SAFE_SETEUID(pw->pw_uid); }
SecureBoot implies integrity lockdown even if tst_lockdown_enabled() cannot check lockdown status directly. Udpate skip condition in ioperm() and iopl() tests. Signed-off-by: Martin Doucha <mdoucha@suse.cz> --- Changes since v2: - new patch testcases/kernel/syscalls/ioperm/Makefile | 3 +++ testcases/kernel/syscalls/ioperm/ioperm01.c | 3 ++- testcases/kernel/syscalls/ioperm/ioperm02.c | 5 +++++ testcases/kernel/syscalls/iopl/Makefile | 3 +++ testcases/kernel/syscalls/iopl/iopl01.c | 3 ++- testcases/kernel/syscalls/iopl/iopl02.c | 6 ++++++ 6 files changed, 21 insertions(+), 2 deletions(-)