diff mbox series

[v6,2/3] Add tst_secureboot_enabled() helper function

Message ID 20210112095759.11910-2-mdoucha@suse.cz
State Accepted
Headers show
Series [v6,1/3] Add tst_kconfig_get() helper function | expand

Commit Message

Martin Doucha Jan. 12, 2021, 9:57 a.m. UTC
Also check for SecureBoot status in tst_lockdown_enabled() if the lockdown
sysfile is not available/readable and the kernel is configured to enable
lockdown automatically under SecureBoot.

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

Changes since v3:
- rewritten using direct read from /sys/ (without libefivar)

Changes since v5:
- fixed #includes

 include/tst_lockdown.h |  1 +
 lib/tst_lockdown.c     | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

Comments

Cyril Hrubis Jan. 14, 2021, 1:38 p.m. UTC | #1
Hi!
> +int tst_secureboot_enabled(void)
> +{
> +	int fd;
> +	char data[5];
> +
> +	if (access(EFIVAR_SECUREBOOT, F_OK)) {
> +		tst_res(TINFO, "Efivar FS not available");
> +		return -1;
> +	}
> +
> +	fd = open(EFIVAR_SECUREBOOT, O_RDONLY);
> +
> +	if (fd == -1) {
> +		tst_res(TINFO | TERRNO,
> +			"Cannot open SecureBoot Efivar sysfile");
> +		return -1;
> +	} else if (fd < 0) {
> +		tst_brk(TBROK | TERRNO, "Invalid open() return value %d", fd);
> +		return -1;
> +	}

If we change the access() above to R_OK we can just do SAFE_OPEN() here, right?

Other than this the code looks good.
Martin Doucha Jan. 14, 2021, 1:46 p.m. UTC | #2
On 14. 01. 21 14:38, Cyril Hrubis wrote:
> Hi!
>> +int tst_secureboot_enabled(void)
>> +{
>> +	int fd;
>> +	char data[5];
>> +
>> +	if (access(EFIVAR_SECUREBOOT, F_OK)) {
>> +		tst_res(TINFO, "Efivar FS not available");
>> +		return -1;
>> +	}
>> +
>> +	fd = open(EFIVAR_SECUREBOOT, O_RDONLY);
>> +
>> +	if (fd == -1) {
>> +		tst_res(TINFO | TERRNO,
>> +			"Cannot open SecureBoot Efivar sysfile");
>> +		return -1;
>> +	} else if (fd < 0) {
>> +		tst_brk(TBROK | TERRNO, "Invalid open() return value %d", fd);
>> +		return -1;
>> +	}
> 
> If we change the access() above to R_OK we can just do SAFE_OPEN() here, right?
> 
> Other than this the code looks good.

We could change it, but I'd prefer to know that the Efivar FS exists but
cannnot be open()ed due to missing permissions than to mix these two
states into the same vague info message.
Cyril Hrubis Jan. 14, 2021, 2:09 p.m. UTC | #3
Hi!
> We could change it, but I'd prefer to know that the Efivar FS exists but
> cannnot be open()ed due to missing permissions than to mix these two
> states into the same vague info message.

Ok. Pushed, thanks.
diff mbox series

Patch

diff --git a/include/tst_lockdown.h b/include/tst_lockdown.h
index 78eaeccea..172a7daf5 100644
--- a/include/tst_lockdown.h
+++ b/include/tst_lockdown.h
@@ -5,6 +5,7 @@ 
 
 #define PATH_LOCKDOWN	"/sys/kernel/security/lockdown"
 
+int tst_secureboot_enabled(void);
 int tst_lockdown_enabled(void);
 
 #endif /* TST_LOCKDOWN_H */
diff --git a/lib/tst_lockdown.c b/lib/tst_lockdown.c
index e7c19813c..cda7d7abd 100644
--- a/lib/tst_lockdown.c
+++ b/lib/tst_lockdown.c
@@ -10,6 +10,36 @@ 
 #include "tst_safe_macros.h"
 #include "tst_safe_stdio.h"
 #include "tst_lockdown.h"
+#include "tst_private.h"
+
+#define EFIVAR_SECUREBOOT "/sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c"
+
+int tst_secureboot_enabled(void)
+{
+	int fd;
+	char data[5];
+
+	if (access(EFIVAR_SECUREBOOT, F_OK)) {
+		tst_res(TINFO, "Efivar FS not available");
+		return -1;
+	}
+
+	fd = open(EFIVAR_SECUREBOOT, O_RDONLY);
+
+	if (fd == -1) {
+		tst_res(TINFO | TERRNO,
+			"Cannot open SecureBoot Efivar sysfile");
+		return -1;
+	} else if (fd < 0) {
+		tst_brk(TBROK | TERRNO, "Invalid open() return value %d", fd);
+		return -1;
+	}
+
+	SAFE_READ(1, fd, data, 5);
+	SAFE_CLOSE(fd);
+	tst_res(TINFO, "SecureBoot: %s", data[4] ? "on" : "off");
+	return data[4];
+}
 
 int tst_lockdown_enabled(void)
 {
@@ -17,6 +47,14 @@  int tst_lockdown_enabled(void)
 	FILE *file;
 
 	if (access(PATH_LOCKDOWN, F_OK) != 0) {
+		char flag;
+
+		flag = tst_kconfig_get("CONFIG_EFI_SECURE_BOOT_LOCK_DOWN");
+
+		/* SecureBoot enabled could mean integrity lockdown */
+		if (flag == 'y' && tst_secureboot_enabled() > 0)
+			return 1;
+
 		tst_res(TINFO, "Unable to determine system lockdown state");
 		return 0;
 	}