diff mbox series

[v4] x86/kexec: Carry forward IMA measurement log on kexec

Message ID Yn01Cfb3Divf49g7@noodles-fedora.dhcp.thefacebook.com (mailing list archive)
State New
Headers show
Series [v4] x86/kexec: Carry forward IMA measurement log on kexec | expand

Commit Message

Jonathan McDowell May 12, 2022, 4:25 p.m. UTC
On kexec file load Integrity Measurement Architecture (IMA) subsystem
may verify the IMA signature of the kernel and initramfs, and measure
it. The command line parameters passed to the kernel in the kexec call
may also be measured by IMA. A remote attestation service can verify
a TPM quote based on the TPM event log, the IMA measurement list, and
the TPM PCR data. This can be achieved only if the IMA measurement log
is carried over from the current kernel to the next kernel across
the kexec call.

powerpc and ARM64 both achieve this using device tree with a
"linux,ima-kexec-buffer" node. x86 platforms generally don't make use of
device tree, so use the setup_data mechanism to pass the IMA buffer to
the new kernel.

Signed-off-by: Jonathan McDowell <noodles@fb.com>
---
v4:
 - Guard ima.h function prototypes with CONFIG_HAVE_IMA_KEXEC
v3:
 - Rebase on tip/master
 - Pull ima_(free|get)_kexec_buffer into x86 code
 - Push ifdefs into functions where possible
 - Reverse fir tree variable declarations
 - Fix section annotation on ima_free_kexec_buffer (__meminit)
 - Only allocate ima_setup_data space when IMA_KEXEC is enabled
v2:
 - Fix operation with EFI systems
---
 arch/x86/Kconfig                      |  1 +
 arch/x86/include/uapi/asm/bootparam.h |  9 ++++
 arch/x86/kernel/e820.c                |  6 +--
 arch/x86/kernel/kexec-bzimage64.c     | 38 ++++++++++++++++
 arch/x86/kernel/setup.c               | 62 +++++++++++++++++++++++++++
 drivers/of/kexec.c                    |  1 +
 include/linux/ima.h                   |  5 +++
 include/linux/of.h                    |  2 -
 security/integrity/ima/ima_kexec.c    |  2 +-
 9 files changed, 120 insertions(+), 6 deletions(-)

Comments

Lakshmi Ramasubramanian May 13, 2022, 5:19 p.m. UTC | #1
Hi Jonathan,

