All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function
@ 2020-11-09 16:46 Martin Doucha
  2020-11-09 16:46 ` [LTP] [PATCH v3 2/2] syscalls/iopl, ioperm: Check for SecureBoot lockdown Martin Doucha
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Martin Doucha @ 2020-11-09 16:46 UTC (permalink / raw)
  To: ltp

Also check for SecureBoot status in tst_lockdown_enabled() if the lockdown
sysfile is not available/readable

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

Changes since v1:
- check whether machine is in EFI mode first

Changes since v2:
- move tst_secureboot_enabled() code to a separate header file
- move EFIVAR_CFLAGS and EFIVAR_LIBS out of global CFLAGS and LDLIBS

 configure.ac             |  1 +
 include/mk/config.mk.in  |  2 ++
 include/tst_lockdown.h   |  4 +++
 include/tst_secureboot.h | 53 ++++++++++++++++++++++++++++++++++++++++
 m4/ltp-libefivar.m4      |  9 +++++++
 5 files changed, 69 insertions(+)
 create mode 100644 include/tst_secureboot.h
 create mode 100644 m4/ltp-libefivar.m4

diff --git a/configure.ac b/configure.ac
index 03e4e09c9..d9ca5ad38 100644
--- a/configure.ac
+++ b/configure.ac
@@ -296,6 +296,7 @@ LTP_CHECK_CAPABILITY_SUPPORT
 LTP_CHECK_CC_WARN_OLDSTYLE
 LTP_CHECK_CLONE_SUPPORTS_7_ARGS
 LTP_CHECK_CRYPTO
+LTP_CHECK_EFIVAR
 LTP_CHECK_FORTIFY_SOURCE
 LTP_CHECK_KERNEL_DEVEL
 LTP_CHECK_KEYUTILS_SUPPORT
diff --git a/include/mk/config.mk.in b/include/mk/config.mk.in
index 427608a17..45105cdab 100644
--- a/include/mk/config.mk.in
+++ b/include/mk/config.mk.in
@@ -45,6 +45,8 @@ KEYUTILS_LIBS		:= @KEYUTILS_LIBS@
 HAVE_FTS_H		:= @HAVE_FTS_H@
 LIBMNL_LIBS		:= @LIBMNL_LIBS@
 LIBMNL_CFLAGS		:= @LIBMNL_CFLAGS@
+EFIVAR_CFLAGS		:= @EFIVAR_CFLAGS@
+EFIVAR_LIBS		:= @EFIVAR_LIBS@
 
 prefix			:= @prefix@
 
diff --git a/include/tst_lockdown.h b/include/tst_lockdown.h
index 78eaeccea..1c60151a9 100644
--- a/include/tst_lockdown.h
+++ b/include/tst_lockdown.h
@@ -5,6 +5,10 @@
 
 #define PATH_LOCKDOWN	"/sys/kernel/security/lockdown"
 
+/*
+ * Note: Also check tst_secureboot_enabled(). It may indicate integrity
+ * lockdown even if tst_lockdown_enabled() returns 0.
+ */
 int tst_lockdown_enabled(void);
 
 #endif /* TST_LOCKDOWN_H */
diff --git a/include/tst_secureboot.h b/include/tst_secureboot.h
new file mode 100644
index 000000000..70980a76a
--- /dev/null
+++ b/include/tst_secureboot.h
@@ -0,0 +1,53 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef TST_SECUREBOOT_H
+#define TST_SECUREBOOT_H
+
+#include "config.h"
+#include <stdlib.h>
+
+#ifdef HAVE_EFIVAR
+#include <efivar.h>
+#endif /* HAVE_EFIVAR */
+
+static inline int tst_secureboot_enabled(void)
+{
+#ifdef HAVE_EFIVAR
+	int ret, status = 0;
+	uint8_t *data = NULL;
+	size_t size = 0;
+	uint32_t attrs = 0;
+
+	if (!efi_variables_supported()) {
+		tst_res(TINFO, "SecureBoot: off (non-EFI system)");
+		return 0;
+	}
+
+	efi_error_clear();
+	ret = efi_get_variable(EFI_GLOBAL_GUID, "SecureBoot", &data, &size,
+		&attrs);
+
+	if (ret) {
+		char *fn, *func, *msg;
+		int ln, err, i = 0;
+
+		while (efi_error_get(i++, &fn, &func, &ln, &msg, &err) > 0)
+			tst_res(TINFO, "Efivar error: %s", msg);
+
+		efi_error_clear();
+	} else if (data) {
+		status = *data;
+		tst_res(TINFO, "SecureBoot: %s", status ? "on" : "off");
+	}
+
+	if (data)
+		free(data);
+
+	return status;
+#else /* HAVE_EFIVAR */
+	tst_res(TINFO, "%s(): LTP was built without efivar support", __func__);
+	return -1;
+#endif /* HAVE_EFIVAR */
+}
+
+#endif /* TST_SECUREBOOT_H */
diff --git a/m4/ltp-libefivar.m4 b/m4/ltp-libefivar.m4
new file mode 100644
index 000000000..0a2750701
--- /dev/null
+++ b/m4/ltp-libefivar.m4
@@ -0,0 +1,9 @@
+dnl SPDX-License-Identifier: GPL-2.0-or-later
+dnl Copyright (c) 2020 SUSE LLC <mdoucha@suse.cz>
+
+AC_DEFUN([LTP_CHECK_EFIVAR], [
+	dnl efivar library and headers
+	PKG_CHECK_MODULES([EFIVAR], [efivar], [
+		AC_DEFINE([HAVE_EFIVAR], [1], [Define to 1 if you have libefivar library and headers])
+	], [have_efivar=no])
+])
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 2/2] syscalls/iopl, ioperm: Check for SecureBoot lockdown
  2020-11-09 16:46 [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function Martin Doucha
@ 2020-11-09 16:46 ` Martin Doucha
  2020-11-10  5:54   ` Li Wang
  2020-11-10 11:51   ` Petr Vorel
  2020-11-10 11:49 ` [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function Petr Vorel
  2020-11-12 14:21 ` Cyril Hrubis
  2 siblings, 2 replies; 16+ messages in thread
