| Message ID | 20201219175713.18888-3-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Carry forward IMA measurement log on kexec on ARM64 | expand |
Hi Lakshmi, On Sat, 2020-12-19 at 09:57 -0800, Lakshmi Ramasubramanian wrote: > > diff --git a/arch/powerpc/kexec/Makefile b/arch/powerpc/kexec/Makefile > index 4aff6846c772..b6c52608cb49 100644 > --- a/arch/powerpc/kexec/Makefile > +++ b/arch/powerpc/kexec/Makefile > @@ -9,13 +9,6 @@ obj-$(CONFIG_PPC32) += relocate_32.o > > obj-$(CONFIG_KEXEC_FILE) += file_load.o ranges.o file_load_$(BITS).o elf_$(BITS).o > > -ifdef CONFIG_HAVE_IMA_KEXEC > -ifdef CONFIG_IMA > -obj-y += ima.o > -endif > -endif Notice how "kexec/ima.o" is only included if the architecture supports it and IMA is configured. In addition only if CONFIG_IMA_KEXEC is configured, is the IMA measurement list carried across kexec. After moving the rest of ima.c to drivers/of/kexec.c, this changes. Notice how drivers/of/Kconfig includes kexec.o: obj-$(CONFIG_KEXEC_FILE) += kexec.o It is not dependent on CONFIG_HAVE_IMA_KEXEC. Shouldn't all of the functions defined in ima.c being moved to kexec.o be defined within a CONFIG_HAVE_IMA_KEXEC ifdef? thanks, Mimi
On 12/22/20 6:26 AM, Mimi Zohar wrote: Hi Mimi, > > On Sat, 2020-12-19 at 09:57 -0800, Lakshmi Ramasubramanian wrote: >> >> diff --git a/arch/powerpc/kexec/Makefile b/arch/powerpc/kexec/Makefile >> index 4aff6846c772..b6c52608cb49 100644 >> --- a/arch/powerpc/kexec/Makefile >> +++ b/arch/powerpc/kexec/Makefile >> @@ -9,13 +9,6 @@ obj-$(CONFIG_PPC32) += relocate_32.o >> >> obj-$(CONFIG_KEXEC_FILE) += file_load.o ranges.o file_load_$(BITS).o elf_$(BITS).o >> >> -ifdef CONFIG_HAVE_IMA_KEXEC >> -ifdef CONFIG_IMA >> -obj-y += ima.o >> -endif >> -endif > > Notice how "kexec/ima.o" is only included if the architecture supports > it and IMA is configured. In addition only if CONFIG_IMA_KEXEC is > configured, is the IMA measurement list carried across kexec. After > moving the rest of ima.c to drivers/of/kexec.c, this changes. Notice > how drivers/of/Kconfig includes kexec.o: > > obj-$(CONFIG_KEXEC_FILE) += kexec.o > > It is not dependent on CONFIG_HAVE_IMA_KEXEC. Shouldn't all of the > functions defined in ima.c being moved to kexec.o be defined within a > CONFIG_HAVE_IMA_KEXEC ifdef? > Thanks for reviewing the changes. In "drivers/of/kexec.c" the function remove_ima_buffer() is defined under "#ifdef CONFIG_HAVE_IMA_KEXEC" setup_ima_buffer() is defined under "#ifdef CONFIG_IMA_KEXEC" - the same way it was defined in "arch/powerpc/kexec/ima.c". As you know, CONFIG_IMA_KEXEC depends on CONFIG_HAVE_IMA_KEXEC (as defined in "security/integrity/ima/Kconfig"). ima_get_kexec_buffer() and ima_free_kexec_buffer() are unconditionally defined in "drivers/of/kexec.c" even though they are called only when CONFIG_HAVE_IMA_KEXEC is enabled. I will update these two functions to be moved under "#ifdef CONFIG_HAVE_IMA_KEXEC" Rob/Mimi/Thiago - Please let me know if you have other comments in the v13 patches. Will address those as well and post v14. thanks, -lakshmi
On Tue, 2020-12-22 at 10:53 -0800, Lakshmi Ramasubramanian wrote: > On 12/22/20 6:26 AM, Mimi Zohar wrote: > > Hi Mimi, > > > > > On Sat, 2020-12-19 at 09:57 -0800, Lakshmi Ramasubramanian wrote: > >> > >> diff --git a/arch/powerpc/kexec/Makefile b/arch/powerpc/kexec/Makefile > >> index 4aff6846c772..b6c52608cb49 100644 > >> --- a/arch/powerpc/kexec/Makefile > >> +++ b/arch/powerpc/kexec/Makefile > >> @@ -9,13 +9,6 @@ obj-$(CONFIG_PPC32) += relocate_32.o > >> > >> obj-$(CONFIG_KEXEC_FILE) += file_load.o ranges.o file_load_$(BITS).o elf_$(BITS).o > >> > >> -ifdef CONFIG_HAVE_IMA_KEXEC > >> -ifdef CONFIG_IMA > >> -obj-y += ima.o > >> -endif > >> -endif > > > > Notice how "kexec/ima.o" is only included if the architecture supports > > it and IMA is configured. In addition only if CONFIG_IMA_KEXEC is > > configured, is the IMA measurement list carried across kexec. After > > moving the rest of ima.c to drivers/of/kexec.c, this changes. Notice > > how drivers/of/Kconfig includes kexec.o: > > > > obj-$(CONFIG_KEXEC_FILE) += kexec.o > > > > It is not dependent on CONFIG_HAVE_IMA_KEXEC. Shouldn't all of the > > functions defined in ima.c being moved to kexec.o be defined within a > > CONFIG_HAVE_IMA_KEXEC ifdef? > > > > Thanks for reviewing the changes. > > In "drivers/of/kexec.c" the function remove_ima_buffer() is defined > under "#ifdef CONFIG_HAVE_IMA_KEXEC" > > setup_ima_buffer() is defined under "#ifdef CONFIG_IMA_KEXEC" - the same > way it was defined in "arch/powerpc/kexec/ima.c". > > As you know, CONFIG_IMA_KEXEC depends on CONFIG_HAVE_IMA_KEXEC (as > defined in "security/integrity/ima/Kconfig"). > > ima_get_kexec_buffer() and ima_free_kexec_buffer() are unconditionally > defined in "drivers/of/kexec.c" even though they are called only when > CONFIG_HAVE_IMA_KEXEC is enabled. I will update these two functions to > be moved under "#ifdef CONFIG_HAVE_IMA_KEXEC" The issue is the reverse. CONFIG_HAVE_IMA_KEXEC may be enabled without CONFIG_IMA_KEXEC being enabled. This allows the architecture to support carrying the measurement list across kexec, but requires enabling it at build time. Only if CONFIG_HAVE_IMA_KEXEC is enabled should any of these functions be compiled at build. This allows restoring the previous IMA measurement list, even if CONFIG_IMA_KEXEC is not enabled. Only if CONFIG_IMA_KEXEC is enabled, should carrying the measurement list across kexec be enabled. See how arch_ima_add_kexec_buffer, write_number, setup_ima_buffer are ifdef'ed in arch/powerpc/kexec/ima.c. Mimi
On 12/22/20 11:45 AM, Mimi Zohar wrote: > On Tue, 2020-12-22 at 10:53 -0800, Lakshmi Ramasubramanian wrote: >> On 12/22/20 6:26 AM, Mimi Zohar wrote: >> >> Hi Mimi, >> >>> >>> On Sat, 2020-12-19 at 09:57 -0800, Lakshmi Ramasubramanian wrote: >>>> >>>> diff --git a/arch/powerpc/kexec/Makefile b/arch/powerpc/kexec/Makefile >>>> index 4aff6846c772..b6c52608cb49 100644 >>>> --- a/arch/powerpc/kexec/Makefile >>>> +++ b/arch/powerpc/kexec/Makefile >>>> @@ -9,13 +9,6 @@ obj-$(CONFIG_PPC32) += relocate_32.o >>>> >>>> obj-$(CONFIG_KEXEC_FILE) += file_load.o ranges.o file_load_$(BITS).o elf_$(BITS).o >>>> >>>> -ifdef CONFIG_HAVE_IMA_KEXEC >>>> -ifdef CONFIG_IMA >>>> -obj-y += ima.o >>>> -endif >>>> -endif >>> >>> Notice how "kexec/ima.o" is only included if the architecture supports >>> it and IMA is configured. In addition only if CONFIG_IMA_KEXEC is >>> configured, is the IMA measurement list carried across kexec. After >>> moving the rest of ima.c to drivers/of/kexec.c, this changes. Notice >>> how drivers/of/Kconfig includes kexec.o: >>> >>> obj-$(CONFIG_KEXEC_FILE) += kexec.o >>> >>> It is not dependent on CONFIG_HAVE_IMA_KEXEC. Shouldn't all of the >>> functions defined in ima.c being moved to kexec.o be defined within a >>> CONFIG_HAVE_IMA_KEXEC ifdef? >>> >> >> Thanks for reviewing the changes. >> >> In "drivers/of/kexec.c" the function remove_ima_buffer() is defined >> under "#ifdef CONFIG_HAVE_IMA_KEXEC" >> >> setup_ima_buffer() is defined under "#ifdef CONFIG_IMA_KEXEC" - the same >> way it was defined in "arch/powerpc/kexec/ima.c". >> >> As you know, CONFIG_IMA_KEXEC depends on CONFIG_HAVE_IMA_KEXEC (as >> defined in "security/integrity/ima/Kconfig"). >> >> ima_get_kexec_buffer() and ima_free_kexec_buffer() are unconditionally >> defined in "drivers/of/kexec.c" even though they are called only when >> CONFIG_HAVE_IMA_KEXEC is enabled. I will update these two functions to >> be moved under "#ifdef CONFIG_HAVE_IMA_KEXEC" > > The issue is the reverse. CONFIG_HAVE_IMA_KEXEC may be enabled without > CONFIG_IMA_KEXEC being enabled. This allows the architecture to > support carrying the measurement list across kexec, but requires > enabling it at build time. > > Only if CONFIG_HAVE_IMA_KEXEC is enabled should any of these functions > be compiled at build. This allows restoring the previous IMA > measurement list, even if CONFIG_IMA_KEXEC is not enabled. > > Only if CONFIG_IMA_KEXEC is enabled, should carrying the measurement > list across kexec be enabled. See how arch_ima_add_kexec_buffer, > write_number, setup_ima_buffer are ifdef'ed in > arch/powerpc/kexec/ima.c. > Yes - I agree. I will make the following changes: => Enable the functions moved from "arch/powerpc/kexec/ima.c" to "drivers/of/kexec.c" only when CONFIG_HAVE_IMA_KEXEC is enabled. => Also, compile write_number() and setup_ima_buffer() only when CONFIG_IMA_KEXEC is enabled. thanks, -lakshmi
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes: > The functions defined in "arch/powerpc/kexec/ima.c" handle setting up > and freeing the resources required to carry over the IMA measurement > list from the current kernel to the next kernel across kexec system call. > These functions do not have architecture specific code, but are > currently limited to powerpc. > > Move setup_ima_buffer() call into of_kexec_setup_new_fdt() defined in > "drivers/of/kexec.c". > > Move the remaining architecture independent functions from > "arch/powerpc/kexec/ima.c" to "drivers/of/kexec.c". > Delete "arch/powerpc/kexec/ima.c" and "arch/powerpc/include/asm/ima.h". > Remove references to the deleted files in powerpc and in ima. > > Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > --- > arch/powerpc/include/asm/ima.h | 27 ---- > arch/powerpc/kexec/Makefile | 7 - > arch/powerpc/kexec/file_load.c | 7 - > arch/powerpc/kexec/ima.c | 202 ------------------------- > drivers/of/kexec.c | 235 +++++++++++++++++++++++++++++ > include/linux/of.h | 2 + > security/integrity/ima/ima.h | 4 - > security/integrity/ima/ima_kexec.c | 1 + > 8 files changed, 238 insertions(+), 247 deletions(-) > delete mode 100644 arch/powerpc/include/asm/ima.h > delete mode 100644 arch/powerpc/kexec/ima.c This looks good, provided the changes from the discussion with Mimi are made. Also, minor nits below. > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index 6ebefec616e4..7c3947ad3773 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -24,10 +24,6 @@ > > #include "../integrity.h" > > -#ifdef CONFIG_HAVE_IMA_KEXEC > -#include <asm/ima.h> > -#endif > - > enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN, > IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII }; > enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 }; This belongs in patch 1. > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > index 38bcd7543e27..8a6712981dee 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -10,6 +10,7 @@ > #include <linux/seq_file.h> > #include <linux/vmalloc.h> > #include <linux/kexec.h> > +#include <linux/of.h> > #include <linux/ima.h> > #include "ima.h" This include isn't necessary.
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes: > On 12/22/20 11:45 AM, Mimi Zohar wrote: >> On Tue, 2020-12-22 at 10:53 -0800, Lakshmi Ramasubramanian wrote: >>> On 12/22/20 6:26 AM, Mimi Zohar wrote: >>> >>> Hi Mimi, >>> >>>> >>>> On Sat, 2020-12-19 at 09:57 -0800, Lakshmi Ramasubramanian wrote: >>>>> >>>>> diff --git a/arch/powerpc/kexec/Makefile b/arch/powerpc/kexec/Makefile >>>>> index 4aff6846c772..b6c52608cb49 100644 >>>>> --- a/arch/powerpc/kexec/Makefile >>>>> +++ b/arch/powerpc/kexec/Makefile >>>>> @@ -9,13 +9,6 @@ obj-$(CONFIG_PPC32) += relocate_32.o >>>>> obj-$(CONFIG_KEXEC_FILE) += file_load.o ranges.o >>>>> file_load_$(BITS).o elf_$(BITS).o >>>>> -ifdef CONFIG_HAVE_IMA_KEXEC >>>>> -ifdef CONFIG_IMA >>>>> -obj-y += ima.o >>>>> -endif >>>>> -endif >>>> >>>> Notice how "kexec/ima.o" is only included if the architecture supports >>>> it and IMA is configured. In addition only if CONFIG_IMA_KEXEC is >>>> configured, is the IMA measurement list carried across kexec. After >>>> moving the rest of ima.c to drivers/of/kexec.c, this changes. Notice >>>> how drivers/of/Kconfig includes kexec.o: >>>> >>>> obj-$(CONFIG_KEXEC_FILE) += kexec.o >>>> >>>> It is not dependent on CONFIG_HAVE_IMA_KEXEC. Shouldn't all of the >>>> functions defined in ima.c being moved to kexec.o be defined within a >>>> CONFIG_HAVE_IMA_KEXEC ifdef? >>>> >>> >>> Thanks for reviewing the changes. >>> >>> In "drivers/of/kexec.c" the function remove_ima_buffer() is defined >>> under "#ifdef CONFIG_HAVE_IMA_KEXEC" >>> >>> setup_ima_buffer() is defined under "#ifdef CONFIG_IMA_KEXEC" - the same >>> way it was defined in "arch/powerpc/kexec/ima.c". >>> >>> As you know, CONFIG_IMA_KEXEC depends on CONFIG_HAVE_IMA_KEXEC (as >>> defined in "security/integrity/ima/Kconfig"). >>> >>> ima_get_kexec_buffer() and ima_free_kexec_buffer() are unconditionally >>> defined in "drivers/of/kexec.c" even though they are called only when >>> CONFIG_HAVE_IMA_KEXEC is enabled. I will update these two functions to >>> be moved under "#ifdef CONFIG_HAVE_IMA_KEXEC" >> The issue is the reverse. CONFIG_HAVE_IMA_KEXEC may be enabled without >> CONFIG_IMA_KEXEC being enabled. This allows the architecture to >> support carrying the measurement list across kexec, but requires >> enabling it at build time. >> Only if CONFIG_HAVE_IMA_KEXEC is enabled should any of these functions >> be compiled at build. This allows restoring the previous IMA >> measurement list, even if CONFIG_IMA_KEXEC is not enabled. >> Only if CONFIG_IMA_KEXEC is enabled, should carrying the measurement >> list across kexec be enabled. See how arch_ima_add_kexec_buffer, >> write_number, setup_ima_buffer are ifdef'ed in >> arch/powerpc/kexec/ima.c. >> > > Yes - I agree. I will make the following changes: > > => Enable the functions moved from "arch/powerpc/kexec/ima.c" to > "drivers/of/kexec.c" only when CONFIG_HAVE_IMA_KEXEC is enabled. > > => Also, compile write_number() and setup_ima_buffer() only when > CONFIG_IMA_KEXEC is enabled. Sounds good, with one additional change: So far, CONFIG_HAVE_IMA_KEXEC was tested only in files that were built when CONFIG_IMA was set. With this series this is not the case anymore (in drivers/of/kexec.c). The simplest way to keep this consistent is to only enable CONFIG_HAVE_IMA_KEXEC if CONFIG_IMA is also set. For example, with this: diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index e9f13fe08492..4ddd17215ecf 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -548,7 +548,7 @@ config KEXEC config KEXEC_FILE bool "kexec file based system call" select KEXEC_CORE - select HAVE_IMA_KEXEC + select HAVE_IMA_KEXEC if IMA select BUILD_BIN2C select KEXEC_ELF depends on PPC64 And then the same thing on the arm64 patch.
Actually, I have one more comment on this patch: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes: > diff --git a/arch/powerpc/kexec/file_load.c b/arch/powerpc/kexec/file_load.c > index 956bcb2d1ec2..9f3ec0b239ef 100644 > --- a/arch/powerpc/kexec/file_load.c > +++ b/arch/powerpc/kexec/file_load.c > @@ -20,7 +20,6 @@ > #include <linux/of_fdt.h> > #include <linux/libfdt.h> > #include <asm/setup.h> > -#include <asm/ima.h> > > #define SLAVE_CODE_SIZE 256 /* First 0x100 bytes */ > > @@ -163,12 +162,6 @@ int setup_new_fdt(const struct kimage *image, void *fdt, > if (ret) > goto err; > > - ret = setup_ima_buffer(image, fdt, fdt_path_offset(fdt, "/chosen")); > - if (ret) { > - pr_err("Error setting up the new device tree.\n"); > - return ret; > - } > - > return 0; > > err: With this change, setup_new_fdt() is nothing more than a call to of_kexec_setup_new_fdt(). It should be removed, and its caller should call of_kexec_setup_new_fdt() directly. This change could be done in patch 4 of this series, to keep this patch simpler.
On 12/22/20 4:48 PM, Thiago Jung Bauermann wrote: > > Actually, I have one more comment on this patch: > > Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes: > >> diff --git a/arch/powerpc/kexec/file_load.c b/arch/powerpc/kexec/file_load.c >> index 956bcb2d1ec2..9f3ec0b239ef 100644 >> --- a/arch/powerpc/kexec/file_load.c >> +++ b/arch/powerpc/kexec/file_load.c >> @@ -20,7 +20,6 @@ >> #include <linux/of_fdt.h> >> #include <linux/libfdt.h> >> #include <asm/setup.h> >> -#include <asm/ima.h> >> >> #define SLAVE_CODE_SIZE 256 /* First 0x100 bytes */ >> >> @@ -163,12 +162,6 @@ int setup_new_fdt(const struct kimage *image, void *fdt, >> if (ret) >> goto err; >> >> - ret = setup_ima_buffer(image, fdt, fdt_path_offset(fdt, "/chosen")); >> - if (ret) { >> - pr_err("Error setting up the new device tree.\n"); >> - return ret; >> - } >> - >> return 0; >> >> err: > > With this change, setup_new_fdt() is nothing more than a call to > of_kexec_setup_new_fdt(). It should be removed, and its caller should > call of_kexec_setup_new_fdt() directly. > > This change could be done in patch 4 of this series, to keep this patch > simpler. > Sure Thiago - I will make that change. thanks, -lakshmi
On 12/22/20 4:19 PM, Thiago Jung Bauermann wrote: > > Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes: > >> The functions defined in "arch/powerpc/kexec/ima.c" handle setting up >> and freeing the resources required to carry over the IMA measurement >> list from the current kernel to the next kernel across kexec system call. >> These functions do not have architecture specific code, but are >> currently limited to powerpc. >> >> Move setup_ima_buffer() call into of_kexec_setup_new_fdt() defined in >> "drivers/of/kexec.c". >> >> Move the remaining architecture independent functions from >> "arch/powerpc/kexec/ima.c" to "drivers/of/kexec.c". >> Delete "arch/powerpc/kexec/ima.c" and "arch/powerpc/include/asm/ima.h". >> Remove references to the deleted files in powerpc and in ima. >> >> Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> >> --- >> arch/powerpc/include/asm/ima.h | 27 ---- >> arch/powerpc/kexec/Makefile | 7 - >> arch/powerpc/kexec/file_load.c | 7 - >> arch/powerpc/kexec/ima.c | 202 ------------------------- >> drivers/of/kexec.c | 235 +++++++++++++++++++++++++++++ >> include/linux/of.h | 2 + >> security/integrity/ima/ima.h | 4 - >> security/integrity/ima/ima_kexec.c | 1 + >> 8 files changed, 238 insertions(+), 247 deletions(-) >> delete mode 100644 arch/powerpc/include/asm/ima.h >> delete mode 100644 arch/powerpc/kexec/ima.c > > This looks good, provided the changes from the discussion with Mimi are > made. Also, minor nits below. I will address the changes Mimi had stated. > >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h >> index 6ebefec616e4..7c3947ad3773 100644 >> --- a/security/integrity/ima/ima.h >> +++ b/security/integrity/ima/ima.h >> @@ -24,10 +24,6 @@ >> >> #include "../integrity.h" >> >> -#ifdef CONFIG_HAVE_IMA_KEXEC >> -#include <asm/ima.h> >> -#endif >> - >> enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN, >> IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII }; >> enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 }; > > This belongs in patch 1. No - the reference to "asm/ima.h" cannot be removed in Patch #1 since ima_get_kexec_buffer() and ima_free_kexec_buffer() are still declared in this header. They are moved in this patch only (Patch #2). >> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c >> index 38bcd7543e27..8a6712981dee 100644 >> --- a/security/integrity/ima/ima_kexec.c >> +++ b/security/integrity/ima/ima_kexec.c >> @@ -10,6 +10,7 @@ >> #include <linux/seq_file.h> >> #include <linux/vmalloc.h> >> #include <linux/kexec.h> >> +#include <linux/of.h> >> #include <linux/ima.h> >> #include "ima.h" > > This include isn't necessary. This change is necessary because ima_get_kexec_buffer() and ima_free_kexec_buffer() are now declared in "linux/of.h". -lakshmi
On 12/22/20 4:40 PM, Thiago Jung Bauermann wrote: > > Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes: > >> On 12/22/20 11:45 AM, Mimi Zohar wrote: >>> On Tue, 2020-12-22 at 10:53 -0800, Lakshmi Ramasubramanian wrote: >>>> On 12/22/20 6:26 AM, Mimi Zohar wrote: >>>> >>>> Hi Mimi, >>>> >>>>> >>>>> On Sat, 2020-12-19 at 09:57 -0800, Lakshmi Ramasubramanian wrote: >>>>>> >>>>>> diff --git a/arch/powerpc/kexec/Makefile b/arch/powerpc/kexec/Makefile >>>>>> index 4aff6846c772..b6c52608cb49 100644 >>>>>> --- a/arch/powerpc/kexec/Makefile >>>>>> +++ b/arch/powerpc/kexec/Makefile >>>>>> @@ -9,13 +9,6 @@ obj-$(CONFIG_PPC32) += relocate_32.o >>>>>> obj-$(CONFIG_KEXEC_FILE) += file_load.o ranges.o >>>>>> file_load_$(BITS).o elf_$(BITS).o >>>>>> -ifdef CONFIG_HAVE_IMA_KEXEC >>>>>> -ifdef CONFIG_IMA >>>>>> -obj-y += ima.o >>>>>> -endif >>>>>> -endif >>>>> >>>>> Notice how "kexec/ima.o" is only included if the architecture supports >>>>> it and IMA is configured. In addition only if CONFIG_IMA_KEXEC is >>>>> configured, is the IMA measurement list carried across kexec. After >>>>> moving the rest of ima.c to drivers/of/kexec.c, this changes. Notice >>>>> how drivers/of/Kconfig includes kexec.o: >>>>> >>>>> obj-$(CONFIG_KEXEC_FILE) += kexec.o >>>>> >>>>> It is not dependent on CONFIG_HAVE_IMA_KEXEC. Shouldn't all of the >>>>> functions defined in ima.c being moved to kexec.o be defined within a >>>>> CONFIG_HAVE_IMA_KEXEC ifdef? >>>>> >>>> >>>> Thanks for reviewing the changes. >>>> >>>> In "drivers/of/kexec.c" the function remove_ima_buffer() is defined >>>> under "#ifdef CONFIG_HAVE_IMA_KEXEC" >>>> >>>> setup_ima_buffer() is defined under "#ifdef CONFIG_IMA_KEXEC" - the same >>>> way it was defined in "arch/powerpc/kexec/ima.c". >>>> >>>> As you know, CONFIG_IMA_KEXEC depends on CONFIG_HAVE_IMA_KEXEC (as >>>> defined in "security/integrity/ima/Kconfig"). >>>> >>>> ima_get_kexec_buffer() and ima_free_kexec_buffer() are unconditionally >>>> defined in "drivers/of/kexec.c" even though they are called only when >>>> CONFIG_HAVE_IMA_KEXEC is enabled. I will update these two functions to >>>> be moved under "#ifdef CONFIG_HAVE_IMA_KEXEC" >>> The issue is the reverse. CONFIG_HAVE_IMA_KEXEC may be enabled without >>> CONFIG_IMA_KEXEC being enabled. This allows the architecture to >>> support carrying the measurement list across kexec, but requires >>> enabling it at build time. >>> Only if CONFIG_HAVE_IMA_KEXEC is enabled should any of these functions >>> be compiled at build. This allows restoring the previous IMA >>> measurement list, even if CONFIG_IMA_KEXEC is not enabled. >>> Only if CONFIG_IMA_KEXEC is enabled, should carrying the measurement >>> list across kexec be enabled. See how arch_ima_add_kexec_buffer, >>> write_number, setup_ima_buffer are ifdef'ed in >>> arch/powerpc/kexec/ima.c. >>> >> >> Yes - I agree. I will make the following changes: >> >> => Enable the functions moved from "arch/powerpc/kexec/ima.c" to >> "drivers/of/kexec.c" only when CONFIG_HAVE_IMA_KEXEC is enabled. >> >> => Also, compile write_number() and setup_ima_buffer() only when >> CONFIG_IMA_KEXEC is enabled. > > Sounds good, with one additional change: > > So far, CONFIG_HAVE_IMA_KEXEC was tested only in files that were built > when CONFIG_IMA was set. With this series this is not the case anymore > (in drivers/of/kexec.c). The simplest way to keep this consistent is to > only enable CONFIG_HAVE_IMA_KEXEC if CONFIG_IMA is also set. > > For example, with this: > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index e9f13fe08492..4ddd17215ecf 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -548,7 +548,7 @@ config KEXEC > config KEXEC_FILE > bool "kexec file based system call" > select KEXEC_CORE > - select HAVE_IMA_KEXEC > + select HAVE_IMA_KEXEC if IMA > select BUILD_BIN2C > select KEXEC_ELF > depends on PPC64 > > And then the same thing on the arm64 patch. This is a good idea Thiago - I will make this change in the Kconfig for both powerpc and arm64. thanks, -lakshmi
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes: > On 12/22/20 4:19 PM, Thiago Jung Bauermann wrote: >> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes: >> >>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h >>> index 6ebefec616e4..7c3947ad3773 100644 >>> --- a/security/integrity/ima/ima.h >>> +++ b/security/integrity/ima/ima.h >>> @@ -24,10 +24,6 @@ >>> #include "../integrity.h" >>> -#ifdef CONFIG_HAVE_IMA_KEXEC >>> -#include <asm/ima.h> >>> -#endif >>> - >>> enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN, >>> IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII }; >>> enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 }; >> This belongs in patch 1. > > No - the reference to "asm/ima.h" cannot be removed in Patch #1 since > ima_get_kexec_buffer() and ima_free_kexec_buffer() are still declared in > this header. They are moved in this patch only (Patch #2). Indeed, you are right. My mistake. >>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c >>> index 38bcd7543e27..8a6712981dee 100644 >>> --- a/security/integrity/ima/ima_kexec.c >>> +++ b/security/integrity/ima/ima_kexec.c >>> @@ -10,6 +10,7 @@ >>> #include <linux/seq_file.h> >>> #include <linux/vmalloc.h> >>> #include <linux/kexec.h> >>> +#include <linux/of.h> >>> #include <linux/ima.h> >>> #include "ima.h" >> This include isn't necessary. > > This change is necessary because ima_get_kexec_buffer() and > ima_free_kexec_buffer() are now declared in "linux/of.h". You are right here as well. Before I made the suggestion, I had tested building the kernel without the include above and it worked fine, but that's because <linux/of.h> is being included indirectly by some other header file. It's better to include it explicitly.
On Sat, Dec 19, 2020 at 09:57:09AM -0800, Lakshmi Ramasubramanian wrote: > The functions defined in "arch/powerpc/kexec/ima.c" handle setting up > and freeing the resources required to carry over the IMA measurement > list from the current kernel to the next kernel across kexec system call. > These functions do not have architecture specific code, but are > currently limited to powerpc. > > Move setup_ima_buffer() call into of_kexec_setup_new_fdt() defined in > "drivers/of/kexec.c". > > Move the remaining architecture independent functions from > "arch/powerpc/kexec/ima.c" to "drivers/of/kexec.c". > Delete "arch/powerpc/kexec/ima.c" and "arch/powerpc/include/asm/ima.h". > Remove references to the deleted files in powerpc and in ima. > > Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > --- > arch/powerpc/include/asm/ima.h | 27 ---- > arch/powerpc/kexec/Makefile | 7 - > arch/powerpc/kexec/file_load.c | 7 - > arch/powerpc/kexec/ima.c | 202 ------------------------- > drivers/of/kexec.c | 235 +++++++++++++++++++++++++++++ > include/linux/of.h | 2 + > security/integrity/ima/ima.h | 4 - > security/integrity/ima/ima_kexec.c | 1 + > 8 files changed, 238 insertions(+), 247 deletions(-) > delete mode 100644 arch/powerpc/include/asm/ima.h > delete mode 100644 arch/powerpc/kexec/ima.c > diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c > index 66787be081fe..33d97106f176 100644 > --- a/drivers/of/kexec.c > +++ b/drivers/of/kexec.c > @@ -11,6 +11,7 @@ > > #include <linux/kernel.h> > #include <linux/kexec.h> > +#include <linux/memblock.h> > #include <linux/libfdt.h> > #include <linux/of.h> > #include <linux/of_fdt.h> > @@ -59,6 +60,181 @@ static int fdt_find_and_del_mem_rsv(void *fdt, unsigned long start, unsigned lon > return -ENOENT; > } > > +/** > + * get_addr_size_cells - Get address and size of root node > + * > + * @addr_cells: Return address of the root node > + * @size_cells: Return size of the root node > + * > + * Return: 0 on success, or negative errno on error. > + */ > +static int get_addr_size_cells(int *addr_cells, int *size_cells) > +{ > + struct device_node *root; > + > + root = of_find_node_by_path("/"); > + if (!root) > + return -EINVAL; > + > + *addr_cells = of_n_addr_cells(root); > + *size_cells = of_n_size_cells(root); > + > + of_node_put(root); > + > + return 0; > +} > + > +/** > + * do_get_kexec_buffer - Get address and size of device tree property > + * > + * @prop: Device tree property > + * @len: Size of @prop > + * @addr: Return address of the node > + * @size: Return size of the node > + * > + * Return: 0 on success, or negative errno on error. > + */ > +static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr, > + size_t *size) > +{ > + int ret, addr_cells, size_cells; > + > + ret = get_addr_size_cells(&addr_cells, &size_cells); > + if (ret) > + return ret; > + > + if (len < 4 * (addr_cells + size_cells)) > + return -ENOENT; > + > + *addr = of_read_number(prop, addr_cells); > + *size = of_read_number(prop + 4 * addr_cells, size_cells); > + > + return 0; > +} > + > +#ifdef CONFIG_HAVE_IMA_KEXEC > +/** > + * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt > + * > + * @fdt: Flattened Device Tree to update > + * @chosen_node: Offset to the chosen node in the device tree > + * > + * The IMA measurement buffer is of no use to a subsequent kernel, so we always > + * remove it from the device tree. > + */ > +static void remove_ima_buffer(void *fdt, int chosen_node) > +{ > + int ret, len; > + unsigned long addr; > + size_t size; > + const void *prop; > + Should be able to do this instead of #ifdef: if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC)) return; Otherwise, I think it looks good. Rob
diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h deleted file mode 100644 index 51f64fd06c19..000000000000 --- a/arch/powerpc/include/asm/ima.h +++ /dev/null @@ -1,27 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 */ -#ifndef _ASM_POWERPC_IMA_H -#define _ASM_POWERPC_IMA_H - -struct kimage; - -int ima_get_kexec_buffer(void **addr, size_t *size); -int ima_free_kexec_buffer(void); - -#ifdef CONFIG_IMA -void remove_ima_buffer(void *fdt, int chosen_node); -#else -static inline void remove_ima_buffer(void *fdt, int chosen_node) {} -#endif - -#ifdef CONFIG_IMA_KEXEC -int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node); -#else -static inline int setup_ima_buffer(const struct kimage *image, void *fdt, - int chosen_node) -{ - remove_ima_buffer(fdt, chosen_node); - return 0; -} -#endif /* CONFIG_IMA_KEXEC */ - -#endif /* _ASM_POWERPC_IMA_H */ diff --git a/arch/powerpc/kexec/Makefile b/arch/powerpc/kexec/Makefile index 4aff6846c772..b6c52608cb49 100644 --- a/arch/powerpc/kexec/Makefile +++ b/arch/powerpc/kexec/Makefile @@ -9,13 +9,6 @@ obj-$(CONFIG_PPC32) += relocate_32.o obj-$(CONFIG_KEXEC_FILE) += file_load.o ranges.o file_load_$(BITS).o elf_$(BITS).o -ifdef CONFIG_HAVE_IMA_KEXEC -ifdef CONFIG_IMA -obj-y += ima.o -endif -endif - - # Disable GCOV, KCOV & sanitizers in odd or sensitive code GCOV_PROFILE_core_$(BITS).o := n KCOV_INSTRUMENT_core_$(BITS).o := n diff --git a/arch/powerpc/kexec/file_load.c b/arch/powerpc/kexec/file_load.c index 956bcb2d1ec2..9f3ec0b239ef 100644 --- a/arch/powerpc/kexec/file_load.c +++ b/arch/powerpc/kexec/file_load.c @@ -20,7 +20,6 @@ #include <linux/of_fdt.h> #include <linux/libfdt.h> #include <asm/setup.h> -#include <asm/ima.h> #define SLAVE_CODE_SIZE 256 /* First 0x100 bytes */ @@ -163,12 +162,6 @@ int setup_new_fdt(const struct kimage *image, void *fdt, if (ret) goto err; - ret = setup_ima_buffer(image, fdt, fdt_path_offset(fdt, "/chosen")); - if (ret) { - pr_err("Error setting up the new device tree.\n"); - return ret; - } - return 0; err: diff --git a/arch/powerpc/kexec/ima.c b/arch/powerpc/kexec/ima.c deleted file mode 100644 index 7378d59c0c1e..000000000000 --- a/arch/powerpc/kexec/ima.c +++ /dev/null @@ -1,202 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-or-later -/* - * Copyright (C) 2016 IBM Corporation - * - * Authors: - * Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> - */ - -#include <linux/slab.h> -#include <linux/kexec.h> -#include <linux/of.h> -#include <linux/memblock.h> -#include <linux/libfdt.h> - -static int get_addr_size_cells(int *addr_cells, int *size_cells) -{ - struct device_node *root; - - root = of_find_node_by_path("/"); - if (!root) - return -EINVAL; - - *addr_cells = of_n_addr_cells(root); - *size_cells = of_n_size_cells(root); - - of_node_put(root); - - return 0; -} - -static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr, - size_t *size) -{ - int ret, addr_cells, size_cells; - - ret = get_addr_size_cells(&addr_cells, &size_cells); - if (ret) - return ret; - - if (len < 4 * (addr_cells + size_cells)) - return -ENOENT; - - *addr = of_read_number(prop, addr_cells); - *size = of_read_number(prop + 4 * addr_cells, size_cells); - - return 0; -} - -/** - * ima_get_kexec_buffer - get IMA buffer from the previous kernel - * @addr: On successful return, set to point to the buffer contents. - * @size: On successful return, set to the buffer size. - * - * Return: 0 on success, negative errno on error. - */ -int ima_get_kexec_buffer(void **addr, size_t *size) -{ - int ret, len; - unsigned long tmp_addr; - size_t tmp_size; - const void *prop; - - prop = of_get_property(of_chosen, "linux,ima-kexec-buffer", &len); - if (!prop) - return -ENOENT; - - ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size); - if (ret) - return ret; - - *addr = __va(tmp_addr); - *size = tmp_size; - - return 0; -} - -/** - * ima_free_kexec_buffer - free memory used by the IMA buffer - */ -int ima_free_kexec_buffer(void) -{ - int ret; - unsigned long addr; - size_t size; - struct property *prop; - - prop = of_find_property(of_chosen, "linux,ima-kexec-buffer", NULL); - if (!prop) - return -ENOENT; - - ret = do_get_kexec_buffer(prop->value, prop->length, &addr, &size); - if (ret) - return ret; - - ret = of_remove_property(of_chosen, prop); - if (ret) - return ret; - - return memblock_free(addr, size); - -} - -/** - * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt - * - * The IMA measurement buffer is of no use to a subsequent kernel, so we always - * remove it from the device tree. - */ -void remove_ima_buffer(void *fdt, int chosen_node) -{ - int ret, len; - unsigned long addr; - size_t size; - const void *prop; - - prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len); - if (!prop) - return; - - ret = do_get_kexec_buffer(prop, len, &addr, &size); - fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer"); - if (ret) - return; - - ret = delete_fdt_mem_rsv(fdt, addr, size); - if (!ret) - pr_debug("Removed old IMA buffer reservation.\n"); -} - -#ifdef CONFIG_IMA_KEXEC -static int write_number(void *p, u64 value, int cells) -{ - if (cells == 1) { - u32 tmp; - - if (value > U32_MAX) - return -EINVAL; - - tmp = cpu_to_be32(value); - memcpy(p, &tmp, sizeof(tmp)); - } else if (cells == 2) { - u64 tmp; - - tmp = cpu_to_be64(value); - memcpy(p, &tmp, sizeof(tmp)); - } else - return -EINVAL; - - return 0; -} - -/** - * setup_ima_buffer - add IMA buffer information to the fdt - * @image: kexec image being loaded. - * @fdt: Flattened device tree for the next kernel. - * @chosen_node: Offset to the chosen node. - * - * Return: 0 on success, or negative errno on error. - */ -int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node) -{ - int ret, addr_cells, size_cells, entry_size; - u8 value[16]; - - remove_ima_buffer(fdt, chosen_node); - if (!image->arch.ima_buffer_size) - return 0; - - ret = get_addr_size_cells(&addr_cells, &size_cells); - if (ret) - return ret; - - entry_size = 4 * (addr_cells + size_cells); - - if (entry_size > sizeof(value)) - return -EINVAL; - - ret = write_number(value, image->arch.ima_buffer_addr, addr_cells); - if (ret) - return ret; - - ret = write_number(value + 4 * addr_cells, image->arch.ima_buffer_size, - size_cells); - if (ret) - return ret; - - ret = fdt_setprop(fdt, chosen_node, "linux,ima-kexec-buffer", value, - entry_size); - if (ret < 0) - return -EINVAL; - - ret = fdt_add_mem_rsv(fdt, image->arch.ima_buffer_addr, - image->arch.ima_buffer_size); - if (ret) - return -EINVAL; - - pr_debug("IMA buffer at 0x%llx, size = 0x%zx\n", - image->arch.ima_buffer_addr, image->arch.ima_buffer_size); - - return 0; -} -#endif /* CONFIG_IMA_KEXEC */ diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c index 66787be081fe..33d97106f176 100644 --- a/drivers/of/kexec.c +++ b/drivers/of/kexec.c @@ -11,6 +11,7 @@ #include <linux/kernel.h> #include <linux/kexec.h> +#include <linux/memblock.h> #include <linux/libfdt.h> #include <linux/of.h> #include <linux/of_fdt.h> @@ -59,6 +60,181 @@ static int fdt_find_and_del_mem_rsv(void *fdt, unsigned long start, unsigned lon return -ENOENT; } +/** + * get_addr_size_cells - Get address and size of root node + * + * @addr_cells: Return address of the root node + * @size_cells: Return size of the root node + * + * Return: 0 on success, or negative errno on error. + */ +static int get_addr_size_cells(int *addr_cells, int *size_cells) +{ + struct device_node *root; + + root = of_find_node_by_path("/"); + if (!root) + return -EINVAL; + + *addr_cells = of_n_addr_cells(root); + *size_cells = of_n_size_cells(root); + + of_node_put(root); + + return 0; +} + +/** + * do_get_kexec_buffer - Get address and size of device tree property + * + * @prop: Device tree property + * @len: Size of @prop + * @addr: Return address of the node + * @size: Return size of the node + * + * Return: 0 on success, or negative errno on error. + */ +static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr, + size_t *size) +{ + int ret, addr_cells, size_cells; + + ret = get_addr_size_cells(&addr_cells, &size_cells); + if (ret) + return ret; + + if (len < 4 * (addr_cells + size_cells)) + return -ENOENT; + + *addr = of_read_number(prop, addr_cells); + *size = of_read_number(prop + 4 * addr_cells, size_cells); + + return 0; +} + +#ifdef CONFIG_HAVE_IMA_KEXEC +/** + * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt + * + * @fdt: Flattened Device Tree to update + * @chosen_node: Offset to the chosen node in the device tree + * + * The IMA measurement buffer is of no use to a subsequent kernel, so we always + * remove it from the device tree. + */ +static void remove_ima_buffer(void *fdt, int chosen_node) +{ + int ret, len; + unsigned long addr; + size_t size; + const void *prop; + + prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len); + if (!prop) + return; + + ret = do_get_kexec_buffer(prop, len, &addr, &size); + fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer"); + if (ret) + return; + + ret = fdt_find_and_del_mem_rsv(fdt, addr, size); + if (!ret) + pr_debug("Removed old IMA buffer reservation.\n"); +} +#else /* CONFIG_HAVE_IMA_KEXEC */ +static inline void remove_ima_buffer(void *fdt, int chosen_node) {} +#endif /* CONFIG_HAVE_IMA_KEXEC */ + +#ifdef CONFIG_IMA_KEXEC +/** + * write_number - Convert number to big-endian format + * + * @p: Buffer to write the number to + * @value: Number to convert + * @cells: Number of cells + * + * Return: 0 on success, or negative errno on error. + */ +static int write_number(void *p, u64 value, int cells) +{ + if (cells == 1) { + u32 tmp; + + if (value > U32_MAX) + return -EINVAL; + + tmp = cpu_to_be32(value); + memcpy(p, &tmp, sizeof(tmp)); + } else if (cells == 2) { + u64 tmp; + + tmp = cpu_to_be64(value); + memcpy(p, &tmp, sizeof(tmp)); + } else + return -EINVAL; + + return 0; +} + +/** + * setup_ima_buffer - add IMA buffer information to the fdt + * @image: kexec image being loaded. + * @fdt: Flattened device tree for the next kernel. + * @chosen_node: Offset to the chosen node. + * + * Return: 0 on success, or negative errno on error. + */ +static int setup_ima_buffer(const struct kimage *image, void *fdt, + int chosen_node) +{ + int ret, addr_cells, size_cells, entry_size; + u8 value[16]; + + if (!image->arch.ima_buffer_size) + return 0; + + ret = get_addr_size_cells(&addr_cells, &size_cells); + if (ret) + return ret; + + entry_size = 4 * (addr_cells + size_cells); + + if (entry_size > sizeof(value)) + return -EINVAL; + + ret = write_number(value, image->arch.ima_buffer_addr, addr_cells); + if (ret) + return ret; + + ret = write_number(value + 4 * addr_cells, image->arch.ima_buffer_size, + size_cells); + if (ret) + return ret; + + ret = fdt_setprop(fdt, chosen_node, "linux,ima-kexec-buffer", value, + entry_size); + if (ret < 0) + return -EINVAL; + + ret = fdt_add_mem_rsv(fdt, image->arch.ima_buffer_addr, + image->arch.ima_buffer_size); + if (ret) + return -EINVAL; + + pr_debug("IMA buffer at 0x%llx, size = 0x%zx\n", + image->arch.ima_buffer_addr, image->arch.ima_buffer_size); + + return 0; +} +#else /* CONFIG_IMA_KEXEC */ +static inline int setup_ima_buffer(const struct kimage *image, void *fdt, + int chosen_node) +{ + return 0; +} +#endif /* CONFIG_IMA_KEXEC */ + /* * of_kexec_setup_new_fdt - modify /chosen and memory reservation for the next kernel * @@ -219,6 +395,11 @@ int of_kexec_setup_new_fdt(const struct kimage *image, void *fdt, } ret = fdt_setprop(fdt, chosen_node, "linux,booted-from-kexec", NULL, 0); + if (ret) + goto out; + + remove_ima_buffer(fdt, chosen_node); + ret = setup_ima_buffer(image, fdt, fdt_path_offset(fdt, "/chosen")); out: if (ret) @@ -226,3 +407,57 @@ int of_kexec_setup_new_fdt(const struct kimage *image, void *fdt, return 0; } + +/** + * ima_get_kexec_buffer - get IMA buffer from the previous kernel + * @addr: On successful return, set to point to the buffer contents. + * @size: On successful return, set to the buffer size. + * + * Return: 0 on success, negative errno on error. + */ +int ima_get_kexec_buffer(void **addr, size_t *size) +{ + int ret, len; + unsigned long tmp_addr; + size_t tmp_size; + const void *prop; + + prop = of_get_property(of_chosen, "linux,ima-kexec-buffer", &len); + if (!prop) + return -ENOENT; + + ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size); + if (ret) + return ret; + + *addr = __va(tmp_addr); + *size = tmp_size; + + return 0; +} + +/** + * ima_free_kexec_buffer - free memory used by the IMA buffer + */ +int ima_free_kexec_buffer(void) +{ + int ret; + unsigned long addr; + size_t size; + struct property *prop; + + prop = of_find_property(of_chosen, "linux,ima-kexec-buffer", NULL); + if (!prop) + return -ENOENT; + + ret = do_get_kexec_buffer(prop->value, prop->length, &addr, &size); + if (ret) + return ret; + + ret = of_remove_property(of_chosen, prop); + if (ret) + return ret; + + return memblock_free(addr, size); + +} diff --git a/include/linux/of.h b/include/linux/of.h index 3375f5295875..48eb2d05c0ec 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -562,6 +562,8 @@ struct kimage; int of_kexec_setup_new_fdt(const struct kimage *image, void *fdt, unsigned long initrd_load_addr, unsigned long initrd_len, const char *cmdline); +int ima_get_kexec_buffer(void **addr, size_t *size); +int ima_free_kexec_buffer(void); #else /* CONFIG_OF */ diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 6ebefec616e4..7c3947ad3773 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -24,10 +24,6 @@ #include "../integrity.h" -#ifdef CONFIG_HAVE_IMA_KEXEC -#include <asm/ima.h> -#endif - enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN, IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII }; enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 }; diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index 38bcd7543e27..8a6712981dee 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -10,6 +10,7 @@ #include <linux/seq_file.h> #include <linux/vmalloc.h> #include <linux/kexec.h> +#include <linux/of.h> #include <linux/ima.h> #include "ima.h"