Re: [RFC PATCH v7 14/16] scripts: add boot policy generation program

> From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > Sent: Wednesday, November 3, 2021 5:43 PM >> From: deven.desai(a)linux.microsoft.com >> [mailto:deven.desai@linux.microsoft.com] >> From: Deven Bowers <deven.desai(a)linux.microsoft.com&gt; >> >> Enables an IPE policy to be enforced from kernel start, enabling access >> control based on trust from kernel startup. This is accomplished by >> transforming an IPE policy indicated by CONFIG_IPE_BOOT_POLICY into a >> c-string literal that is parsed at kernel startup as an unsigned policy. >> >> Signed-off-by: Deven Bowers <deven.desai(a)linux.microsoft.com&gt; >> --- >> >> Relevant changes since v6: >> * Move patch 01/12 to [14/16] of the series >> >> --- >> MAINTAINERS | 1 + >> scripts/Makefile | 1 + >> scripts/ipe/Makefile | 2 + >> scripts/ipe/polgen/.gitignore | 1 + >> scripts/ipe/polgen/Makefile | 6 ++ >> scripts/ipe/polgen/polgen.c | 145 ++++++++++++++++++++++++++++++++++ >> security/ipe/.gitignore | 1 + >> security/ipe/Kconfig | 10 +++ >> security/ipe/Makefile | 13 +++ >> security/ipe/ctx.c | 18 +++++ >> 10 files changed, 198 insertions(+) >> create mode 100644 scripts/ipe/Makefile >> create mode 100644 scripts/ipe/polgen/.gitignore >> create mode 100644 scripts/ipe/polgen/Makefile >> create mode 100644 scripts/ipe/polgen/polgen.c >> create mode 100644 security/ipe/.gitignore >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index f1e76f791d47..a84ca781199b 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -9283,6 +9283,7 @@ INTEGRITY POLICY ENFORCEMENT (IPE) >> M: Deven Bowers <deven.desai(a)linux.microsoft.com&gt; >> M: Fan Wu <wufan(a)linux.microsoft.com&gt; >> S: Supported >> +F: scripts/ipe/ >> F: security/ipe/ >> >> INTEL 810/815 FRAMEBUFFER DRIVER >> diff --git a/scripts/Makefile b/scripts/Makefile >> index 9adb6d247818..a31da6d57a36 100644 >> --- a/scripts/Makefile >> +++ b/scripts/Makefile >> @@ -41,6 +41,7 @@ targets += module.lds >> subdir-$(CONFIG_GCC_PLUGINS) += gcc-plugins >> subdir-$(CONFIG_MODVERSIONS) += genksyms >> subdir-$(CONFIG_SECURITY_SELINUX) += selinux >> +subdir-$(CONFIG_SECURITY_IPE) += ipe >> >> # Let clean descend into subdirs >> subdir- += basic dtc gdb kconfig mod >> diff --git a/scripts/ipe/Makefile b/scripts/ipe/Makefile >> new file mode 100644 >> index 000000000000..e87553fbb8d6 >> --- /dev/null >> +++ b/scripts/ipe/Makefile >> @@ -0,0 +1,2 @@ >> +# SPDX-License-Identifier: GPL-2.0-only >> +subdir-y := polgen >> diff --git a/scripts/ipe/polgen/.gitignore b/scripts/ipe/polgen/.gitignore >> new file mode 100644 >> index 000000000000..80f32f25d200 >> --- /dev/null >> +++ b/scripts/ipe/polgen/.gitignore >> @@ -0,0 +1 @@ >> +polgen >> diff --git a/scripts/ipe/polgen/Makefile b/scripts/ipe/polgen/Makefile >> new file mode 100644 >> index 000000000000..066060c22b4a >> --- /dev/null >> +++ b/scripts/ipe/polgen/Makefile >> @@ -0,0 +1,6 @@ >> +# SPDX-License-Identifier: GPL-2.0 >> +hostprogs-always-y := polgen >> +HOST_EXTRACFLAGS += \ >> + -I$(srctree)/include \ >> + -I$(srctree)/include/uapi \ >> + >> diff --git a/scripts/ipe/polgen/polgen.c b/scripts/ipe/polgen/polgen.c >> new file mode 100644 >> index 000000000000..73cf13e743f7 >> --- /dev/null >> +++ b/scripts/ipe/polgen/polgen.c >> @@ -0,0 +1,145 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) Microsoft Corporation. All rights reserved. >> + */ >> + >> +#include <stdlib.h> >> +#include <stddef.h> >> +#include <stdio.h> >> +#include <unistd.h> >> +#include <errno.h> >> + >> +static void usage(const char *const name) >> +{ >> + printf("Usage: %s OutputFile (PolicyFile)\n", name); >> + exit(EINVAL); >> +} >> + >> +static int policy_to_buffer(const char *pathname, char **buffer, size_t *size) >> +{ >> + int rc = 0; >> + FILE *fd; >> + char *lbuf; >> + size_t fsize; >> + size_t read; >> + >> + fd = fopen(pathname, "r"); >> + if (!fd) { >> + rc = errno; >> + goto out; >> + } >> + >> + fseek(fd, 0, SEEK_END); >> + fsize = ftell(fd); >> + rewind(fd); >> + >> + lbuf = malloc(fsize); >> + if (!lbuf) { >> + rc = ENOMEM; >> + goto out_close; >> + } >> + >> + read = fread((void *)lbuf, sizeof(*lbuf), fsize, fd); >> + if (read != fsize) { >> + rc = -1; >> + goto out_free; >> + } >> + >> + *buffer = lbuf; >> + *size = fsize; >> + fclose(fd); >> + >> + return rc; >> + >> +out_free: >> + free(lbuf); >> +out_close: >> + fclose(fd); >> +out: >> + return rc; >> +} >> + >> +static int write_boot_policy(const char *pathname, const char *buf, size_t > size) >> +{ >> + int rc = 0; >> + FILE *fd; >> + size_t i; >> + >> + fd = fopen(pathname, "w"); >> + if (!fd) { >> + rc = errno; >> + goto err; >> + } >> + >> + fprintf(fd, "/* This file is automatically generated."); >> + fprintf(fd, " Do not edit. */\n"); >> + fprintf(fd, "#include <stddef.h>\n"); >> + fprintf(fd, "\nextern const char *const ipe_boot_policy;\n\n"); >> + fprintf(fd, "const char *const ipe_boot_policy =\n"); >> + >> + if (!buf || size == 0) { >> + fprintf(fd, "\tNULL;\n"); >> + fclose(fd); >> + return 0; >> + } >> + >> + fprintf(fd, "\t\""); >> + >> + for (i = 0; i < size; ++i) { >> + switch (buf[i]) { >> + case '"': >> + fprintf(fd, "\\\""); >> + break; >> + case '\'': >> + fprintf(fd, "'"); >> + break; >> + case '\n': >> + fprintf(fd, "\\n\"\n\t\""); >> + break; >> + case '\\': >> + fprintf(fd, "\\\\"); >> + break; >> + case '\t': >> + fprintf(fd, "\\t"); >> + break; >> + case '\?': >> + fprintf(fd, "\\?"); >> + break; >> + default: >> + fprintf(fd, "%c", buf[i]); >> + } >> + } >> + fprintf(fd, "\";\n"); >> + fclose(fd); >> + >> + return 0; >> + >> +err: >> + if (fd) >> + fclose(fd); >> + return rc; >> +} >> + >> +int main(int argc, const char *const argv[]) >> +{ >> + int rc = 0; >> + size_t len = 0; >> + char *policy = NULL; >> + >> + if (argc < 2) >> + usage(argv[0]); >> + >> + if (argc > 2) { >> + rc = policy_to_buffer(argv[2], &policy, &len); >> + if (rc != 0) >> + goto cleanup; >> + } >> + >> + rc = write_boot_policy(argv[1], policy, len); >> +cleanup: >> + if (policy) >> + free(policy); >> + if (rc != 0) >> + perror("An error occurred during policy conversion: "); >> + return rc; >> +} >> diff --git a/security/ipe/.gitignore b/security/ipe/.gitignore >> new file mode 100644 >> index 000000000000..eca22ad5ed22 >> --- /dev/null >> +++ b/security/ipe/.gitignore >> @@ -0,0 +1 @@ >> +boot-policy.c >> \ No newline at end of file >> diff --git a/security/ipe/Kconfig b/security/ipe/Kconfig >> index fcf82a8152ec..39df680b67a2 100644 >> --- a/security/ipe/Kconfig >> +++ b/security/ipe/Kconfig >> @@ -20,6 +20,16 @@ menuconfig SECURITY_IPE >> >> if SECURITY_IPE >> >> +config IPE_BOOT_POLICY >> + string "Integrity policy to apply on system startup" >> + help >> + This option specifies a filepath to a IPE policy that is compiled >> + into the kernel. This policy will be enforced until a policy update >> + is deployed via the $securityfs/ipe/policies/$policy_name/active >> + interface. >> + >> + If unsure, leave blank. >> + >> choice >> prompt "Hash algorithm used in auditing policies" >> default IPE_AUDIT_HASH_SHA1 >> diff --git a/security/ipe/Makefile b/security/ipe/Makefile >> index 1e7b2d7fcd9e..89fec670f954 100644 >> --- a/security/ipe/Makefile >> +++ b/security/ipe/Makefile >> @@ -7,7 +7,18 @@ >> >> ccflags-y := -I$(srctree)/security/ipe/modules >> >> +quiet_cmd_polgen = IPE_POL $(2) >> + cmd_polgen = scripts/ipe/polgen/polgen security/ipe/boot-policy.c $(2) >> + >> +$(eval $(call config_filename,IPE_BOOT_POLICY)) >> + >> +targets += boot-policy.c >> + >> +$(obj)/boot-policy.c: scripts/ipe/polgen/polgen >> $(IPE_BOOT_POLICY_FILENAME) FORCE >> + $(call if_changed,polgen,$(IPE_BOOT_POLICY_FILENAME)) >> + >> obj-$(CONFIG_SECURITY_IPE) += \ >> + boot-policy.o \ >> ctx.o \ >> eval.o \ >> fs.o \ >> @@ -21,3 +32,5 @@ obj-$(CONFIG_SECURITY_IPE) += \ >> policyfs.o \ >> >> obj-$(CONFIG_AUDIT) += audit.o >> + >> +clean-files := boot-policy.c \ >> diff --git a/security/ipe/ctx.c b/security/ipe/ctx.c >> index fc9b8e467bc9..879acf4ceac5 100644 >> --- a/security/ipe/ctx.c >> +++ b/security/ipe/ctx.c >> @@ -15,6 +15,7 @@ >> #include <linux/spinlock.h> >> #include <linux/moduleparam.h> >> >> +extern const char *const ipe_boot_policy; >> static bool success_audit; >> static bool enforce = true; >> >> @@ -329,6 +330,7 @@ void ipe_put_ctx(struct ipe_context *ctx) >> int __init ipe_init_ctx(void) >> { >> int rc = 0; >> + struct ipe_policy *p = NULL; >> struct ipe_context *lns = NULL; >> >> lns = create_ctx(); >> @@ -342,10 +344,26 @@ int __init ipe_init_ctx(void) >> WRITE_ONCE(lns->enforce, enforce); >> spin_unlock(&lns->lock); >> >> + if (ipe_boot_policy) { >> + p = ipe_new_policy(ipe_boot_policy, strlen(ipe_boot_policy), >> + NULL, 0); >> + if (IS_ERR(p)) { >> + rc = PTR_ERR(lns); > This should be: > > rc = PTR_ERR(p); > >> + goto err; >> + } >> + >> + ipe_add_policy(lns, p); >> + rc = ipe_set_active_pol(p); >> + if (!rc) > Here you need to set a non-zero value, so that ipe_init() > does not enable the LSM. Actually you probably should just check that rc is not zero and goto err. Roberto

> I would set to 1 a new global variable, like ipe_lsm_enabled, > in ipe_init() just before security_add_hooks(). > > Then, I would add a check of this variable in ipe_init_securityfs() > to avoid the kernel panic.