On 5/12/2022 9:25 AM, Jonathan McDowell wrote:
> On kexec file load Integrity Measurement Architecture (IMA) subsystem
> may verify the IMA signature of the kernel and initramfs, and measure
> it. The command line parameters passed to the kernel in the kexec call
> may also be measured by IMA. A remote attestation service can verify
> a TPM quote based on the TPM event log, the IMA measurement list, and
> the TPM PCR data. This can be achieved only if the IMA measurement log
> is carried over from the current kernel to the next kernel across
> the kexec call.
> 
> powerpc and ARM64 both achieve this using device tree with a
> "linux,ima-kexec-buffer" node. x86 platforms generally don't make use of
> device tree, so use the setup_data mechanism to pass the IMA buffer to
> the new kernel.
> 
> Signed-off-by: Jonathan McDowell <noodles@fb.com>
> ---
> v4:
>   - Guard ima.h function prototypes with CONFIG_HAVE_IMA_KEXEC
> v3:
>   - Rebase on tip/master
>   - Pull ima_(free|get)_kexec_buffer into x86 code
>   - Push ifdefs into functions where possible
>   - Reverse fir tree variable declarations
>   - Fix section annotation on ima_free_kexec_buffer (__meminit)
>   - Only allocate ima_setup_data space when IMA_KEXEC is enabled
> v2:
>   - Fix operation with EFI systems
> ---
>   arch/x86/Kconfig                      |  1 +
>   arch/x86/include/uapi/asm/bootparam.h |  9 ++++
>   arch/x86/kernel/e820.c                |  6 +--
>   arch/x86/kernel/kexec-bzimage64.c     | 38 ++++++++++++++++
>   arch/x86/kernel/setup.c               | 62 +++++++++++++++++++++++++++
>   drivers/of/kexec.c                    |  1 +
>   include/linux/ima.h                   |  5 +++
>   include/linux/of.h                    |  2 -
>   security/integrity/ima/ima_kexec.c    |  2 +-
>   9 files changed, 120 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index f1320d9a3da3..594636f02da4 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2027,6 +2027,7 @@ config KEXEC_FILE
>   	bool "kexec file based system call"
>   	select KEXEC_CORE
>   	select BUILD_BIN2C
> +	select HAVE_IMA_KEXEC if IMA
>   	depends on X86_64
>   	depends on CRYPTO=y
>   	depends on CRYPTO_SHA256=y
> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
> index bea5cdcdf532..ca0796ac4403 100644
> --- a/arch/x86/include/uapi/asm/bootparam.h
> +++ b/arch/x86/include/uapi/asm/bootparam.h
> @@ -11,6 +11,7 @@
>   #define SETUP_APPLE_PROPERTIES		5
>   #define SETUP_JAILHOUSE			6
>   #define SETUP_CC_BLOB			7
> +#define SETUP_IMA			8
>   
>   #define SETUP_INDIRECT			(1<<31)
>   
> @@ -172,6 +173,14 @@ struct jailhouse_setup_data {
>   	} __attribute__((packed)) v2;
>   } __attribute__((packed));
>   
> +/*
> + * IMA buffer setup data information from the previous kernel during kexec
> + */
> +struct ima_setup_data {
> +	__u64 addr;
> +	__u64 size;
> +} __attribute__((packed));
> +
>   /* The so-called "zeropage" */
>   struct boot_params {
>   	struct screen_info screen_info;			/* 0x000 */
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index f267205f2d5a..9dac24680ff8 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -1017,10 +1017,10 @@ void __init e820__reserve_setup_data(void)
>   		e820__range_update(pa_data, sizeof(*data)+data->len, E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
>   
>   		/*
> -		 * SETUP_EFI is supplied by kexec and does not need to be
> -		 * reserved.
> +		 * SETUP_EFI and SETUP_IMA are supplied by kexec and do not need
> +		 * to be reserved.
>   		 */
> -		if (data->type != SETUP_EFI)
> +		if (data->type != SETUP_EFI && data->type != SETUP_IMA)
>   			e820__range_update_kexec(pa_data,
>   						 sizeof(*data) + data->len,
>   						 E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> index 170d0fd68b1f..54bd4ce5f908 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -186,6 +186,33 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
>   }
>   #endif /* CONFIG_EFI */
>   
> +static void
> +setup_ima_state(const struct kimage *image, struct boot_params *params,
> +		unsigned long params_load_addr,
> +		unsigned int ima_setup_data_offset)
> +{
> +#ifdef CONFIG_IMA_KEXEC
> +	struct setup_data *sd = (void *)params + ima_setup_data_offset;
> +	unsigned long setup_data_phys;
> +	struct ima_setup_data *ima;
> +
> +	if (!image->ima_buffer_size)
> +		return;
> +
> +	sd->type = SETUP_IMA;
> +	sd->len = sizeof(*ima);
> +
> +	ima = (void *)sd + sizeof(struct setup_data);
> +	ima->addr = image->ima_buffer_addr;
> +	ima->size = image->ima_buffer_size;
> +
> +	/* Add setup data */
> +	setup_data_phys = params_load_addr + ima_setup_data_offset;
> +	sd->next = params->hdr.setup_data;
> +	params->hdr.setup_data = setup_data_phys;
> +#endif /* CONFIG_IMA_KEXEC */
> +}
> +
>   static int
>   setup_boot_parameters(struct kimage *image, struct boot_params *params,
>   		      unsigned long params_load_addr,
> @@ -247,6 +274,13 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
>   	setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
>   			efi_setup_data_offset);
>   #endif
> +
> +	/* Setup IMA log buffer state */
> +	setup_ima_state(image, params, params_load_addr,
> +			efi_setup_data_offset +
> +			sizeof(struct setup_data) +
> +			sizeof(struct efi_setup_data));
Here you could check image->ima_buffer_size and call setup_ima_state() 
only if it is non-zero.

> +
>   	/* Setup EDD info */
>   	memcpy(params->eddbuf, boot_params.eddbuf,
>   				EDDMAXNR * sizeof(struct edd_info));
> @@ -403,6 +437,10 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
>   				sizeof(struct setup_data) +
>   				sizeof(struct efi_setup_data);
>   
> +	if (IS_ENABLED(CONFIG_IMA_KEXEC))
> +		kbuf.bufsz += sizeof(struct setup_data) +
> +			      sizeof(struct ima_setup_data);
> +
>   	params = kzalloc(kbuf.bufsz, GFP_KERNEL);
>   	if (!params)
>   		return ERR_PTR(-ENOMEM);
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 249981bf3d8a..ab5e7a351845 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -11,6 +11,7 @@
>   #include <linux/dma-map-ops.h>
>   #include <linux/dmi.h>
>   #include <linux/efi.h>
> +#include <linux/ima.h>
>   #include <linux/init_ohci1394_dma.h>
>   #include <linux/initrd.h>
>   #include <linux/iscsi_ibft.h>
> @@ -145,6 +146,11 @@ __visible unsigned long mmu_cr4_features __ro_after_init;
>   __visible unsigned long mmu_cr4_features __ro_after_init = X86_CR4_PAE;
>   #endif
>   
> +#ifdef CONFIG_IMA
> +static phys_addr_t ima_kexec_buffer_phys;
> +static size_t ima_kexec_buffer_size;
> +#endif
> +
>   /* Boot loader ID and version as integers, for the benefit of proc_dointvec */
>   int bootloader_type, bootloader_version;
>   
> @@ -335,6 +341,59 @@ static void __init reserve_initrd(void)
>   }
>   #endif /* CONFIG_BLK_DEV_INITRD */
>   
> +static void __init add_early_ima_buffer(u64 phys_addr)
> +{
> +#ifdef CONFIG_IMA
> +	struct ima_setup_data *data;
> +
> +	data = early_memremap(phys_addr + sizeof(struct setup_data),
> +			      sizeof(*data));
> +	if (!data) {
> +		pr_warn("setup: failed to memremap ima_setup_data entry\n");
> +		return;
> +	}
Here if memory allocation fails, would kexec system call fail or would 
it only not add IMA buffer, but continue with the system call?

> +	if (data->size != 0) {
> +		memblock_reserve(data->addr, data->size);
> +		ima_kexec_buffer_phys = data->addr;
> +		ima_kexec_buffer_size = data->size;
> +	}
> +	early_memunmap(data, sizeof(*data));
> +#else
> +	pr_warn("Passed IMA kexec data, but CONFIG_IMA not set. Ignoring.\n");
Is this warning message useful? Can we just inline (NOP) this function 
if CONFIG_IMA is not set?

> +#endif
> +}
> +
> +#if defined(CONFIG_IMA) && !defined(CONFIG_OF_FLATTREE)
> +int __meminit ima_free_kexec_buffer(void)
> +{
ima_free_kexec_buffer() should be invoked if the previous kernel had 
passed the IMA buffer (i.e., CONFIG_HAVE_IMA_KEXEC is set). 
CONFIG_HAVE_IMA_KEXEC would be set only if CONFIG_IMA is set. Is the 
above check still required?

thanks,
  -lakshmi

> +	int rc;
> +
> +	if (ima_kexec_buffer_size == 0)
> +		return -ENOENT;
> +
> +	rc = memblock_phys_free(ima_kexec_buffer_phys,
> +				ima_kexec_buffer_size);
> +	if (rc)
> +		return rc;
> +
> +	ima_kexec_buffer_phys = 0;
> +	ima_kexec_buffer_size = 0;
> +
> +	return 0;
> +}
> +
> +int __init ima_get_kexec_buffer(void **addr, size_t *size)
> +{
> +	if (ima_kexec_buffer_size == 0)
> +		return -ENOENT;
> +
> +	*addr = __va(ima_kexec_buffer_phys);
> +	*size = ima_kexec_buffer_size;
> +
> +	return 0;
> +}
> +#endif
> +
>   static void __init parse_setup_data(void)
>   {
>   	struct setup_data *data;
> @@ -360,6 +419,9 @@ static void __init parse_setup_data(void)
>   		case SETUP_EFI:
>   			parse_efi_setup(pa_data, data_len);
>   			break;
> +		case SETUP_IMA:
> +			add_early_ima_buffer(pa_data);
> +			break;
>   		default:
>   			break;
>   		}
> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
> index b9bd1cff1793..74fdd490f7c0 100644
> --- a/drivers/of/kexec.c
> +++ b/drivers/of/kexec.c
> @@ -9,6 +9,7 @@
>    *  Copyright (C) 2016  IBM Corporation
>    */
>   
> +#include <linux/ima.h>
>   #include <linux/kernel.h>
>   #include <linux/kexec.h>
>   #include <linux/memblock.h>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 426b1744215e..ff4bd993e432 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -140,6 +140,11 @@ static inline int ima_measure_critical_data(const char *event_label,
>   
>   #endif /* CONFIG_IMA */
>   
> +#ifdef CONFIG_HAVE_IMA_KEXEC
> +int ima_free_kexec_buffer(void);
> +int ima_get_kexec_buffer(void **addr, size_t *size);
> +#endif
> +
>   #ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
>   extern bool arch_ima_get_secureboot(void);
>   extern const char * const *arch_get_ima_policy(void);
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 04971e85fbc9..c2f58d2e3a0e 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -441,8 +441,6 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
>   				   unsigned long initrd_load_addr,
>   				   unsigned long initrd_len,
>   				   const char *cmdline, size_t extra_fdt_size);
> -int ima_get_kexec_buffer(void **addr, size_t *size);
> -int ima_free_kexec_buffer(void);
>   #else /* CONFIG_OF */
>   
>   static inline void of_core_init(void)
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 13753136f03f..419dc405c831 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -137,7 +137,7 @@ void ima_add_kexec_buffer(struct kimage *image)
>   /*
>    * Restore the measurement list from the previous kernel.
>    */
> -void ima_load_kexec_buffer(void)
> +void __init ima_load_kexec_buffer(void)
>   {
>   	void *kexec_buffer = NULL;
>   	size_t kexec_buffer_size = 0;
Jonathan McDowell May 16, 2022, 3:15 p.m. UTC | #2
On Fri, May 13, 2022 at 10:19:17AM -0700, Lakshmi Ramasubramanian wrote:
> Hi Jonathan,
> 
> On 5/12/2022 9:25 AM, Jonathan McDowell wrote:
> > On kexec file load Integrity Measurement Architecture (IMA) subsystem
> > may verify the IMA signature of the kernel and initramfs, and measure
> > it. The command line parameters passed to the kernel in the kexec call
> > may also be measured by IMA. A remote attestation service can verify
> > a TPM quote based on the TPM event log, the IMA measurement list, and
> > the TPM PCR data. This can be achieved only if the IMA measurement log
> > is carried over from the current kernel to the next kernel across
> > the kexec call.
> > 
> > powerpc and ARM64 both achieve this using device tree with a
> > "linux,ima-kexec-buffer" node. x86 platforms generally don't make use of
> > device tree, so use the setup_data mechanism to pass the IMA buffer to
> > the new kernel.
> > 
> > Signed-off-by: Jonathan McDowell <noodles@fb.com>
> > ---
> > v4:
> >   - Guard ima.h function prototypes with CONFIG_HAVE_IMA_KEXEC
> > v3:
> >   - Rebase on tip/master
> >   - Pull ima_(free|get)_kexec_buffer into x86 code
> >   - Push ifdefs into functions where possible
> >   - Reverse fir tree variable declarations
> >   - Fix section annotation on ima_free_kexec_buffer (__meminit)
> >   - Only allocate ima_setup_data space when IMA_KEXEC is enabled
> > v2:
> >   - Fix operation with EFI systems
> > ---
> >   arch/x86/Kconfig                      |  1 +
> >   arch/x86/include/uapi/asm/bootparam.h |  9 ++++
> >   arch/x86/kernel/e820.c                |  6 +--
> >   arch/x86/kernel/kexec-bzimage64.c     | 38 ++++++++++++++++
> >   arch/x86/kernel/setup.c               | 62 +++++++++++++++++++++++++++
> >   drivers/of/kexec.c                    |  1 +
> >   include/linux/ima.h                   |  5 +++
> >   include/linux/of.h                    |  2 -
> >   security/integrity/ima/ima_kexec.c    |  2 +-
> >   9 files changed, 120 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index f1320d9a3da3..594636f02da4 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -2027,6 +2027,7 @@ config KEXEC_FILE
> >   	bool "kexec file based system call"
> >   	select KEXEC_CORE
> >   	select BUILD_BIN2C
> > +	select HAVE_IMA_KEXEC if IMA
> >   	depends on X86_64
> >   	depends on CRYPTO=y
> >   	depends on CRYPTO_SHA256=y
> > diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
> > index bea5cdcdf532..ca0796ac4403 100644
> > --- a/arch/x86/include/uapi/asm/bootparam.h
> > +++ b/arch/x86/include/uapi/asm/bootparam.h
> > @@ -11,6 +11,7 @@
> >   #define SETUP_APPLE_PROPERTIES		5
> >   #define SETUP_JAILHOUSE			6
> >   #define SETUP_CC_BLOB			7
> > +#define SETUP_IMA			8
> >   #define SETUP_INDIRECT			(1<<31)
> > @@ -172,6 +173,14 @@ struct jailhouse_setup_data {
> >   	} __attribute__((packed)) v2;
> >   } __attribute__((packed));
> > +/*
> > + * IMA buffer setup data information from the previous kernel during kexec
> > + */
> > +struct ima_setup_data {
> > +	__u64 addr;
> > +	__u64 size;
> > +} __attribute__((packed));
> > +
> >   /* The so-called "zeropage" */
> >   struct boot_params {
> >   	struct screen_info screen_info;			/* 0x000 */
> > diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> > index f267205f2d5a..9dac24680ff8 100644
> > --- a/arch/x86/kernel/e820.c
> > +++ b/arch/x86/kernel/e820.c
> > @@ -1017,10 +1017,10 @@ void __init e820__reserve_setup_data(void)
> >   		e820__range_update(pa_data, sizeof(*data)+data->len, E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> >   		/*
> > -		 * SETUP_EFI is supplied by kexec and does not need to be
> > -		 * reserved.
> > +		 * SETUP_EFI and SETUP_IMA are supplied by kexec and do not need
> > +		 * to be reserved.
> >   		 */
> > -		if (data->type != SETUP_EFI)
> > +		if (data->type != SETUP_EFI && data->type != SETUP_IMA)
> >   			e820__range_update_kexec(pa_data,
> >   						 sizeof(*data) + data->len,
> >   						 E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> > diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> > index 170d0fd68b1f..54bd4ce5f908 100644
> > --- a/arch/x86/kernel/kexec-bzimage64.c
> > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > @@ -186,6 +186,33 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
> >   }
> >   #endif /* CONFIG_EFI */
> > +static void
> > +setup_ima_state(const struct kimage *image, struct boot_params *params,
> > +		unsigned long params_load_addr,
> > +		unsigned int ima_setup_data_offset)
> > +{
> > +#ifdef CONFIG_IMA_KEXEC
> > +	struct setup_data *sd = (void *)params + ima_setup_data_offset;
> > +	unsigned long setup_data_phys;
> > +	struct ima_setup_data *ima;
> > +
> > +	if (!image->ima_buffer_size)
> > +		return;
> > +
> > +	sd->type = SETUP_IMA;
> > +	sd->len = sizeof(*ima);
> > +
> > +	ima = (void *)sd + sizeof(struct setup_data);
> > +	ima->addr = image->ima_buffer_addr;
> > +	ima->size = image->ima_buffer_size;
> > +
> > +	/* Add setup data */
> > +	setup_data_phys = params_load_addr + ima_setup_data_offset;
> > +	sd->next = params->hdr.setup_data;
> > +	params->hdr.setup_data = setup_data_phys;
> > +#endif /* CONFIG_IMA_KEXEC */
> > +}
> > +
> >   static int
> >   setup_boot_parameters(struct kimage *image, struct boot_params *params,
> >   		      unsigned long params_load_addr,
> > @@ -247,6 +274,13 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
> >   	setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
> >   			efi_setup_data_offset);
> >   #endif
> > +
> > +	/* Setup IMA log buffer state */
> > +	setup_ima_state(image, params, params_load_addr,
> > +			efi_setup_data_offset +
> > +			sizeof(struct setup_data) +
> > +			sizeof(struct efi_setup_data));
> Here you could check image->ima_buffer_size and call setup_ima_state() only
> if it is non-zero.

setup_ima_state() has this check already.

> > +
> >   	/* Setup EDD info */
> >   	memcpy(params->eddbuf, boot_params.eddbuf,
> >   				EDDMAXNR * sizeof(struct edd_info));
> > @@ -403,6 +437,10 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
> >   				sizeof(struct setup_data) +
> >   				sizeof(struct efi_setup_data);
> > +	if (IS_ENABLED(CONFIG_IMA_KEXEC))
> > +		kbuf.bufsz += sizeof(struct setup_data) +
> > +			      sizeof(struct ima_setup_data);
> > +
> >   	params = kzalloc(kbuf.bufsz, GFP_KERNEL);
> >   	if (!params)
> >   		return ERR_PTR(-ENOMEM);
> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > index 249981bf3d8a..ab5e7a351845 100644
> > --- a/arch/x86/kernel/setup.c
> > +++ b/arch/x86/kernel/setup.c
> > @@ -11,6 +11,7 @@
> >   #include <linux/dma-map-ops.h>
> >   #include <linux/dmi.h>
> >   #include <linux/efi.h>
> > +#include <linux/ima.h>
> >   #include <linux/init_ohci1394_dma.h>
> >   #include <linux/initrd.h>
> >   #include <linux/iscsi_ibft.h>
> > @@ -145,6 +146,11 @@ __visible unsigned long mmu_cr4_features __ro_after_init;
> >   __visible unsigned long mmu_cr4_features __ro_after_init = X86_CR4_PAE;
> >   #endif
> > +#ifdef CONFIG_IMA
> > +static phys_addr_t ima_kexec_buffer_phys;
> > +static size_t ima_kexec_buffer_size;
> > +#endif
> > +
> >   /* Boot loader ID and version as integers, for the benefit of proc_dointvec */
> >   int bootloader_type, bootloader_version;
> > @@ -335,6 +341,59 @@ static void __init reserve_initrd(void)
> >   }
> >   #endif /* CONFIG_BLK_DEV_INITRD */
> > +static void __init add_early_ima_buffer(u64 phys_addr)
> > +{
> > +#ifdef CONFIG_IMA
> > +	struct ima_setup_data *data;
> > +
> > +	data = early_memremap(phys_addr + sizeof(struct setup_data),
> > +			      sizeof(*data));
> > +	if (!data) {
> > +		pr_warn("setup: failed to memremap ima_setup_data entry\n");
> > +		return;
> > +	}
> Here if memory allocation fails, would kexec system call fail or would it
> only not add IMA buffer, but continue with the system call?

This is run in the context of the *new* kernel. Boot will continue, but
the IMA buffer will not be successfully passed across. Effectively that
puts us in the same situation as now; things like TPM PCRs will have
been updated, but we won't have the log showing us how we got to the
current state.

> > +	if (data->size != 0) {
> > +		memblock_reserve(data->addr, data->size);
> > +		ima_kexec_buffer_phys = data->addr;
> > +		ima_kexec_buffer_size = data->size;
> > +	}
> > +	early_memunmap(data, sizeof(*data));
> > +#else
> > +	pr_warn("Passed IMA kexec data, but CONFIG_IMA not set. Ignoring.\n");
> Is this warning message useful? Can we just inline (NOP) this function if
> CONFIG_IMA is not set?

It seems useful to me to know if the previous kernel is trying to pass
us IMA information but we're not configured for IMA, and it's not a lot
of overhead in terms of code in a path that's only actually executed if
we *are* passed the IMA kexec info.

> > +#endif
> > +}
> > +
> > +#if defined(CONFIG_IMA) && !defined(CONFIG_OF_FLATTREE)
> > +int __meminit ima_free_kexec_buffer(void)
> > +{
> ima_free_kexec_buffer() should be invoked if the previous kernel had passed
> the IMA buffer (i.e., CONFIG_HAVE_IMA_KEXEC is set). CONFIG_HAVE_IMA_KEXEC
> would be set only if CONFIG_IMA is set. Is the above check still required?

If we don't have IMA configured there's no point compiling this code in,
as there will be no callers of it. The OF_FLATTREE piece is to handle
the fact that the x86 platforms that use device tree (see previous
discussion in this thread about the fact there only seem to be 2 of
them, and they're both 32 bit) will end up needing to wire up the device
tree kexec passing if they want to use this functionality (and in fact
device tree passing across x86 kexec generally).

> thanks,
>  -lakshmi
> 
> > +	int rc;
> > +
> > +	if (ima_kexec_buffer_size == 0)
> > +		return -ENOENT;
> > +
> > +	rc = memblock_phys_free(ima_kexec_buffer_phys,
> > +				ima_kexec_buffer_size);
> > +	if (rc)
> > +		return rc;
> > +
> > +	ima_kexec_buffer_phys = 0;
> > +	ima_kexec_buffer_size = 0;
> > +
> > +	return 0;
> > +}
> > +
> > +int __init ima_get_kexec_buffer(void **addr, size_t *size)
> > +{
> > +	if (ima_kexec_buffer_size == 0)
> > +		return -ENOENT;
> > +
> > +	*addr = __va(ima_kexec_buffer_phys);
> > +	*size = ima_kexec_buffer_size;
> > +
> > +	return 0;
> > +}
> > +#endif
> > +
> >   static void __init parse_setup_data(void)
> >   {
> >   	struct setup_data *data;
> > @@ -360,6 +419,9 @@ static void __init parse_setup_data(void)
> >   		case SETUP_EFI:
> >   			parse_efi_setup(pa_data, data_len);
> >   			break;
> > +		case SETUP_IMA:
> > +			add_early_ima_buffer(pa_data);
> > +			break;
> >   		default:
> >   			break;
> >   		}
> > diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
> > index b9bd1cff1793..74fdd490f7c0 100644
> > --- a/drivers/of/kexec.c
> > +++ b/drivers/of/kexec.c
> > @@ -9,6 +9,7 @@
> >    *  Copyright (C) 2016  IBM Corporation
> >    */
> > +#include <linux/ima.h>
> >   #include <linux/kernel.h>
> >   #include <linux/kexec.h>
> >   #include <linux/memblock.h>
> > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > index 426b1744215e..ff4bd993e432 100644
> > --- a/include/linux/ima.h
> > +++ b/include/linux/ima.h
> > @@ -140,6 +140,11 @@ static inline int ima_measure_critical_data(const char *event_label,
> >   #endif /* CONFIG_IMA */
> > +#ifdef CONFIG_HAVE_IMA_KEXEC
> > +int ima_free_kexec_buffer(void);
> > +int ima_get_kexec_buffer(void **addr, size_t *size);
> > +#endif
> > +
> >   #ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
> >   extern bool arch_ima_get_secureboot(void);
> >   extern const char * const *arch_get_ima_policy(void);
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index 04971e85fbc9..c2f58d2e3a0e 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -441,8 +441,6 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
> >   				   unsigned long initrd_load_addr,
> >   				   unsigned long initrd_len,
> >   				   const char *cmdline, size_t extra_fdt_size);
> > -int ima_get_kexec_buffer(void **addr, size_t *size);
> > -int ima_free_kexec_buffer(void);
> >   #else /* CONFIG_OF */
> >   static inline void of_core_init(void)
> > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> > index 13753136f03f..419dc405c831 100644
> > --- a/security/integrity/ima/ima_kexec.c
> > +++ b/security/integrity/ima/ima_kexec.c
> > @@ -137,7 +137,7 @@ void ima_add_kexec_buffer(struct kimage *image)
> >   /*
> >    * Restore the measurement list from the previous kernel.
> >    */
> > -void ima_load_kexec_buffer(void)
> > +void __init ima_load_kexec_buffer(void)
> >   {
> >   	void *kexec_buffer = NULL;
> >   	size_t kexec_buffer_size = 0;
Lakshmi Ramasubramanian May 17, 2022, 5:19 p.m. UTC | #3
>>> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
>>> index 170d0fd68b1f..54bd4ce5f908 100644
>>> --- a/arch/x86/kernel/kexec-bzimage64.c
>>> +++ b/arch/x86/kernel/kexec-bzimage64.c
>>> @@ -186,6 +186,33 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
>>>    }
>>>    #endif /* CONFIG_EFI */
>>> +static void
>>> +setup_ima_state(const struct kimage *image, struct boot_params *params,
>>> +		unsigned long params_load_addr,
>>> +		unsigned int ima_setup_data_offset)
>>> +{
>>> +#ifdef CONFIG_IMA_KEXEC
>>> +	struct setup_data *sd = (void *)params + ima_setup_data_offset;
>>> +	unsigned long setup_data_phys;
>>> +	struct ima_setup_data *ima;
>>> +
>>> +	if (!image->ima_buffer_size)
>>> +		return;
>>> +
>>> +	sd->type = SETUP_IMA;
>>> +	sd->len = sizeof(*ima);
>>> +
>>> +	ima = (void *)sd + sizeof(struct setup_data);
>>> +	ima->addr = image->ima_buffer_addr;
>>> +	ima->size = image->ima_buffer_size;
>>> +
>>> +	/* Add setup data */
>>> +	setup_data_phys = params_load_addr + ima_setup_data_offset;
>>> +	sd->next = params->hdr.setup_data;
>>> +	params->hdr.setup_data = setup_data_phys;
>>> +#endif /* CONFIG_IMA_KEXEC */
>>> +}
>>> +
>>>    static int
>>>    setup_boot_parameters(struct kimage *image, struct boot_params *params,
>>>    		      unsigned long params_load_addr,
>>> @@ -247,6 +274,13 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
>>>    	setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
>>>    			efi_setup_data_offset);
>>>    #endif
>>> +
>>> +	/* Setup IMA log buffer state */
>>> +	setup_ima_state(image, params, params_load_addr,
>>> +			efi_setup_data_offset +
>>> +			sizeof(struct setup_data) +
>>> +			sizeof(struct efi_setup_data));
>> Here you could check image->ima_buffer_size and call setup_ima_state() only
>> if it is non-zero.
> 
> setup_ima_state() has this check already.

Yes - I noticed that.
I was just suggesting a minor optimization - avoid making this function 
call if IMA buffer is not present.

> 
>>> +
>>>    	/* Setup EDD info */
>>>    	memcpy(params->eddbuf, boot_params.eddbuf,
>>>    				EDDMAXNR * sizeof(struct edd_info));
>>> @@ -403,6 +437,10 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
>>>    				sizeof(struct setup_data) +
>>>    				sizeof(struct efi_setup_data);
>>> +	if (IS_ENABLED(CONFIG_IMA_KEXEC))
>>> +		kbuf.bufsz += sizeof(struct setup_data) +
>>> +			      sizeof(struct ima_setup_data);
>>> +
>>>    	params = kzalloc(kbuf.bufsz, GFP_KERNEL);
>>>    	if (!params)
>>>    		return ERR_PTR(-ENOMEM);
>>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>>> index 249981bf3d8a..ab5e7a351845 100644
>>> --- a/arch/x86/kernel/setup.c
>>> +++ b/arch/x86/kernel/setup.c
>>> @@ -11,6 +11,7 @@
>>>    #include <linux/dma-map-ops.h>
>>>    #include <linux/dmi.h>
>>>    #include <linux/efi.h>
>>> +#include <linux/ima.h>
>>>    #include <linux/init_ohci1394_dma.h>
>>>    #include <linux/initrd.h>
>>>    #include <linux/iscsi_ibft.h>
>>> @@ -145,6 +146,11 @@ __visible unsigned long mmu_cr4_features __ro_after_init;
>>>    __visible unsigned long mmu_cr4_features __ro_after_init = X86_CR4_PAE;
>>>    #endif
>>> +#ifdef CONFIG_IMA
>>> +static phys_addr_t ima_kexec_buffer_phys;
>>> +static size_t ima_kexec_buffer_size;
>>> +#endif
>>> +
>>>    /* Boot loader ID and version as integers, for the benefit of proc_dointvec */
>>>    int bootloader_type, bootloader_version;
>>> @@ -335,6 +341,59 @@ static void __init reserve_initrd(void)
>>>    }
>>>    #endif /* CONFIG_BLK_DEV_INITRD */
>>> +static void __init add_early_ima_buffer(u64 phys_addr)
>>> +{
>>> +#ifdef CONFIG_IMA
>>> +	struct ima_setup_data *data;
>>> +
>>> +	data = early_memremap(phys_addr + sizeof(struct setup_data),
>>> +			      sizeof(*data));
>>> +	if (!data) {
>>> +		pr_warn("setup: failed to memremap ima_setup_data entry\n");
>>> +		return;
>>> +	}
>> Here if memory allocation fails, would kexec system call fail or would it
>> only not add IMA buffer, but continue with the system call?
> 
> This is run in the context of the *new* kernel. Boot will continue, but
> the IMA buffer will not be successfully passed across. Effectively that
> puts us in the same situation as now; things like TPM PCRs will have
> been updated, but we won't have the log showing us how we got to the
> current state.
I think it is better to treat this error as a critical failure.

> 
>>> +	if (data->size != 0) {
>>> +		memblock_reserve(data->addr, data->size);
>>> +		ima_kexec_buffer_phys = data->addr;
>>> +		ima_kexec_buffer_size = data->size;
>>> +	}
>>> +	early_memunmap(data, sizeof(*data));
>>> +#else
>>> +	pr_warn("Passed IMA kexec data, but CONFIG_IMA not set. Ignoring.\n");
>> Is this warning message useful? Can we just inline (NOP) this function if
>> CONFIG_IMA is not set?
> 
> It seems useful to me to know if the previous kernel is trying to pass
> us IMA information but we're not configured for IMA, and it's not a lot
> of overhead in terms of code in a path that's only actually executed if
> we *are* passed the IMA kexec info.

okay.

> 
>>> +#endif
>>> +}
>>> +
>>> +#if defined(CONFIG_IMA) && !defined(CONFIG_OF_FLATTREE)
>>> +int __meminit ima_free_kexec_buffer(void)
>>> +{
>> ima_free_kexec_buffer() should be invoked if the previous kernel had passed
>> the IMA buffer (i.e., CONFIG_HAVE_IMA_KEXEC is set). CONFIG_HAVE_IMA_KEXEC
>> would be set only if CONFIG_IMA is set. Is the above check still required?
> 
> If we don't have IMA configured there's no point compiling this code in,
> as there will be no callers of it. The OF_FLATTREE piece is to handle
> the fact that the x86 platforms that use device tree (see previous
> discussion in this thread about the fact there only seem to be 2 of
> them, and they're both 32 bit) will end up needing to wire up the device
> tree kexec passing if they want to use this functionality (and in fact
> device tree passing across x86 kexec generally).

okay.

  -lakshmi

>>
>>> +	int rc;
>>> +
>>> +	if (ima_kexec_buffer_size == 0)
>>> +		return -ENOENT;
>>> +
>>> +	rc = memblock_phys_free(ima_kexec_buffer_phys,
>>> +				ima_kexec_buffer_size);
>>> +	if (rc)
>>> +		return rc;
>>> +
>>> +	ima_kexec_buffer_phys = 0;
>>> +	ima_kexec_buffer_size = 0;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +int __init ima_get_kexec_buffer(void **addr, size_t *size)
>>> +{
>>> +	if (ima_kexec_buffer_size == 0)
>>> +		return -ENOENT;
>>> +
>>> +	*addr = __va(ima_kexec_buffer_phys);
>>> +	*size = ima_kexec_buffer_size;
>>> +
>>> +	return 0;
>>> +}
>>> +#endif
>>> +
>>>    static void __init parse_setup_data(void)
>>>    {
>>>    	struct setup_data *data;
>>> @@ -360,6 +419,9 @@ static void __init parse_setup_data(void)
>>>    		case SETUP_EFI:
>>>    			parse_efi_setup(pa_data, data_len);
>>>    			break;
>>> +		case SETUP_IMA:
>>> +			add_early_ima_buffer(pa_data);
>>> +			break;
>>>    		default:
>>>    			break;
>>>    		}
>>> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
>>> index b9bd1cff1793..74fdd490f7c0 100644
>>> --- a/drivers/of/kexec.c
>>> +++ b/drivers/of/kexec.c
>>> @@ -9,6 +9,7 @@
>>>     *  Copyright (C) 2016  IBM Corporation
>>>     */
>>> +#include <linux/ima.h>
>>>    #include <linux/kernel.h>
>>>    #include <linux/kexec.h>
>>>    #include <linux/memblock.h>
>>> diff --git a/include/linux/ima.h b/include/linux/ima.h
>>> index 426b1744215e..ff4bd993e432 100644
>>> --- a/include/linux/ima.h
>>> +++ b/include/linux/ima.h
>>> @@ -140,6 +140,11 @@ static inline int ima_measure_critical_data(const char *event_label,
>>>    #endif /* CONFIG_IMA */
>>> +#ifdef CONFIG_HAVE_IMA_KEXEC
>>> +int ima_free_kexec_buffer(void);
>>> +int ima_get_kexec_buffer(void **addr, size_t *size);
>>> +#endif
>>> +
>>>    #ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
>>>    extern bool arch_ima_get_secureboot(void);
>>>    extern const char * const *arch_get_ima_policy(void);
>>> diff --git a/include/linux/of.h b/include/linux/of.h
>>> index 04971e85fbc9..c2f58d2e3a0e 100644
>>> --- a/include/linux/of.h
>>> +++ b/include/linux/of.h
>>> @@ -441,8 +441,6 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
>>>    				   unsigned long initrd_load_addr,
>>>    				   unsigned long initrd_len,
>>>    				   const char *cmdline, size_t extra_fdt_size);
>>> -int ima_get_kexec_buffer(void **addr, size_t *size);
>>> -int ima_free_kexec_buffer(void);
>>>    #else /* CONFIG_OF */
>>>    static inline void of_core_init(void)
>>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
>>> index 13753136f03f..419dc405c831 100644
>>> --- a/security/integrity/ima/ima_kexec.c
>>> +++ b/security/integrity/ima/ima_kexec.c
>>> @@ -137,7 +137,7 @@ void ima_add_kexec_buffer(struct kimage *image)
>>>    /*
>>>     * Restore the measurement list from the previous kernel.
>>>     */
>>> -void ima_load_kexec_buffer(void)
>>> +void __init ima_load_kexec_buffer(void)
>>>    {
>>>    	void *kexec_buffer = NULL;
>>>    	size_t kexec_buffer_size = 0;
Jonathan McDowell May 18, 2022, 10:42 a.m. UTC | #4
On Tue, May 17, 2022 at 10:19:45AM -0700, Lakshmi Ramasubramanian wrote:
> 
> > > > diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> > > > index 170d0fd68b1f..54bd4ce5f908 100644
> > > > --- a/arch/x86/kernel/kexec-bzimage64.c
> > > > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > > > @@ -186,6 +186,33 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
> > > >    }
> > > >    #endif /* CONFIG_EFI */
> > > > +static void
> > > > +setup_ima_state(const struct kimage *image, struct boot_params *params,
> > > > +		unsigned long params_load_addr,
> > > > +		unsigned int ima_setup_data_offset)
> > > > +{
> > > > +#ifdef CONFIG_IMA_KEXEC
> > > > +	struct setup_data *sd = (void *)params + ima_setup_data_offset;
> > > > +	unsigned long setup_data_phys;
> > > > +	struct ima_setup_data *ima;
> > > > +
> > > > +	if (!image->ima_buffer_size)
> > > > +		return;
> > > > +
> > > > +	sd->type = SETUP_IMA;
> > > > +	sd->len = sizeof(*ima);
> > > > +
> > > > +	ima = (void *)sd + sizeof(struct setup_data);
> > > > +	ima->addr = image->ima_buffer_addr;
> > > > +	ima->size = image->ima_buffer_size;
> > > > +
> > > > +	/* Add setup data */
> > > > +	setup_data_phys = params_load_addr + ima_setup_data_offset;
> > > > +	sd->next = params->hdr.setup_data;
> > > > +	params->hdr.setup_data = setup_data_phys;
> > > > +#endif /* CONFIG_IMA_KEXEC */
> > > > +}
> > > > +
> > > >    static int
> > > >    setup_boot_parameters(struct kimage *image, struct boot_params *params,
> > > >    		      unsigned long params_load_addr,
> > > > @@ -247,6 +274,13 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
> > > >    	setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
> > > >    			efi_setup_data_offset);
> > > >    #endif
> > > > +
> > > > +	/* Setup IMA log buffer state */
> > > > +	setup_ima_state(image, params, params_load_addr,
> > > > +			efi_setup_data_offset +
> > > > +			sizeof(struct setup_data) +
> > > > +			sizeof(struct efi_setup_data));
> > > Here you could check image->ima_buffer_size and call setup_ima_state() only
> > > if it is non-zero.
> > 
> > setup_ima_state() has this check already.
> 
> Yes - I noticed that.
> I was just suggesting a minor optimization - avoid making this function call
> if IMA buffer is not present.
> 
> > 
> > > > +
> > > >    	/* Setup EDD info */
> > > >    	memcpy(params->eddbuf, boot_params.eddbuf,
> > > >    				EDDMAXNR * sizeof(struct edd_info));
> > > > @@ -403,6 +437,10 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
> > > >    				sizeof(struct setup_data) +
> > > >    				sizeof(struct efi_setup_data);
> > > > +	if (IS_ENABLED(CONFIG_IMA_KEXEC))
> > > > +		kbuf.bufsz += sizeof(struct setup_data) +
> > > > +			      sizeof(struct ima_setup_data);
> > > > +
> > > >    	params = kzalloc(kbuf.bufsz, GFP_KERNEL);
> > > >    	if (!params)
> > > >    		return ERR_PTR(-ENOMEM);
> > > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > > > index 249981bf3d8a..ab5e7a351845 100644
> > > > --- a/arch/x86/kernel/setup.c
> > > > +++ b/arch/x86/kernel/setup.c
> > > > @@ -11,6 +11,7 @@
> > > >    #include <linux/dma-map-ops.h>
> > > >    #include <linux/dmi.h>
> > > >    #include <linux/efi.h>
> > > > +#include <linux/ima.h>
> > > >    #include <linux/init_ohci1394_dma.h>
> > > >    #include <linux/initrd.h>
> > > >    #include <linux/iscsi_ibft.h>
> > > > @@ -145,6 +146,11 @@ __visible unsigned long mmu_cr4_features __ro_after_init;
> > > >    __visible unsigned long mmu_cr4_features __ro_after_init = X86_CR4_PAE;
> > > >    #endif
> > > > +#ifdef CONFIG_IMA
> > > > +static phys_addr_t ima_kexec_buffer_phys;
> > > > +static size_t ima_kexec_buffer_size;
> > > > +#endif
> > > > +
> > > >    /* Boot loader ID and version as integers, for the benefit of proc_dointvec */
> > > >    int bootloader_type, bootloader_version;
> > > > @@ -335,6 +341,59 @@ static void __init reserve_initrd(void)
> > > >    }
> > > >    #endif /* CONFIG_BLK_DEV_INITRD */
> > > > +static void __init add_early_ima_buffer(u64 phys_addr)
> > > > +{
> > > > +#ifdef CONFIG_IMA
> > > > +	struct ima_setup_data *data;
> > > > +
> > > > +	data = early_memremap(phys_addr + sizeof(struct setup_data),
> > > > +			      sizeof(*data));
> > > > +	if (!data) {
> > > > +		pr_warn("setup: failed to memremap ima_setup_data entry\n");
> > > > +		return;
> > > > +	}
> > > Here if memory allocation fails, would kexec system call fail or would it
> > > only not add IMA buffer, but continue with the system call?
> > 
> > This is run in the context of the *new* kernel. Boot will continue, but
> > the IMA buffer will not be successfully passed across. Effectively that
> > puts us in the same situation as now; things like TPM PCRs will have
> > been updated, but we won't have the log showing us how we got to the
> > current state.
> I think it is better to treat this error as a critical failure.

That's going to crash the entire system, because it's after we've
started execution of the new kernel. Given that the failure mode will
result in the lack of the logs, but not incorrect TPM PCRs, that seems a
bit extreme.

J.
Mimi Zohar May 18, 2022, 2:43 p.m. UTC | #5
On Thu, 2022-05-12 at 16:25 +0000, Jonathan McDowell wrote:
> On kexec file load Integrity Measurement Architecture (IMA) subsystem
> may verify the IMA signature of the kernel and initramfs, and measure
> it. The command line parameters passed to the kernel in the kexec call
> may also be measured by IMA. A remote attestation service can verify
> a TPM quote based on the TPM event log, the IMA measurement list, and
> the TPM PCR data. This can be achieved only if the IMA measurement log
> is carried over from the current kernel to the next kernel across
> the kexec call.
> 
> powerpc and ARM64 both achieve this using device tree with a
> "linux,ima-kexec-buffer" node. x86 platforms generally don't make use of
> device tree, so use the setup_data mechanism to pass the IMA buffer to
> the new kernel.
> 
> Signed-off-by: Jonathan McDowell <noodles@fb.com>

Not from using "setup_data" perspective,

	Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>  # IMA function
definitions

thanks,

Mimi
Jonathan McDowell May 30, 2022, 8:40 a.m. UTC | #6
Borislav,

I don't think there are any outstanding review comments for me to deal
with on this, so is it safe to assume it'll get picked up at some point
once the merge window calms down?

On Wed, May 18, 2022 at 10:43:32AM -0400, Mimi Zohar wrote:
> On Thu, 2022-05-12 at 16:25 +0000, Jonathan McDowell wrote:
> > On kexec file load Integrity Measurement Architecture (IMA) subsystem
> > may verify the IMA signature of the kernel and initramfs, and measure
> > it. The command line parameters passed to the kernel in the kexec call
> > may also be measured by IMA. A remote attestation service can verify
> > a TPM quote based on the TPM event log, the IMA measurement list, and
> > the TPM PCR data. This can be achieved only if the IMA measurement log
> > is carried over from the current kernel to the next kernel across
> > the kexec call.
> > 
> > powerpc and ARM64 both achieve this using device tree with a
> > "linux,ima-kexec-buffer" node. x86 platforms generally don't make use of
> > device tree, so use the setup_data mechanism to pass the IMA buffer to
> > the new kernel.
> > 
> > Signed-off-by: Jonathan McDowell <noodles@fb.com>
> 
> Not from using "setup_data" perspective,
> 
> 	Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>  # IMA function
> definitions
> 
> thanks,
> 
> Mimi

Thanks,
J.
Dave Hansen June 3, 2022, 3:55 p.m. UTC | #7
On 5/30/22 01:40, Jonathan McDowell wrote:
> Borislav,
> 
> I don't think there are any outstanding review comments for me to deal
> with on this, so is it safe to assume it'll get picked up at some point
> once the merge window calms down?

Nothing here looks too crazy, but it's still been _very_ lightly
reviewed.  It doesn't seem like anyone from the kexec world has seen it,
for instance.

Mimi's review was a great start, but it would be really nice to make
sure that the kexec bits look good.
Baoquan He June 6, 2022, 3:54 a.m. UTC | #8
On 06/03/22 at 08:55am, Dave Hansen wrote:
> On 5/30/22 01:40, Jonathan McDowell wrote:
> > Borislav,
> > 
> > I don't think there are any outstanding review comments for me to deal
> > with on this, so is it safe to assume it'll get picked up at some point
> > once the merge window calms down?
> 
> Nothing here looks too crazy, but it's still been _very_ lightly
> reviewed.  It doesn't seem like anyone from the kexec world has seen it,
> for instance.
> 
> Mimi's review was a great start, but it would be really nice to make
> sure that the kexec bits look good.

The change looks good from kexec/kdump side. Not sure if Eric has
any concern.
Baoquan He June 6, 2022, 4:06 a.m. UTC | #9
On 05/12/22 at 04:25pm, Jonathan McDowell wrote:
> On kexec file load Integrity Measurement Architecture (IMA) subsystem
> may verify the IMA signature of the kernel and initramfs, and measure
> it. The command line parameters passed to the kernel in the kexec call
> may also be measured by IMA. A remote attestation service can verify
> a TPM quote based on the TPM event log, the IMA measurement list, and
> the TPM PCR data. This can be achieved only if the IMA measurement log
> is carried over from the current kernel to the next kernel across
> the kexec call.
> 
> powerpc and ARM64 both achieve this using device tree with a
> "linux,ima-kexec-buffer" node. x86 platforms generally don't make use of
> device tree, so use the setup_data mechanism to pass the IMA buffer to
> the new kernel.

The entire looks good to me, other than a minor concern, please see the
inline comment.

Reviewed-by: Baoquan He <bhe@redhat.com>

Hi Coiby,

You can check this patch, see if you can take the same way to solve the
LUKS-encrypted disk issue by passing the key via setup_data.

> 
> Signed-off-by: Jonathan McDowell <noodles@fb.com>
> ---
......snip...

> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> index 170d0fd68b1f..54bd4ce5f908 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -186,6 +186,33 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
>  }
>  #endif /* CONFIG_EFI */
>  
> +static void
> +setup_ima_state(const struct kimage *image, struct boot_params *params,
> +		unsigned long params_load_addr,
> +		unsigned int ima_setup_data_offset)
> +{
> +#ifdef CONFIG_IMA_KEXEC
> +	struct setup_data *sd = (void *)params + ima_setup_data_offset;
> +	unsigned long setup_data_phys;
> +	struct ima_setup_data *ima;
> +
> +	if (!image->ima_buffer_size)
> +		return;
> +
> +	sd->type = SETUP_IMA;
> +	sd->len = sizeof(*ima);
> +
> +	ima = (void *)sd + sizeof(struct setup_data);
> +	ima->addr = image->ima_buffer_addr;
> +	ima->size = image->ima_buffer_size;
> +
> +	/* Add setup data */
> +	setup_data_phys = params_load_addr + ima_setup_data_offset;
> +	sd->next = params->hdr.setup_data;
> +	params->hdr.setup_data = setup_data_phys;
> +#endif /* CONFIG_IMA_KEXEC */
> +}
> +
>  static int
>  setup_boot_parameters(struct kimage *image, struct boot_params *params,
>  		      unsigned long params_load_addr,
> @@ -247,6 +274,13 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
>  	setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
>  			efi_setup_data_offset);
>  #endif
> +
> +	/* Setup IMA log buffer state */
> +	setup_ima_state(image, params, params_load_addr,
> +			efi_setup_data_offset +
> +			sizeof(struct setup_data) +
> +			sizeof(struct efi_setup_data));

Is it a little better to update efi_setup_data_offset beforehand, or
define a local variable?

	efi_setup_data_offset += sizeof(struct setup_data) + sizeof(struct efi_setup_data));
	setup_ima_state(image, params, params_load_addr,
			efi_setup_data_offset));

