[PATCH v3 1/2] ima: fix build error redeclaration of enumerator
Commit a893ea15d764 ("tpm: move tpm_chip definition to
include/linux/tpm.h") introduced a build error when both ima and efi is
enabled. What happens is that both headers (ima.h and efi.h) defines the
same 'NONE' constant, and it broke when they started getting included
from the same file.
In file included from ../security/integrity/ima/ima_fs.c:30:
../security/integrity/ima/ima.h:176:7: error: redeclaration of enumerator "NONE"
hook(NONE) \
^~~~
../security/integrity/ima/ima.h:188:34: note: in definition of macro
"__ima_hook_enumify"
#define __ima_hook_enumify(ENUM) ENUM,
^~~~
../security/integrity/ima/ima.h:191:2: note: in expansion of macro "__ima_hooks"
__ima_hooks(__ima_hook_enumify)
^~~
In file included from ../arch/arm64/include/asm/acpi.h:15,
from ../include/acpi/acpi_io.h:7,
from ../include/linux/acpi.h:47,
from ../include/linux/tpm.h:26,
from ../security/integrity/ima/ima.h:25,
from ../security/integrity/ima/ima_fs.c:30:
../include/linux/efi.h:1723:2: note: previous definition of "NONE" was here
NONE,
^~~~
make[4]: *** [../scripts/Makefile.build:277: security/integrity/ima/ima_fs.o]
Error 1
Rework to prefix the ima enum with 'IMA_*'.
Reviewed-by: Andy Shevchenko
Signed-off-by: Anders Roxell
---
No change since v2.
security/integrity/ima/ima.h | 24 +++
security/integrity/ima/ima_api.c | 3 +-
security/integrity/ima/ima_appraise.c | 40 ++--
security/integrity/ima/ima_main.c | 30 -
security/integrity/ima/ima_policy.c | 92 +--
5 files changed, 95 insertions(+), 94 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..89ceb61f279c 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -173,18 +173,18 @@ static inline unsigned long ima_hash_key(u8 *digest)
}
#define __ima_hooks(hook) \
- hook(NONE) \
- hook(FILE_CHECK)\
- hook(MMAP_CHECK)\
- hook(BPRM_CHECK)\
- hook(CREDS_CHECK) \
- hook(POST_SETATTR) \
- hook(MODULE_CHECK) \
- hook(FIRMWARE_CHECK)\
- hook(KEXEC_KERNEL_CHECK)\
- hook(KEXEC_INITRAMFS_CHECK) \
- hook(POLICY_CHECK) \
- hook(MAX_CHECK)
+ hook(IMA_NONE) \
+ hook(IMA_FILE_CHECK)\
+ hook(IMA_MMAP_CHECK)\
+ hook(IMA_BPRM_CHECK)\
+ hook(IMA_CREDS_CHECK) \
+ hook(IMA_POST_SETATTR) \
+ hook(IMA_MODULE_CHECK) \
+ hook(IMA_FIRMWARE_CHECK)\
+ hook(IMA_KEXEC_KERNEL_CHECK)\
+ hook(IMA_KEXEC_INITRAMFS_CHECK) \
+ hook(IMA_POLICY_CHECK) \
+ hook(IMA_MAX_CHECK)
#define __ima_hook_enumify(ENUM) ENUM,
enum ima_hooks {
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c7505fb122d4..81e705423894 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -168,7 +168,8 @@ void ima_add_violation(struct file *file, const unsigned
char *filename,
* The policy is defined in terms of keypairs:
* subj=, obj=, type=, func=, mask=, fsmagic=
* subj,obj, and type: are LSM specific.
- * func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
+ * func: IMA_FILE_CHECK | IMA_BPRM_CHECK | IMA_CREDS_CHECK \
+ * | IMA_MMAP_CHECK | IMA_MODULE_CHECK
* mask: contains the permission mask
* fsmagic: hex value
*
diff --git a/security/integrity/ima/ima_appraise.c
b/security/integrity/ima/ima_appraise.c
index a2baa85ea2f5..c527cf3f37d3 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -86,16 +86,16 @@ enum integrity_status ima_get_cache_status(struct
integrity_iint_cache *iint,
enum ima_hooks func)
{
switch (func) {
- case MMAP_CHECK:
+ case IMA_MMAP_CHECK:
return iint->ima_mmap_status;
- case BPRM_CHECK:
+ case IMA_BPRM_CHECK:
return iint->ima_bprm_status;
- case CREDS_CHECK:
+ case IMA_CREDS_CHECK:
return iint->ima_creds_status;
- case FILE_CHECK:
- case POST_SETATTR:
+ case IMA_FILE_CHECK:
+ case IMA_POST_SETATTR:
return iint->ima_file_status;
- case MODULE_CHECK ... MAX_CHECK - 1:
+ case IMA_MODULE_CHECK ... IMA_MAX_CHECK - 1:
default:
return iint->ima_read_status;
}
@@ -106,19 +106,19 @@ static void ima_set_cache_status(struct
integrity_iint_cache *iint,
enum integrity_status status)
{
[PATCH v3 2/2] efi: fix build error redeclaration of enumerator
Commit a893ea15d764 ("tpm: move tpm_chip definition to
include/linux/tpm.h") introduced a build error when both ima and efi is
enabled. What happens is that both headers (ima.h and efi.h) defines the
same 'NONE' constant, and it broke when they started getting included
from the same file.
In file included from ../security/integrity/ima/ima_fs.c:30:
../security/integrity/ima/ima.h:176:7: error: redeclaration of enumerator "NONE"
hook(NONE) \
^~~~
../security/integrity/ima/ima.h:188:34: note: in definition of macro
"__ima_hook_enumify"
#define __ima_hook_enumify(ENUM) ENUM,
^~~~
../security/integrity/ima/ima.h:191:2: note: in expansion of macro "__ima_hooks"
__ima_hooks(__ima_hook_enumify)
^~~
In file included from ../arch/arm64/include/asm/acpi.h:15,
from ../include/acpi/acpi_io.h:7,
from ../include/linux/acpi.h:47,
from ../include/linux/tpm.h:26,
from ../security/integrity/ima/ima.h:25,
from ../security/integrity/ima/ima_fs.c:30:
../include/linux/efi.h:1723:2: note: previous definition of "NONE" was here
NONE,
^~~~
make[4]: *** [../scripts/Makefile.build:277: security/integrity/ima/ima_fs.o]
Error 1
Rework to prefix the efi enum with 'EFI_*'.
Signed-off-by: Anders Roxell
---
Changes against v2:
- renamed a missed 'NONE' to 'EFI_NONE'.
arch/x86/platform/efi/quirks.c | 4 +--
drivers/firmware/efi/runtime-wrappers.c | 48 -
include/linux/efi.h | 26 +++---
3 files changed, 39 insertions(+), 39 deletions(-)
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 9ce85e605052..458a0e2bcc57 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -717,7 +717,7 @@ void efi_recover_from_page_fault(unsigned long phys_addr)
* "efi_mm" cannot be used to check if the page fault had occurred
* in the firmware context because efi=old_map doesn't use efi_pgd.
*/
- if (efi_rts_work.efi_rts_id == NONE)
+ if (efi_rts_work.efi_rts_id == EFI_NONE)
return;
/*
@@ -742,7 +742,7 @@ void efi_recover_from_page_fault(unsigned long phys_addr)
* because this case occurs *very* rarely and hence could be improved
* on a need by basis.
*/
- if (efi_rts_work.efi_rts_id == RESET_SYSTEM) {
+ if (efi_rts_work.efi_rts_id == EFI_RESET_SYSTEM) {
pr_info("efi_reset_system() buggy! Reboot through BIOS\n");
machine_real_restart(MRR_BIOS);
return;
diff --git a/drivers/firmware/efi/runtime-wrappers.c
b/drivers/firmware/efi/runtime-wrappers.c
index c70df5ae7c4a..28138534643e 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -85,7 +85,7 @@ struct efi_runtime_work efi_rts_work;
pr_err("Failed to queue work to efi_rts_wq.\n");\
\
exit: \
- efi_rts_work.efi_rts_id = NONE; \
+ efi_rts_work.efi_rts_id = EFI_NONE; \
efi_rts_work.status;\
})
@@ -181,50 +181,50 @@ static void efi_call_rts(struct work_struct *work)
arg5 = efi_rts_work.arg5;
switch (efi_rts_work.efi_rts_id) {
- case GET_TIME:
+ case EFI_GET_TIME:
status = efi_call_virt(get_time, (efi_time_t *)arg1,
(efi_time_cap_t *)arg2);
break;
- case SET_TIME:
+ case EFI_SET_TIME:
status = efi_call_virt(set_time, (efi_time_t *)arg1);
break;
- case GET_WAKEUP_TIME:
+ case EFI_GET_WAKEUP_TIME:
status = efi_call_virt(get_wakeup_time, (efi_bool_t *)arg1,
(efi_bool_t *)arg2, (efi_time_t *)arg3);
break;
- case SET_WAKEUP_TIME:
+ case EFI_SET_WAKEUP_TIME:
status = efi_call_virt(set_wakeup_time, *(efi_bool_t *)arg1,
(efi_time_t *)arg2);
break;
- case GET_VARIABLE:
+ case EFI_GET_VARIABLE:
status = efi_call_virt(get_variable, (efi_char16_t *)arg1,
(efi_guid_t *)arg2, (u32 *)arg3,
(unsigned long *)arg4, (void *)arg5);
break;
- case GET_NEXT_VARIABLE:
+ case EFI_GET_NEXT_VARIABLE:
status = efi_call_virt(get_next_variable, (unsigned long *)arg1,
(efi_char16_t *)arg2,
(efi_guid_t *)arg3);
break;
- case SET_VARIABLE:
[Patch v3 3/4] x86/platform/UV: use efi_enabled() instead of test_bit()
Use ad hoc efi_enabled() instead of fiddling with test_bit(). Cleanup, no functional changes. Cc: Russ Anderson Cc: Mike Travis Cc: Dimitri Sivanich Cc: Steve Wahl Cc: sta...@vger.kernel.org # v4.9+ Signed-off-by: Hedi Berriche --- arch/x86/platform/uv/bios_uv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c index 91e3d5285836..38a2e3431fc6 100644 --- a/arch/x86/platform/uv/bios_uv.c +++ b/arch/x86/platform/uv/bios_uv.c @@ -44,7 +44,7 @@ s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64 a5) * If EFI_OLD_MEMMAP is set, we need to fall back to using our old EFI * callback method, which uses efi_call() directly, with the kernel page tables: */ - if (unlikely(test_bit(EFI_OLD_MEMMAP, ))) + if (unlikely(efi_enabled(EFI_OLD_MEMMAP))) ret = efi_call((void *)__va(tab->function), (u64)which, a1, a2, a3, a4, a5); else ret = efi_call_virt_pointer(tab, function, (u64)which, a1, a2, a3, a4, a5); -- 2.20.1
[Patch v3 0/4] Protect against concurrent calls into UV BIOS
- Changes since v2 Addressed comments from Ard Biesheuvel: * expose efi_runtime_lock to UV platform only instead of globally * remove unnecessary #ifdef CONFIG_EFI from bios_uv.c - Changes since v1: Addressed comments from Bhupesh Sharma, Thomas Gleixner, and Ard Biesheuvel: * made __uv_bios_call() static * moved the efi_enabled() cleanup to its own patchlet * explained the reason for renaming the efi_runtime_lock semaphore * dropped the reviewed-bys as they should be given on the mailing list * Cc'ng sta...@vger.kernel.org given the nature of the problem addressed by the patches --- Calls into UV BIOS were not being serialised which is wrong as it violates EFI runtime rules, and bad as it does result in all sorts of potentially hard to track down hangs and panics when efi_scratch.prev_mm gets clobbered whenever efi_switch_mm() gets called concurrently from two different CPUs. Patch #1 removes an unnecessary #ifdef CONFIG_EFI guard from bios_uv.c. Patch #2 removes uv_bios_call_reentrant() because it's dead code. Patch #3 is a cleanup that drops test_bit() in favour of the ad hoc efi_enabled(). Patch #4 makes uv_bios_call() variants use the efi_runtime_lock semaphore to protect against concurrency. Cc: Russ Anderson Cc: Mike Travis Cc: Dimitri Sivanich Cc: Steve Wahl Cc: sta...@vger.kernel.org # v4.9+ Hedi Berriche (4): x86/platform/UV: remove unnecessary #ifdef CONFIG_EFI x86/platform/UV: kill uv_bios_call_reentrant() as it has no callers x86/platform/UV: use efi_enabled() instead of test_bit() x86/platform/UV: use efi_runtime_lock to serialise BIOS calls arch/x86/include/asm/uv/bios.h | 13 - arch/x86/platform/uv/bios_uv.c | 35 ++--- drivers/firmware/efi/runtime-wrappers.c | 7 + 3 files changed, 34 insertions(+), 21 deletions(-) -- 2.20.1
[Patch v3 2/4] x86/platform/UV: kill uv_bios_call_reentrant()
uv_bios_call_reentrant() has no callers nor is it exported, kill it.
Cleanup, no functional changes.
Cc: Russ Anderson
Cc: Mike Travis
Cc: Dimitri Sivanich
Cc: Steve Wahl
Cc: sta...@vger.kernel.org # v4.9+
Signed-off-by: Hedi Berriche
---
arch/x86/include/asm/uv/bios.h | 1 -
arch/x86/platform/uv/bios_uv.c | 12
2 files changed, 13 deletions(-)
diff --git a/arch/x86/include/asm/uv/bios.h b/arch/x86/include/asm/uv/bios.h
index 00d862cfbcbe..8c6ac271b5b3 100644
--- a/arch/x86/include/asm/uv/bios.h
+++ b/arch/x86/include/asm/uv/bios.h
@@ -140,7 +140,6 @@ enum uv_memprotect {
*/
extern s64 uv_bios_call(enum uv_bios_cmd, u64, u64, u64, u64, u64);
extern s64 uv_bios_call_irqsave(enum uv_bios_cmd, u64, u64, u64, u64, u64);
-extern s64 uv_bios_call_reentrant(enum uv_bios_cmd, u64, u64, u64, u64, u64);
extern s64 uv_bios_get_sn_info(int, int *, long *, long *, long *, long *);
extern s64 uv_bios_freq_base(u64, u64 *);
diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
index 4a61ed2a7bb8..91e3d5285836 100644
--- a/arch/x86/platform/uv/bios_uv.c
+++ b/arch/x86/platform/uv/bios_uv.c
@@ -66,18 +66,6 @@ s64 uv_bios_call_irqsave(enum uv_bios_cmd which, u64 a1, u64
a2, u64 a3,
return ret;
}
-s64 uv_bios_call_reentrant(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3,
- u64 a4, u64 a5)
-{
- s64 ret;
-
- preempt_disable();
- ret = uv_bios_call(which, a1, a2, a3, a4, a5);
- preempt_enable();
-
- return ret;
-}
-
long sn_partition_id;
EXPORT_SYMBOL_GPL(sn_partition_id);
--
2.20.1
[Patch v3 1/4] x86/platform/UV: remove unnecessary #ifdef CONFIG_EFI
CONFIG_EFI is implied by CONFIG_X86_UV and x86/platform/uv/bios_uv.c
requires the latter, get rid of the redundant #ifdef CONFIG_EFI directives.
Cleanup, no functional changes.
Cc: Russ Anderson
Cc: Mike Travis
Cc: Dimitri Sivanich
Cc: Steve Wahl
Cc: sta...@vger.kernel.org # v4.9+
Signed-off-by: Hedi Berriche
---
arch/x86/include/asm/uv/bios.h | 4
arch/x86/platform/uv/bios_uv.c | 2 --
2 files changed, 6 deletions(-)
diff --git a/arch/x86/include/asm/uv/bios.h b/arch/x86/include/asm/uv/bios.h
index e652a7cc6186..00d862cfbcbe 100644
--- a/arch/x86/include/asm/uv/bios.h
+++ b/arch/x86/include/asm/uv/bios.h
@@ -151,11 +151,7 @@ extern s64 uv_bios_change_memprotect(u64, u64, enum
uv_memprotect);
extern s64 uv_bios_reserved_page_pa(u64, u64 *, u64 *, u64 *);
extern int uv_bios_set_legacy_vga_target(bool decode, int domain, int bus);
-#ifdef CONFIG_EFI
extern void uv_bios_init(void);
-#else
-void uv_bios_init(void) { }
-#endif
extern unsigned long sn_rtc_cycles_per_second;
extern int uv_type;
diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
index 4a6a5a26c582..4a61ed2a7bb8 100644
--- a/arch/x86/platform/uv/bios_uv.c
+++ b/arch/x86/platform/uv/bios_uv.c
@@ -188,7 +188,6 @@ int uv_bios_set_legacy_vga_target(bool decode, int domain,
int bus)
}
EXPORT_SYMBOL_GPL(uv_bios_set_legacy_vga_target);
-#ifdef CONFIG_EFI
void uv_bios_init(void)
{
uv_systab = NULL;
@@ -218,4 +217,3 @@ void uv_bios_init(void)
}
pr_info("UV: UVsystab: Revision:%x\n", uv_systab->revision);
}
-#endif
--
2.20.1
[Patch v3 4/4] x86/platform/UV: use efi_runtime_lock to serialise BIOS calls
Calls into UV firmware must be protected against concurrency, expose the
efi_runtime_lock to the UV platform, and use it to serialise UV BIOS calls.
Cc: Russ Anderson
Cc: Mike Travis
Cc: Dimitri Sivanich
Cc: Steve Wahl
Cc: sta...@vger.kernel.org # v4.9+
Signed-off-by: Hedi Berriche
---
arch/x86/include/asm/uv/bios.h | 8 +++-
arch/x86/platform/uv/bios_uv.c | 23 +--
drivers/firmware/efi/runtime-wrappers.c | 7 +++
3 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/arch/x86/include/asm/uv/bios.h b/arch/x86/include/asm/uv/bios.h
index 8c6ac271b5b3..8cfccc3cbbf4 100644
--- a/arch/x86/include/asm/uv/bios.h
+++ b/arch/x86/include/asm/uv/bios.h
@@ -48,7 +48,8 @@ enum {
BIOS_STATUS_SUCCESS = 0,
BIOS_STATUS_UNIMPLEMENTED = -ENOSYS,
BIOS_STATUS_EINVAL = -EINVAL,
- BIOS_STATUS_UNAVAIL = -EBUSY
+ BIOS_STATUS_UNAVAIL = -EBUSY,
+ BIOS_STATUS_ABORT = -EINTR,
};
/* Address map parameters */
@@ -162,4 +163,9 @@ extern long system_serial_number;
extern struct kobject *sgi_uv_kobj;/* /sys/firmware/sgi_uv */
+/*
+ * EFI runtime lock; cf. firmware/efi/runtime-wrappers.c for details
+ */
+extern struct semaphore __efi_uv_runtime_lock;
+
#endif /* _ASM_X86_UV_BIOS_H */
diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
index 38a2e3431fc6..ef60d789c76e 100644
--- a/arch/x86/platform/uv/bios_uv.c
+++ b/arch/x86/platform/uv/bios_uv.c
@@ -29,7 +29,8 @@
struct uv_systab *uv_systab;
-s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64
a5)
+static s64 __uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3,
+ u64 a4, u64 a5)
{
struct uv_systab *tab = uv_systab;
s64 ret;
@@ -51,6 +52,19 @@ s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64
a3, u64 a4, u64 a5)
return ret;
}
+
+s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64
a5)
+{
+ s64 ret;
+
+ if (down_interruptible(&__efi_uv_runtime_lock))
+ return BIOS_STATUS_ABORT;
+
+ ret = __uv_bios_call(which, a1, a2, a3, a4, a5);
+ up(&__efi_uv_runtime_lock);
+
+ return ret;
+}
EXPORT_SYMBOL_GPL(uv_bios_call);
s64 uv_bios_call_irqsave(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3,
@@ -59,10 +73,15 @@ s64 uv_bios_call_irqsave(enum uv_bios_cmd which, u64 a1,
u64 a2, u64 a3,
unsigned long bios_flags;
s64 ret;
+ if (down_interruptible(&__efi_uv_runtime_lock))
+ return BIOS_STATUS_ABORT;
+
local_irq_save(bios_flags);
- ret = uv_bios_call(which, a1, a2, a3, a4, a5);
+ ret = __uv_bios_call(which, a1, a2, a3, a4, a5);
local_irq_restore(bios_flags);
+ up(&__efi_uv_runtime_lock);
+
return ret;
}
diff --git a/drivers/firmware/efi/runtime-wrappers.c
b/drivers/firmware/efi/runtime-wrappers.c
index 8903b9ccfc2b..e2abfdb5cee6 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -146,6 +146,13 @@ void efi_call_virt_check_flags(unsigned long flags, const
char *call)
*/
static DEFINE_SEMAPHORE(efi_runtime_lock);
+/*
+ * Expose the EFI runtime lock to the UV platform
+ */
+#ifdef CONFIG_X86_UV
+extern struct semaphore __efi_uv_runtime_lock __alias(efi_runtime_lock);
+#endif
+
/*
* Calls the appropriate efi_runtime_service() with the appropriate
* arguments.
--
2.20.1
Re: [PATCH 4/5] efi/libstub/tpm: Retrieve TPM event log in 2.0 format
On Wed, 13 Feb 2019 at 15:21, Bartosz Szczepanek wrote: > > On Wed, Feb 13, 2019 at 12:26 PM Jarkko Sakkinen > wrote: > > Collides with Matthew's changes. I want to land those change first > > because they are almost production ready. > > > > Maybe you should consider reviewing those changes to make sure that > > they make sense to you so that you can build these on top of after > > these have landed. > > Yeah, I think so. Actually, I wasn't aware of Matthew's efforts, as it > didn't appear on linux-efi mailing list. (On bad, I haven't checked > linux-integrity.) > > At this point, I think it makes more sense to limit this patchset to > 5/5 patch, which makes TPM event log initialized on ARM platforms. > Patches 1-4 introduce nothing more than Matthew already did, maybe > except putting calc_tpm2_event_size to a library instead of making it > inline. This function has already grown a bit so it may be a better > approach, but that's nothing to affect functionality. > > I'll pull Matthew changes to my tree to confirm operation on ARM > platforms, if that works fine the only thing to merge would be 5/5 + > optionally the library change, if we reach agreement on that. > Sounds good, and yes, it would be good to cc patches that affect the EFI subsystem to linux-efi as well.
Re: [PATCH 4/5] efi/libstub/tpm: Retrieve TPM event log in 2.0 format
On Wed, Feb 13, 2019 at 12:26 PM Jarkko Sakkinen wrote: > Collides with Matthew's changes. I want to land those change first > because they are almost production ready. > > Maybe you should consider reviewing those changes to make sure that > they make sense to you so that you can build these on top of after > these have landed. Yeah, I think so. Actually, I wasn't aware of Matthew's efforts, as it didn't appear on linux-efi mailing list. (On bad, I haven't checked linux-integrity.) At this point, I think it makes more sense to limit this patchset to 5/5 patch, which makes TPM event log initialized on ARM platforms. Patches 1-4 introduce nothing more than Matthew already did, maybe except putting calc_tpm2_event_size to a library instead of making it inline. This function has already grown a bit so it may be a better approach, but that's nothing to affect functionality. I'll pull Matthew changes to my tree to confirm operation on ARM platforms, if that works fine the only thing to merge would be 5/5 + optionally the library change, if we reach agreement on that. Bartosz
[PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table
In the irqchip and EFI code, we have what basically amounts to a quirk
to work around a peculiarity in the GICv3 architecture, which permits
the system memory address of LPI tables to be programmable only once
after a CPU reset. This means kexec kernels must use the same memory
as the first kernel, and thus ensure that this memory has not been
given out for other purposes by the time the ITS init code runs, which
is not very early for secondary CPUs.
On systems with many CPUs, these reservations could overflow the
memblock reservation table, and this was addressed in commit
eff896288872 ("efi/arm: Defer persistent reservations until after
paging_init()"). However, this turns out to have made things worse,
since the allocation of page tables and heap space for the resized
memblock reservation table itself may overwrite the regions we are
attempting to reserve, which may cause all kinds of corruption,
also considering that the ITS will still be poking bits into that
memory in response to incoming MSIs.
So instead, let's grow the static memblock reservation table on such
systems so it can accommodate these reservations at an earlier time.
This will permit us to revert the above commit in a subsequent patch.
Signed-off-by: Ard Biesheuvel
---
arch/arm64/include/asm/memory.h | 11 +++
include/linux/memblock.h| 3 ---
mm/memblock.c | 10 --
3 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index e1ec947e7c0c..7e2b13cdd970 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -332,6 +332,17 @@ static inline void *phys_to_virt(phys_addr_t x)
#define virt_addr_valid(kaddr) \
(_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))
+/*
+ * Given that the GIC architecture permits ITS implementations that can only be
+ * configured with a LPI table address once, GICv3 systems with many CPUs may
+ * end up reserving a lot of different regions after a kexec for their LPI
+ * tables, as we are forced to reuse the same memory after kexec (and thus
+ * reserve it persistently with EFI beforehand)
+ */
+#if defined(CONFIG_EFI) && defined(CONFIG_ARM_GIC_V3_ITS)
+#define INIT_MEMBLOCK_RESERVED_REGIONS (INIT_MEMBLOCK_REGIONS + 2 * NR_CPUS)
+#endif
+
#include
#endif
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 64c41cf45590..859b55b66db2 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -29,9 +29,6 @@ extern unsigned long max_pfn;
*/
extern unsigned long long max_possible_pfn;
-#define INIT_MEMBLOCK_REGIONS 128
-#define INIT_PHYSMEM_REGIONS 4
-
/**
* enum memblock_flags - definition of memory region attributes
* @MEMBLOCK_NONE: no special request
diff --git a/mm/memblock.c b/mm/memblock.c
index 022d4cbb3618..a526c3ab8390 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -26,6 +26,12 @@
#include "internal.h"
+#define INIT_MEMBLOCK_REGIONS 128
+#define INIT_PHYSMEM_REGIONS 4
+#ifndef INIT_MEMBLOCK_RESERVED_REGIONS
+#define INIT_MEMBLOCK_RESERVED_REGIONS INIT_MEMBLOCK_REGIONS
+#endif
+
/**
* DOC: memblock overview
*
@@ -92,7 +98,7 @@ unsigned long max_pfn;
unsigned long long max_possible_pfn;
static struct memblock_region
memblock_memory_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
-static struct memblock_region
memblock_reserved_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
+static struct memblock_region
memblock_reserved_init_regions[INIT_MEMBLOCK_RESERVED_REGIONS]
__initdata_memblock;
#ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
static struct memblock_region
memblock_physmem_init_regions[INIT_PHYSMEM_REGIONS] __initdata_memblock;
#endif
@@ -105,7 +111,7 @@ struct memblock memblock __initdata_memblock = {
.reserved.regions = memblock_reserved_init_regions,
.reserved.cnt = 1,/* empty dummy entry */
- .reserved.max = INIT_MEMBLOCK_REGIONS,
+ .reserved.max = INIT_MEMBLOCK_RESERVED_REGIONS,
.reserved.name = "reserved",
#ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
--
2.20.1
[PATCH 2/2] efi/arm: Revert "Defer persistent reservations until after paging_init()"
This reverts commit eff896288872d687d9662000ec9ae11b6d61766f, which
deferred the processing of persistent memory reservations to a point
where the memory may have already been allocated and overwritten,
defeating the purpose.
Signed-off-by: Ard Biesheuvel
---
arch/arm64/kernel/setup.c | 1 -
drivers/firmware/efi/efi.c | 4
drivers/firmware/efi/libstub/arm-stub.c | 3 ---
include/linux/efi.h | 7 ---
4 files changed, 15 deletions(-)
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 4b0e1231625c..d09ec76f08cf 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -313,7 +313,6 @@ void __init setup_arch(char **cmdline_p)
arm64_memblock_init();
paging_init();
- efi_apply_persistent_mem_reservations();
acpi_table_upgrade();
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 4c46ff6f2242..55b77c576c42 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -592,11 +592,7 @@ int __init efi_config_parse_tables(void *config_tables,
int count, int sz,
early_memunmap(tbl, sizeof(*tbl));
}
- return 0;
-}
-int __init efi_apply_persistent_mem_reservations(void)
-{
if (efi.mem_reserve != EFI_INVALID_TABLE_ADDR) {
unsigned long prsv = efi.mem_reserve;
diff --git a/drivers/firmware/efi/libstub/arm-stub.c
b/drivers/firmware/efi/libstub/arm-stub.c
index eee42d5e25ee..c037c6c5d0b7 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -75,9 +75,6 @@ void install_memreserve_table(efi_system_table_t
*sys_table_arg)
efi_guid_t memreserve_table_guid = LINUX_EFI_MEMRESERVE_TABLE_GUID;
efi_status_t status;
- if (IS_ENABLED(CONFIG_ARM))
- return;
-
status = efi_call_early(allocate_pool, EFI_LOADER_DATA, sizeof(*rsv),
(void **));
if (status != EFI_SUCCESS) {
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 45ff763fba76..28604a8d0aa9 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1198,8 +1198,6 @@ static inline bool efi_enabled(int feature)
extern void efi_reboot(enum reboot_mode reboot_mode, const char *__unused);
extern bool efi_is_table_address(unsigned long phys_addr);
-
-extern int efi_apply_persistent_mem_reservations(void);
#else
static inline bool efi_enabled(int feature)
{
@@ -1218,11 +1216,6 @@ static inline bool efi_is_table_address(unsigned long
phys_addr)
{
return false;
}
-
-static inline int efi_apply_persistent_mem_reservations(void)
-{
- return 0;
-}
#endif
extern int efi_status_to_err(efi_status_t status);
--
2.20.1
[PATCH 0/2] efi/arm/gicv3: implement fix for memory reservation issue
Another attempt at fixing the chicked-and-egg issue where the number of memblock reservations for GICv3 LPI tables overflow the statically allocated table, and reallocating it involves allocating memory pages that may turn out to be the ones we were attempting to reserve in the first place. If this is accepted as an appropriate fix, something similar should be backported to v4.19 as well, although there, we'll need to increase the memblock reservation table size even more, given that it lacks a later optimization to the EFI memreserve code to merge the linked list entries. Cc: Catalin Marinas Cc: Will Deacon Cc: Andrew Morton Cc: Marc Zyngier Cc: James Morse Cc: linux...@kvack.org Ard Biesheuvel (2): arm64: account for GICv3 LPI tables in static memblock reserve table efi/arm: Revert "Defer persistent reservations until after paging_init()" arch/arm64/include/asm/memory.h | 11 +++ arch/arm64/kernel/setup.c | 1 - drivers/firmware/efi/efi.c | 4 drivers/firmware/efi/libstub/arm-stub.c | 3 --- include/linux/efi.h | 7 --- include/linux/memblock.h| 3 --- mm/memblock.c | 10 -- 7 files changed, 19 insertions(+), 20 deletions(-) -- 2.20.1
Re: [PATCH 4/5] efi/libstub/tpm: Retrieve TPM event log in 2.0 format
On Mon, Feb 11, 2019 at 03:30:51PM +0100, b...@semihalf.com wrote: > From: Bartosz Szczepanek > > Currently, the only way to get TPM 2.0 event log from firmware is to use > device tree. Introduce efi_retrieve_tpm2_eventlog_2 function to enable > retrieving it from EFI structures. > > Include lib/tpm.c into EFI stub to calculate event sizes using helper > function. > > Signed-off-by: Bartosz Szczepanek Collides with Matthew's changes. I want to land those change first because they are almost production ready. Maybe you should consider reviewing those changes to make sure that they make sense to you so that you can build these on top of after these have landed. There is not rush as I already made my v5.1 PR. These will be released earliest in v5.2. /Jarkko
Re: [PATCH 3/5] tpm: Use library version of calc_tpm2_event_size in sysfs code
On Mon, Feb 11, 2019 at 03:30:50PM +0100, b...@semihalf.com wrote: > From: Bartosz Szczepanek > > Expect negative values from calc_tpm2_event_size as error codes. > Pass efispecid instead of event header to calc_tpm2_event_size. > > Also, include tpm library in the build. > > Signed-off-by: Bartosz Szczepanek Event log uses securityfs, not sysfs. /Jarkko
Re: [PATCH 2/5] tpm: Change calc_tpm2_event_size signature
On Mon, Feb 11, 2019 at 03:30:49PM +0100, b...@semihalf.com wrote: > From: Bartosz Szczepanek > > Pass tcg_efi_specid_event as an argument instead of tcg_pcr_event, as the > former is what is actually needed to compute event size. tcg_pcr_event > structure describes TPM event log header (even though its name), from where > efispecid can be extracted -- it seems cleaner and less misleading to do it > out of calc_tpm2_event_size function. > > Also, use ssize_t instead of int for event log size. > > Signed-off-by: Bartosz Szczepanek Unreviewable without call sites. NAK. /Jarkko
Re: [PATCH 1/5] tpm: Copy calc_tpm2_event_size() to TPM library
On Wed, Feb 13, 2019 at 01:14:43PM +0200, Jarkko Sakkinen wrote: > On Mon, Feb 11, 2019 at 03:30:48PM +0100, b...@semihalf.com wrote: > > From: Bartosz Szczepanek > > > > Function to calculate event size in TPM 2.0 log will also be needed in EFI > > stub. Separate it to library to make it accessible out of TPM character > > driver. > > > > It will be removed from tpm2.c in subsequent commit. > > > > Signed-off-by: Bartosz Szczepanek > > Collides with Matthew's patch set, which has priority over this because > it was sent earlier. > > Matthew, what you think of this? Maybe could replace 1/4 with this in > your patch set? Somehow feels a bit cleaner approach. This commit is in any case broken. It leaves two versions of the function hanging to the code base. /Jarkko
Re: [PATCH 1/5] tpm: Copy calc_tpm2_event_size() to TPM library
On Mon, Feb 11, 2019 at 03:30:48PM +0100, b...@semihalf.com wrote:
> From: Bartosz Szczepanek
>
> Function to calculate event size in TPM 2.0 log will also be needed in EFI
> stub. Separate it to library to make it accessible out of TPM character
> driver.
>
> It will be removed from tpm2.c in subsequent commit.
>
> Signed-off-by: Bartosz Szczepanek
Collides with Matthew's patch set, which has priority over this because
it was sent earlier.
Matthew, what you think of this? Maybe could replace 1/4 with this in
your patch set? Somehow feels a bit cleaner approach.
> ---
> lib/tpm.c | 80
> +++
> 1 file changed, 80 insertions(+)
> create mode 100644 lib/tpm.c
>
> diff --git a/lib/tpm.c b/lib/tpm.c
> new file mode 100644
> index ..aaeeafe52426
> --- /dev/null
> +++ b/lib/tpm.c
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2016 IBM Corporation
Do we want copyright statements to new files? I'm sure that this code
would have more copyright holders than just IBM (eg VMWare). Git
documents this anyway. This is something that will be left unmaintained.
> + * Parts of this file based on earlier work by:
> + * Nayna Jain
> + * Petr Vandrovec
Please remove these three lines. These type of lists are just inaccurate
presentaion of the commit log.
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
You already have SPDX identifier. Unncessary repeat.
> + */
> +#include
> +#include
> +#include
> +#include
> +
> +/*
> + * calc_tpm2_event_size() - calculate the event size, where event
> + * is an entry in the TPM 2.0 event log. The event is of type Crypto
> + * Agile Log Entry Format as defined in TCG EFI Protocol Specification
> + * Family "2.0".
> +
> + * @event: event whose size is to be calculated.
> + * @event_header: the first event in the event log.
> + *
> + * Returns size of the event. If it is an invalid event, returns 0.
> + */
> +int calc_tpm2_event_size(struct tcg_pcr_event2 *event,
> + struct tcg_pcr_event *event_header)
> +{
> + struct tcg_efi_specid_event *efispecid;
> + struct tcg_event_field *event_field;
> + void *marker;
> + void *marker_start;
> + u32 halg_size;
> + size_t size;
> + u16 halg;
> + int i;
> + int j;
> +
> + marker = event;
> + marker_start = marker;
> + marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type)
> + + sizeof(event->count);
> +
> + efispecid = (struct tcg_efi_specid_event *)event_header->event;
> +
> + /* Check if event is malformed. */
> + if (event->count > efispecid->num_algs)
> + return 0;
> +
> + for (i = 0; i < event->count; i++) {
> + halg_size = sizeof(event->digests[i].alg_id);
> + memcpy(, marker, halg_size);
> + marker = marker + halg_size;
> + for (j = 0; j < efispecid->num_algs; j++) {
> + if (halg == efispecid->digest_sizes[j].alg_id) {
> + marker +=
> + efispecid->digest_sizes[j].digest_size;
> + break;
> + }
> + }
> + /* Algorithm without known length. Such event is unparseable. */
> + if (j == efispecid->num_algs)
> + return 0;
> + }
> +
> + event_field = (struct tcg_event_field *)marker;
> + marker = marker + sizeof(event_field->event_size)
> + + event_field->event_size;
> + size = marker - marker_start;
> +
> + if ((event->event_type == 0) && (event_field->event_size == 0))
> + return 0;
> +
> + return size;
> +}
> +EXPORT_SYMBOL(calc_tpm2_event_size);
> --
> 2.14.4
>
Re: [PATCH v2 2/2] efi: fix build error redeclaration of enumerator
On Tue, 12 Feb 2019 at 12:21, Anders Roxell wrote:
>
> Commit a893ea15d764 ("tpm: move tpm_chip definition to
> include/linux/tpm.h") introduced a build error when both ima and efi is
> enabled. What happens is that both headers (ima.h and efi.h) defines the
> same 'NONE' constant, and it broke when they started getting included
> from the same file.
>
> In file included from ../security/integrity/ima/ima_fs.c:30:
> ../security/integrity/ima/ima.h:176:7: error: redeclaration of enumerator
> "NONE"
> hook(NONE) \
>^~~~
> ../security/integrity/ima/ima.h:188:34: note: in definition of macro
> "__ima_hook_enumify"
> #define __ima_hook_enumify(ENUM) ENUM,
> ^~~~
> ../security/integrity/ima/ima.h:191:2: note: in expansion of macro
> "__ima_hooks"
> __ima_hooks(__ima_hook_enumify)
> ^~~
> In file included from ../arch/arm64/include/asm/acpi.h:15,
> from ../include/acpi/acpi_io.h:7,
> from ../include/linux/acpi.h:47,
> from ../include/linux/tpm.h:26,
> from ../security/integrity/ima/ima.h:25,
> from ../security/integrity/ima/ima_fs.c:30:
> ../include/linux/efi.h:1723:2: note: previous definition of "NONE" was here
> NONE,
> ^~~~
> make[4]: *** [../scripts/Makefile.build:277: security/integrity/ima/ima_fs.o]
> Error 1
>
> Rework to prefix the efi enum with 'EFI_*'.
>
> Signed-off-by: Anders Roxell
> ---
>
> No changes since v1.
>
> arch/x86/platform/efi/quirks.c | 2 +-
I found that I missed rename one 'NONE' to 'EFI_NONE' in the file above.
Will send a v3 shortly.
Cheers,
Anders
> drivers/firmware/efi/runtime-wrappers.c | 48 -
> include/linux/efi.h | 26 +++---
> 3 files changed, 38 insertions(+), 38 deletions(-)
>
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 9ce85e605052..b7c0b04ee6ad 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -742,7 +742,7 @@ void efi_recover_from_page_fault(unsigned long phys_addr)
> * because this case occurs *very* rarely and hence could be improved
> * on a need by basis.
> */
> - if (efi_rts_work.efi_rts_id == RESET_SYSTEM) {
> + if (efi_rts_work.efi_rts_id == EFI_RESET_SYSTEM) {
> pr_info("efi_reset_system() buggy! Reboot through BIOS\n");
> machine_real_restart(MRR_BIOS);
> return;
> diff --git a/drivers/firmware/efi/runtime-wrappers.c
> b/drivers/firmware/efi/runtime-wrappers.c
> index c70df5ae7c4a..28138534643e 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -85,7 +85,7 @@ struct efi_runtime_work efi_rts_work;
> pr_err("Failed to queue work to efi_rts_wq.\n");\
> \
> exit: \
> - efi_rts_work.efi_rts_id = NONE; \
> + efi_rts_work.efi_rts_id = EFI_NONE; \
> efi_rts_work.status;\
> })
>
> @@ -181,50 +181,50 @@ static void efi_call_rts(struct work_struct *work)
> arg5 = efi_rts_work.arg5;
>
> switch (efi_rts_work.efi_rts_id) {
> - case GET_TIME:
> + case EFI_GET_TIME:
> status = efi_call_virt(get_time, (efi_time_t *)arg1,
>(efi_time_cap_t *)arg2);
> break;
> - case SET_TIME:
> + case EFI_SET_TIME:
> status = efi_call_virt(set_time, (efi_time_t *)arg1);
> break;
> - case GET_WAKEUP_TIME:
> + case EFI_GET_WAKEUP_TIME:
> status = efi_call_virt(get_wakeup_time, (efi_bool_t *)arg1,
>(efi_bool_t *)arg2, (efi_time_t
> *)arg3);
> break;
> - case SET_WAKEUP_TIME:
> + case EFI_SET_WAKEUP_TIME:
> status = efi_call_virt(set_wakeup_time, *(efi_bool_t *)arg1,
>(efi_time_t *)arg2);
> break;
> - case GET_VARIABLE:
> + case EFI_GET_VARIABLE:
> status = efi_call_virt(get_variable, (efi_char16_t *)arg1,
>(efi_guid_t *)arg2, (u32 *)arg3,
>(unsigned long *)arg4, (void *)arg5);
> break;
> - case GET_NEXT_VARIABLE:
> + case EFI_GET_NEXT_VARIABLE:
> status = efi_call_virt(get_next_variable, (unsigned long
> *)arg1,
>(efi_char16_t *)arg2,
>(efi_guid_t *)arg3);
> break;
> - case SET_VARIABLE:
> + case EFI_SET_VARIABLE:
>