From: Martin Doucha @ 2020-11-09 16:46 UTC (permalink / raw)
  To: ltp

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(-)

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);
 }
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 2/2] syscalls/iopl, ioperm: Check for SecureBoot lockdown
  2020-11-09 16:46 ` [LTP] [PATCH v3 2/2] syscalls/iopl, ioperm: Check for SecureBoot lockdown Martin Doucha
@ 2020-11-10  5:54   ` Li Wang
  2020-11-10  8:52     ` Cyril Hrubis
  2020-11-10 11:51   ` Petr Vorel
  1 sibling, 1 reply; 16+ messages in thread
From: Li Wang @ 2020-11-10  5:54 UTC (permalink / raw)
  To: ltp

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.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20201110/bdfdaaa4/attachment.htm>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 2/2] syscalls/iopl, ioperm: Check for SecureBoot lockdown
  2020-11-10  5:54   ` Li Wang
@ 2020-11-10  8:52     ` Cyril Hrubis
  2020-11-10  9:16       ` Li Wang
  2020-11-10 10:23       ` Petr Vorel
  0 siblings, 2 replies; 16+ messages in thread
From: Cyril Hrubis @ 2020-11-10  8:52 UTC (permalink / raw)
  To: ltp

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.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 2/2] syscalls/iopl, ioperm: Check for SecureBoot lockdown
  2020-11-10  8:52     ` Cyril Hrubis
@ 2020-11-10  9:16       ` Li Wang
  2020-11-10 10:23       ` Petr Vorel
  1 sibling, 0 replies; 16+ messages in thread
From: Li Wang @ 2020-11-10  9:16 UTC (permalink / raw)
  To: ltp

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>

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20201110/23fbd2f2/attachment.htm>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 2/2] syscalls/iopl, ioperm: Check for SecureBoot lockdown
  2020-11-10  8:52     ` Cyril Hrubis
  2020-11-10  9:16       ` Li Wang
@ 2020-11-10 10:23       ` Petr Vorel
  1 sibling, 0 replies; 16+ messages in thread
From: Petr Vorel @ 2020-11-10 10:23 UTC (permalink / raw)
  To: ltp

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function
  2020-11-09 16:46 [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function Martin Doucha
  2020-11-09 16:46 ` [LTP] [PATCH v3 2/2] syscalls/iopl, ioperm: Check for SecureBoot lockdown Martin Doucha
@ 2020-11-10 11:49 ` Petr Vorel
  2020-11-10 11:52   ` Martin Doucha
  2020-11-12 14:21 ` Cyril Hrubis
  2 siblings, 1 reply; 16+ messages in thread
From: Petr Vorel @ 2020-11-10 11:49 UTC (permalink / raw)
  To: ltp

> Also check for SecureBoot status in tst_lockdown_enabled() if the lockdown
> sysfile is not available/readable

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 2/2] syscalls/iopl, ioperm: Check for SecureBoot lockdown
  2020-11-09 16:46 ` [LTP] [PATCH v3 2/2] syscalls/iopl, ioperm: Check for SecureBoot lockdown Martin Doucha
  2020-11-10  5:54   ` Li Wang