No strong opinion. If nobody has concern about it.

> +
>  	/* Setup EDD info */
>  	memcpy(params->eddbuf, boot_params.eddbuf,
>  				EDDMAXNR * sizeof(struct edd_info));
Jonathan McDowell June 10, 2022, 9:52 a.m. UTC | #10
On Mon, Jun 06, 2022 at 12:06:51PM +0800, Baoquan He wrote:
> On 05/12/22 at 04:25pm, Jonathan McDowell wrote:
> > On kexec file load Integrity Measurement Architecture (IMA) subsystem
> > may verify the IMA signature of the kernel and initramfs, and measure
> > it. The command line parameters passed to the kernel in the kexec call
> > may also be measured by IMA. A remote attestation service can verify
> > a TPM quote based on the TPM event log, the IMA measurement list, and
> > the TPM PCR data. This can be achieved only if the IMA measurement log
> > is carried over from the current kernel to the next kernel across
> > the kexec call.
> > 
> > powerpc and ARM64 both achieve this using device tree with a
> > "linux,ima-kexec-buffer" node. x86 platforms generally don't make use of
> > device tree, so use the setup_data mechanism to pass the IMA buffer to
> > the new kernel.
> 
> The entire looks good to me, other than a minor concern, please see the
> inline comment.
> 
> Reviewed-by: Baoquan He <bhe@redhat.com>

Thanks. Still waiting to see if Eric has any comments before deciding
whether to spin a v5 or not.

> Hi Coiby,
> 
> You can check this patch, see if you can take the same way to solve the
> LUKS-encrypted disk issue by passing the key via setup_data.
> 
> > 
> > Signed-off-by: Jonathan McDowell <noodles@fb.com>
> > ---
> ......snip...
> 
> > diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> > index 170d0fd68b1f..54bd4ce5f908 100644
> > --- a/arch/x86/kernel/kexec-bzimage64.c
> > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > @@ -186,6 +186,33 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
> >  }
> >  #endif /* CONFIG_EFI */
> >  
> > +static void
> > +setup_ima_state(const struct kimage *image, struct boot_params *params,
> > +		unsigned long params_load_addr,
> > +		unsigned int ima_setup_data_offset)
> > +{
> > +#ifdef CONFIG_IMA_KEXEC
> > +	struct setup_data *sd = (void *)params + ima_setup_data_offset;
> > +	unsigned long setup_data_phys;
> > +	struct ima_setup_data *ima;
> > +
> > +	if (!image->ima_buffer_size)
> > +		return;
> > +
> > +	sd->type = SETUP_IMA;
> > +	sd->len = sizeof(*ima);
> > +
> > +	ima = (void *)sd + sizeof(struct setup_data);
> > +	ima->addr = image->ima_buffer_addr;
> > +	ima->size = image->ima_buffer_size;
> > +
> > +	/* Add setup data */
> > +	setup_data_phys = params_load_addr + ima_setup_data_offset;
> > +	sd->next = params->hdr.setup_data;
> > +	params->hdr.setup_data = setup_data_phys;
> > +#endif /* CONFIG_IMA_KEXEC */
> > +}
> > +
> >  static int
> >  setup_boot_parameters(struct kimage *image, struct boot_params *params,
> >  		      unsigned long params_load_addr,
> > @@ -247,6 +274,13 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
> >  	setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
> >  			efi_setup_data_offset);
> >  #endif
> > +
> > +	/* Setup IMA log buffer state */
> > +	setup_ima_state(image, params, params_load_addr,
> > +			efi_setup_data_offset +
> > +			sizeof(struct setup_data) +
> > +			sizeof(struct efi_setup_data));
> 
> Is it a little better to update efi_setup_data_offset beforehand, or
> define a local variable?
> 
> 	efi_setup_data_offset += sizeof(struct setup_data) + sizeof(struct efi_setup_data));
> 	setup_ima_state(image, params, params_load_addr,
> 			efi_setup_data_offset));
> 
> No strong opinion. If nobody has concern about it.
> 
> > +
> >  	/* Setup EDD info */
> >  	memcpy(params->eddbuf, boot_params.eddbuf,
> >  				EDDMAXNR * sizeof(struct edd_info));
>
diff mbox series