@ 2020-11-10 11:51   ` Petr Vorel
  2020-11-10 11:55     ` Cyril Hrubis
  1 sibling, 1 reply; 16+ messages in thread
From: Petr Vorel @ 2020-11-10 11:51 UTC (permalink / raw)
  To: ltp

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function
  2020-11-10 11:49 ` [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function Petr Vorel
@ 2020-11-10 11:52   ` Martin Doucha
  2020-11-10 12:04     ` Petr Vorel
  0 siblings, 1 reply; 16+ messages in thread
From: Martin Doucha @ 2020-11-10 11:52 UTC (permalink / raw)
  To: ltp

On 10. 11. 20 12:49, Petr Vorel wrote:
>> Also check for SecureBoot status in tst_lockdown_enabled() if the lockdown
>> sysfile is not available/readable
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> Kind regards,
> Petr

Ah, yes, I guess that part of commit message should be dropped. Sorry
about that.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 2/2] syscalls/iopl, ioperm: Check for SecureBoot lockdown
  2020-11-10 11:51   ` Petr Vorel
@ 2020-11-10 11:55     ` Cyril Hrubis
  0 siblings, 0 replies; 16+ messages in thread
From: Cyril Hrubis @ 2020-11-10 11:55 UTC (permalink / raw)
  To: ltp

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.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function
  2020-11-10 11:52   ` Martin Doucha
@ 2020-11-10 12:04     ` Petr Vorel
  0 siblings, 0 replies; 16+ messages in thread
From: Petr Vorel @ 2020-11-10 12:04 UTC (permalink / raw)
  To: ltp

> On 10. 11. 20 12:49, Petr Vorel wrote:
> >> Also check for SecureBoot status in tst_lockdown_enabled() if the lockdown
> >> sysfile is not available/readable

> > Reviewed-by: Petr Vorel <pvorel@suse.cz>

> > Kind regards,
> > Petr

> Ah, yes, I guess that part of commit message should be dropped. Sorry
> about that.
np, maintainer will drop that.

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function
  2020-11-09 16:46 [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function Martin Doucha
  2020-11-09 16:46 ` [LTP] [PATCH v3 2/2] syscalls/iopl, ioperm: Check for SecureBoot lockdown Martin Doucha
  2020-11-10 11:49 ` [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function Petr Vorel
@ 2020-11-12 14:21 ` Cyril Hrubis
  2020-11-12 14:57   ` Martin Doucha
  2 siblings, 1 reply; 16+ messages in thread
From: Cyril Hrubis @ 2020-11-12 14:21 UTC (permalink / raw)
  To: ltp

Hi!
I've looked into the library and what it actually does in this case is
that it opens a sysfs file and reads a few bytes from there. I guess
that we can even avoid linking the library in this case, since we just
want to know a value of the single bit in the SecureBoot file.

The full path is:

/sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c

The SecureBoot is the name of the variable and the hex numbers
represends the global GUID.

Now on my system with secure boot disabled the content of the file looks
like:

cat /sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c  |xxd
00000000: 0600 0000 00                             .....

The first four bytes are attributes, we can ingore them and the last
byte is the data byte, which tells us if secure boot is enabled or not.

So it may be as well easier to:

* Check if that file exists

* Read five bytes and return the last one

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function
  2020-11-12 14:21 ` Cyril Hrubis
@ 2020-11-12 14:57   ` Martin Doucha
  2020-11-12 17:43     ` Petr Vorel
  2020-11-13 15:24     ` Cyril Hrubis
  0 siblings, 2 replies; 16+ messages in thread
From: Martin Doucha @ 2020-11-12 14:57 UTC (permalink / raw)
  To: ltp

On 12. 11. 20 15:21, Cyril Hrubis wrote:
> Hi!
> I've looked into the library and what it actually does in this case is
> that it opens a sysfs file and reads a few bytes from there. I guess
> that we can even avoid linking the library in this case, since we just
> want to know a value of the single bit in the SecureBoot file.
> 
> The full path is:
> 
> /sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c

Yes, we could read the sysfile directly. But do we want to deal with
potential compatibility issues and test breakage if the UEFI vars API
changes in the future? The binary format of those sysfiles is controlled
by the UEFI Forum, not by kernel devs. The efivars library is available
on basically all modern distros and we most likely won't do any
SecureBoot tests on distros that don't have it.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function
  2020-11-12 14:57   ` Martin Doucha
@ 2020-11-12 17:43     ` Petr Vorel
  2020-11-13  6:02       ` Li Wang
  2020-11-13 15:24     ` Cyril Hrubis
  1 sibling, 1 reply; 16+ messages in thread
From: Petr Vorel @ 2020-11-12 17:43 UTC (permalink / raw)
  To: ltp

Hi,

> On 12. 11. 20 15:21, Cyril Hrubis wrote:
> > Hi!
> > I've looked into the library and what it actually does in this case is
> > that it opens a sysfs file and reads a few bytes from there. I guess
> > that we can even avoid linking the library in this case, since we just
> > want to know a value of the single bit in the SecureBoot file.

> > The full path is:

> > /sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c

> Yes, we could read the sysfile directly. But do we want to deal with
> potential compatibility issues and test breakage if the UEFI vars API
> changes in the future? The binary format of those sysfiles is controlled
> by the UEFI Forum, not by kernel devs. The efivars library is available
> on basically all modern distros and we most likely won't do any
> SecureBoot tests on distros that don't have it.

I also don't see a big deal to use the efivars library.

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function
  2020-11-12 17:43     ` Petr Vorel
@ 2020-11-13  6:02       ` Li Wang
  0 siblings, 0 replies; 16+ messages in thread
From: Li Wang @ 2020-11-13  6:02 UTC (permalink / raw)
  To: ltp

On Fri, Nov 13, 2020 at 1:44 AM Petr Vorel <pvorel@suse.cz> wrote:

> Hi,
>
> > On 12. 11. 20 15:21, Cyril Hrubis wrote:
> > > Hi!
> > > I've looked into the library and what it actually does in this case is
> > > that it opens a sysfs file and reads a few bytes from there. I guess
> > > that we can even avoid linking the library in this case, since we just
> > > want to know a value of the single bit in the SecureBoot file.
>
> > > The full path is:
>
> > >
> /sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c
>
> > Yes, we could read the sysfile directly. But do we want to deal with
> > potential compatibility issues and test breakage if the UEFI vars API
> > changes in the future? The binary format of those sysfiles is controlled
> > by the UEFI Forum, not by kernel devs. The efivars library is available
> > on basically all modern distros and we most likely won't do any
> > SecureBoot tests on distros that don't have it.
>
> I also don't see a big deal to use the efivars library.
>

That's true. I have no objection to the patchset.

But we always try to avoid the LTP dependency on other libraries, in this
point, I agree with Cyril.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20201113/f273b39a/attachment.htm>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function
  2020-11-12 14:57   ` Martin Doucha
  2020-11-12 17:43     ` Petr Vorel
@ 2020-11-13 15:24     ` Cyril Hrubis
  1 sibling, 0 replies; 16+ messages in thread
From: Cyril Hrubis @ 2020-11-13 15:24 UTC (permalink / raw)
  To: ltp

Hi!
> > I've looked into the library and what it actually does in this case is
> > that it opens a sysfs file and reads a few bytes from there. I guess
> > that we can even avoid linking the library in this case, since we just
> > want to know a value of the single bit in the SecureBoot file.
> > 
> > The full path is:
> > 
> > /sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c
> 
> Yes, we could read the sysfile directly. But do we want to deal with
> potential compatibility issues and test breakage if the UEFI vars API
> changes in the future? The binary format of those sysfiles is controlled
> by the UEFI Forum, not by kernel devs. The efivars library is available
> on basically all modern distros and we most likely won't do any
> SecureBoot tests on distros that don't have it.

I do not see how the code that uses the library is actually better. If
the format changes you will need a newer UEFI library that will
presumbly handle the difference. Which is even worse than hardcoding the
stuff in LTP because the UEFI library has to be updated by a distribution.

In that case patching the code in LTP will be faster and work everywhere
and not only on distributions that are fast enough to update packages.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-11-13 15:24 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-09 16:46 [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function Martin Doucha
2020-11-09 16:46 ` [LTP] [PATCH v3 2/2] syscalls/iopl, ioperm: Check for SecureBoot lockdown Martin Doucha
2020-11-10  5:54   ` Li Wang
2020-11-10  8:52     ` Cyril Hrubis
2020-11-10  9:16       ` Li Wang
2020-11-10 10:23       ` Petr Vorel
2020-11-10 11:51   ` Petr Vorel
2020-11-10 11:55     ` Cyril Hrubis
2020-11-10 11:49 ` [LTP] [PATCH v3 1/2] Add tst_secureboot_enabled() helper function Petr Vorel
2020-11-10 11:52   ` Martin Doucha
2020-11-10 12:04     ` Petr Vorel
2020-11-12 14:21 ` Cyril Hrubis
2020-11-12 14:57   ` Martin Doucha
2020-11-12 17:43     ` Petr Vorel
2020-11-13  6:02       ` Li Wang
2020-11-13 15:24     ` Cyril Hrubis

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.