Patch

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f1320d9a3da3..594636f02da4 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2027,6 +2027,7 @@  config KEXEC_FILE
 	bool "kexec file based system call"
 	select KEXEC_CORE
 	select BUILD_BIN2C
+	select HAVE_IMA_KEXEC if IMA
 	depends on X86_64
 	depends on CRYPTO=y
 	depends on CRYPTO_SHA256=y
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index bea5cdcdf532..ca0796ac4403 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -11,6 +11,7 @@ 
 #define SETUP_APPLE_PROPERTIES		5
 #define SETUP_JAILHOUSE			6
 #define SETUP_CC_BLOB			7
+#define SETUP_IMA			8
 
 #define SETUP_INDIRECT			(1<<31)
 
@@ -172,6 +173,14 @@  struct jailhouse_setup_data {
 	} __attribute__((packed)) v2;
 } __attribute__((packed));
 
+/*
+ * IMA buffer setup data information from the previous kernel during kexec
+ */
+struct ima_setup_data {
+	__u64 addr;
+	__u64 size;
+} __attribute__((packed));
+
 /* The so-called "zeropage" */
 struct boot_params {
 	struct screen_info screen_info;			/* 0x000 */
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index f267205f2d5a..9dac24680ff8 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -1017,10 +1017,10 @@  void __init e820__reserve_setup_data(void)
 		e820__range_update(pa_data, sizeof(*data)+data->len, E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
 
 		/*
-		 * SETUP_EFI is supplied by kexec and does not need to be
-		 * reserved.
+		 * SETUP_EFI and SETUP_IMA are supplied by kexec and do not need
+		 * to be reserved.
 		 */
-		if (data->type != SETUP_EFI)
+		if (data->type != SETUP_EFI && data->type != SETUP_IMA)
 			e820__range_update_kexec(pa_data,
 						 sizeof(*data) + data->len,
 						 E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index 170d0fd68b1f..54bd4ce5f908 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -186,6 +186,33 @@  setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
 }
 #endif /* CONFIG_EFI */
 
+static void
+setup_ima_state(const struct kimage *image, struct boot_params *params,
+		unsigned long params_load_addr,
+		unsigned int ima_setup_data_offset)
+{
+#ifdef CONFIG_IMA_KEXEC
+	struct setup_data *sd = (void *)params + ima_setup_data_offset;
+	unsigned long setup_data_phys;
+	struct ima_setup_data *ima;
+
+	if (!image->ima_buffer_size)
+		return;
+
+	sd->type = SETUP_IMA;
+	sd->len = sizeof(*ima);
+
+	ima = (void *)sd + sizeof(struct setup_data);
+	ima->addr = image->ima_buffer_addr;
+	ima->size = image->ima_buffer_size;
+
+	/* Add setup data */
+	setup_data_phys = params_load_addr + ima_setup_data_offset;
+	sd->next = params->hdr.setup_data;
+	params->hdr.setup_data = setup_data_phys;
+#endif /* CONFIG_IMA_KEXEC */
+}
+
 static int
 setup_boot_parameters(struct kimage *image, struct boot_params *params,
 		      unsigned long params_load_addr,
@@ -247,6 +274,13 @@  setup_boot_parameters(struct kimage *image, struct boot_params *params,
 	setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
 			efi_setup_data_offset);
 #endif
+
+	/* Setup IMA log buffer state */
+	setup_ima_state(image, params, params_load_addr,
+			efi_setup_data_offset +
+			sizeof(struct setup_data) +
+			sizeof(struct efi_setup_data));
+
 	/* Setup EDD info */
 	memcpy(params->eddbuf, boot_params.eddbuf,
 				EDDMAXNR * sizeof(struct edd_info));
@@ -403,6 +437,10 @@  static void *bzImage64_load(struct kimage *image, char *kernel,
 				sizeof(struct setup_data) +
 				sizeof(struct efi_setup_data);
 
+	if (IS_ENABLED(CONFIG_IMA_KEXEC))
+		kbuf.bufsz += sizeof(struct setup_data) +
+			      sizeof(struct ima_setup_data);
+
 	params = kzalloc(kbuf.bufsz, GFP_KERNEL);
 	if (!params)
 		return ERR_PTR(-ENOMEM);
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 249981bf3d8a..ab5e7a351845 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -11,6 +11,7 @@ 
 #include <linux/dma-map-ops.h>
 #include <linux/dmi.h>
 #include <linux/efi.h>
+#include <linux/ima.h>
 #include <linux/init_ohci1394_dma.h>
 #include <linux/initrd.h>
 #include <linux/iscsi_ibft.h>
@@ -145,6 +146,11 @@  __visible unsigned long mmu_cr4_features __ro_after_init;
 __visible unsigned long mmu_cr4_features __ro_after_init = X86_CR4_PAE;
 #endif
 
+#ifdef CONFIG_IMA
+static phys_addr_t ima_kexec_buffer_phys;
+static size_t ima_kexec_buffer_size;
+#endif
+
 /* Boot loader ID and version as integers, for the benefit of proc_dointvec */
 int bootloader_type, bootloader_version;
 
@@ -335,6 +341,59 @@  static void __init reserve_initrd(void)
 }
 #endif /* CONFIG_BLK_DEV_INITRD */
 
+static void __init add_early_ima_buffer(u64 phys_addr)
+{
+#ifdef CONFIG_IMA
+	struct ima_setup_data *data;
+
+	data = early_memremap(phys_addr + sizeof(struct setup_data),
+			      sizeof(*data));
+	if (!data) {
+		pr_warn("setup: failed to memremap ima_setup_data entry\n");
+		return;
+	}
+	if (data->size != 0) {
+		memblock_reserve(data->addr, data->size);
+		ima_kexec_buffer_phys = data->addr;
+		ima_kexec_buffer_size = data->size;
+	}
+	early_memunmap(data, sizeof(*data));
+#else
+	pr_warn("Passed IMA kexec data, but CONFIG_IMA not set. Ignoring.\n");
+#endif
+}
+
+#if defined(CONFIG_IMA) && !defined(CONFIG_OF_FLATTREE)
+int __meminit ima_free_kexec_buffer(void)
+{
+	int rc;
+
+	if (ima_kexec_buffer_size == 0)
+		return -ENOENT;
+
+	rc = memblock_phys_free(ima_kexec_buffer_phys,
+				ima_kexec_buffer_size);
+	if (rc)
+		return rc;
+
+	ima_kexec_buffer_phys = 0;
+	ima_kexec_buffer_size = 0;
+
+	return 0;
+}
+
+int __init ima_get_kexec_buffer(void **addr, size_t *size)
+{
+	if (ima_kexec_buffer_size == 0)
+		return -ENOENT;
+
+	*addr = __va(ima_kexec_buffer_phys);
+	*size = ima_kexec_buffer_size;
+
+	return 0;
+}
+#endif
+
 static void __init parse_setup_data(void)
 {
 	struct setup_data *data;
@@ -360,6 +419,9 @@  static void __init parse_setup_data(void)
 		case SETUP_EFI:
 			parse_efi_setup(pa_data, data_len);
 			break;
+		case SETUP_IMA:
+			add_early_ima_buffer(pa_data);
+			break;
 		default:
 			break;
 		}
diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
index b9bd1cff1793..74fdd490f7c0 100644
--- a/drivers/of/kexec.c
+++ b/drivers/of/kexec.c
@@ -9,6 +9,7 @@ 
  *  Copyright (C) 2016  IBM Corporation
  */
 
+#include <linux/ima.h>
 #include <linux/kernel.h>
 #include <linux/kexec.h>
 #include <linux/memblock.h>
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 426b1744215e..ff4bd993e432 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -140,6 +140,11 @@  static inline int ima_measure_critical_data(const char *event_label,
 
 #endif /* CONFIG_IMA */
 
+#ifdef CONFIG_HAVE_IMA_KEXEC
+int ima_free_kexec_buffer(void);
+int ima_get_kexec_buffer(void **addr, size_t *size);
+#endif
+
 #ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
 extern bool arch_ima_get_secureboot(void);
 extern const char * const *arch_get_ima_policy(void);
diff --git a/include/linux/of.h b/include/linux/of.h
index 04971e85fbc9..c2f58d2e3a0e 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -441,8 +441,6 @@  void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
 				   unsigned long initrd_load_addr,
 				   unsigned long initrd_len,
 				   const char *cmdline, size_t extra_fdt_size);
-int ima_get_kexec_buffer(void **addr, size_t *size);
-int ima_free_kexec_buffer(void);
 #else /* CONFIG_OF */
 
 static inline void of_core_init(void)
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 13753136f03f..419dc405c831 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -137,7 +137,7 @@  void ima_add_kexec_buffer(struct kimage *image)
 /*
  * Restore the measurement list from the previous kernel.
  */
-void ima_load_kexec_buffer(void)
+void __init ima_load_kexec_buffer(void)
 {
 	void *kexec_buffer = NULL;
 	size_t kexec_buffer_size = 0;