[PATCH V34 28/29] efi: Restrict efivar_ssdt_load when the kernel is locked down
efivar_ssdt_load allows the kernel to import arbitrary ACPI code from an
EFI variable, which gives arbitrary code execution in ring 0. Prevent
that when the kernel is locked down.
Signed-off-by: Matthew Garrett
Cc: Ard Biesheuvel
Cc: linux-efi@vger.kernel.org
---
drivers/firmware/efi/efi.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 55b77c576c42..9f92a013ab27 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -31,6 +31,7 @@
#include
#include
#include
+#include
#include
@@ -242,6 +243,11 @@ static void generic_ops_unregister(void)
static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata;
static int __init efivar_ssdt_setup(char *str)
{
+ int ret = security_locked_down(LOCKDOWN_ACPI_TABLES);
+
+ if (ret)
+ return ret;
+
if (strlen(str) < sizeof(efivar_ssdt))
memcpy(efivar_ssdt, str, strlen(str));
else
--
2.22.0.410.gd8fdbe21b5-goog
Re: [PATCH v3 07/10] lib/memregion: Uplevel the pmem "region" ida to a global allocator
On Fri, Jun 7, 2019 at 1:23 PM Matthew Wilcox wrote:
>
> On Fri, Jun 07, 2019 at 12:27:50PM -0700, Dan Williams wrote:
> > diff --git a/lib/memregion.c b/lib/memregion.c
> > new file mode 100644
> > index ..f6c6a94c7921
> > --- /dev/null
> > +++ b/lib/memregion.c
> > @@ -0,0 +1,15 @@
> > +#include
> > +
> > +static DEFINE_IDA(region_ids);
> > +
> > +int memregion_alloc(gfp_t gfp)
> > +{
> > + return ida_alloc(®ion_ids, gfp);
> > +}
> > +EXPORT_SYMBOL(memregion_alloc);
> > +
> > +void memregion_free(int id)
> > +{
> > + ida_free(®ion_ids, id);
> > +}
> > +EXPORT_SYMBOL(memregion_free);
>
> Does this trivial abstraction have to live in its own file? I'd make
> memregion_alloc/free static inlines that live in a header file, then
> all you need do is find a suitable .c file to store memregion_ids in,
> and export that one symbol instead of two.
Ok, I think since these "memregion" objects tend to be closely related
to "device memory" I'll stash this in kernel/memremap.c with the rest
of the "ZONE_DEVICE" apis.
Re: [PATCH v2 4/8] x86, efi: Reserve UEFI 2.8 Specific Purpose Memory for dax
On Sat, Jun 8, 2019 at 12:20 AM Ard Biesheuvel
wrote:
>
> On Fri, 7 Jun 2019 at 19:34, Dan Williams wrote:
> >
> > On Fri, Jun 7, 2019 at 8:23 AM Dan Williams
> > wrote:
> > >
> > > On Fri, Jun 7, 2019 at 5:29 AM Ard Biesheuvel
> > > wrote:
> > [..]
> > > > > #ifdef CONFIG_EFI_APPLICATION_RESERVED
> > > > > static inline bool is_efi_application_reserved(efi_memory_desc_t *md)
> > > > > {
> > > > > return md->type == EFI_CONVENTIONAL_MEMORY
> > > > > && (md->attribute & EFI_MEMORY_SP);
> > > > > }
> > > > > #else
> > > > > static inline bool is_efi_application_reserved(efi_memory_desc_t *md)
> > > > > {
> > > > > return false;
> > > > > }
> > > > > #endif
> > > >
> > > > I think this policy decision should not live inside the EFI subsystem.
> > > > EFI just gives you the memory map, and mangling that information
> > > > depending on whether you think a certain memory attribute should be
> > > > ignored is the job of the MM subsystem.
> > >
> > > The problem is that we don't have an mm subsystem at the time a
> > > decision needs to be made. The reservation policy needs to be deployed
> > > before even memblock has been initialized in order to keep kernel
> > > allocations out of the reservation. I agree with the sentiment I just
> > > don't see how to practically achieve an optional "System RAM" vs
> > > "Application Reserved" routing decision without an early (before
> > > e820__memblock_setup()) conditional branch.
> >
> > I can at least move it out of include/linux/efi.h and move it to
> > arch/x86/include/asm/efi.h since it is an x86 specific policy decision
> > / implementation for now.
>
> No, that doesn't make sense to me. If it must live in the EFI
> subsystem, I'd prefer it to be in the core code, not in x86 specific
> code, since there is nothing x86 specific about it.
The decision on whether / if to take any action on this hint is
implementation specific, so I argue it does not belong in the EFI
core. The spec does not mandate any action as it's just a hint.
Instead x86 is making a policy decision in how it translates it to the
x86-specific E820 representation. So, I as I go to release v3 of this
patch set I do not see an argument to move the
is_efi_application_reserved() definition out of
arch/x86/include/asm/efi.h it's 100% tied to the e820 translation.
Now, if some other EFI supporting architecture wanted to follow the
x86 policy we could move it it to a shared location, but that's
something for a follow-on patch set.
Re: [RFC PATCH 1/6] efi / ras: CCIX Memory error reporting
On Thu, 6 Jun 2019 20:36:49 +0800
Jonathan Cameron wrote:
> CCIX defines a number of different error types
> (See CCIX spec 1.0) and UEFI 2.8 defines a CPER record to allow
> for them to be reported when firmware first handling is in use.
> The last part of that record is a copy of the CCIX protocol
> error record which can provide very detailed information.
>
> This patch introduces infrastructure and support for one of those
> error types, CCIX Memory Errors. Later patches will supply
> equivalent support for the other error types.
>
> The variable length and content of the different messages makes
> a single tracepoint impractical. As such the current RAS
> tracepoint only covers the memory error. Additional trace points
> will be introduced for other error types along with their
> cper handling in a follow up series.
>
> RAS daemon support to follow shortly. qemu injection patches
> also available but not currently planing to upstream those.
>
> Signed-off-by: Jonathan Cameron
As this is still and RFC I'm not going to spin a new version yet,
but we need some ifdef fun in the event header as it's calling
functions much like extlog_mem_event does.
I'll roll that fix into v2 once people have had time to look at
this version.
Thanks,
Jonathan
> ---
> drivers/acpi/apei/Kconfig| 8 +
> drivers/acpi/apei/ghes.c | 39
> drivers/firmware/efi/Kconfig | 5 +
> drivers/firmware/efi/Makefile| 1 +
> drivers/firmware/efi/cper-ccix.c | 356 +++
> drivers/firmware/efi/cper.c | 6 +
> include/linux/cper.h | 118 ++
> include/ras/ras_event.h | 77 +++
> 8 files changed, 610 insertions(+)
>
> diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
> index 6b18f8bc7be35..e687b18dee344 100644
> --- a/drivers/acpi/apei/Kconfig
> +++ b/drivers/acpi/apei/Kconfig
> @@ -68,3 +68,11 @@ config ACPI_APEI_ERST_DEBUG
> error information to and from a persistent store. Enable this
> if you want to debugging and testing the ERST kernel support
> and firmware implementation.
> +
> +config ACPI_APEI_CCIX
> + bool "APEI CCIX error recovery support"
> + depends on ACPI_APEI && MEMORY_FAILURE
> + help
> + CCIX has a number of defined error types. This option enables
> + the handling of CPER records generated by a firmware performing
> + firmware first error handling of these CCIX errors.
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 993940d582f50..cfc7dc31a9380 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -477,6 +477,42 @@ static void ghes_handle_aer(struct
> acpi_hest_generic_data *gdata)
> #endif
> }
>
> +static void ghes_handle_ccix_per(struct acpi_hest_generic_data *gdata, int
> sev)
> +{
> +#ifdef CONFIG_ACPI_APEI_CCIX
> + struct cper_sec_ccix_header *header = acpi_hest_get_payload(gdata);
> + __u32 *dw;
> + enum ccix_per_type per_type;
> + static u32 err_seq;
> + void *payload;
> +
> + /* Check if space for CCIX CPER header and 8 DW of a PER log header */
> + if (gdata->error_data_length <
> + sizeof(*header) + CCIX_PER_LOG_HEADER_DWS * sizeof(__u32))
> + return;
> +
> + if ((header->validation_bits & CPER_CCIX_VALID_PER_LOG) == 0)
> + return;
> +
> + dw = (__u32 *)(header + 1);
> +
> + per_type = FIELD_GET(CCIX_PER_LOG_DW1_PER_TYPE_M, dw[1]);
> + payload = acpi_hest_get_payload(gdata);
> +
> + switch (per_type) {
> + case CCIX_MEMORY_ERROR:
> + trace_ccix_memory_error_event(payload, err_seq, sev,
> +
> ccix_mem_err_ven_len_get(payload));
> + break;
> + default:
> + /* Unknown error type */
> + pr_info("CCIX error of unknown or vendor defined type\n");
> + break;
> + }
> + err_seq++;
> +#endif
> +}
> +
> static void ghes_do_proc(struct ghes *ghes,
>const struct acpi_hest_generic_status *estatus)
> {
> @@ -507,6 +543,9 @@ static void ghes_do_proc(struct ghes *ghes,
> else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
> ghes_handle_aer(gdata);
> }
> + else if (guid_equal(sec_type, &CPER_SEC_CCIX)) {
> + ghes_handle_ccix_per(gdata, estatus->error_severity);
> + }
> else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
> struct cper_sec_proc_arm *err =
> acpi_hest_get_payload(gdata);
>
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index d4ea929e8b344..9ea161f68da8d 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -195,6 +195,11 @@ config UEFI_CPER_X86
> depends on UEFI_CPER && X86
> default y
>
> +config UEFI_CPER_CCIX
> + bool
> + depends on UEF
Re: [RFC PATCH] Export Runtime Configuration Interface table to sysfs
(+ Peter)
On Mon, 17 Jun 2019 at 12:11, wrote:
>
> From: Narendra K
>
> System firmware advertises the address of the 'Runtime
> Configuration Interface table version 2 (RCI2)' via
> an EFI Configuration Table entry. This code retrieves the RCI2
> table from the address and exports it to sysfs as a binary
> attribute 'rci2' under /sys/firmware/efi/tables directory.
> The approach adopted is similar to the attribute 'DMI' under
> /sys/firmware/dmi/tables.
>
> RCI2 table contains BIOS HII in XML format and is used to populate
> BIOS setup page in Dell EMC OpenManage Server Administrator tool.
> The BIOS setup page contains BIOS tokens which can be configured.
>
> Signed-off-by: Narendra K
> ---
> Hi, the patch is created on kernel version 5.2-rc4. It applies to
> 5.2-rc5 also. If the approach looks correct, I will resubmit with RFC
> tag removed.
>
Unfortunately, we cannot implement a generic interface for dumping
config tables, since there is no generic method to record the length.
So I don't have any problems with doing it this way.
I do have some comments, though.
First of all, do you know which memory type is used for this table? (more below)
> Documentation/ABI/testing/sysfs-firmware-efi | 9 ++
> drivers/firmware/efi/Makefile| 2 +-
> drivers/firmware/efi/efi.c | 20 ++-
> drivers/firmware/efi/rci2_table.c| 148 +++
> include/linux/efi.h | 7 +
> 5 files changed, 184 insertions(+), 2 deletions(-)
> create mode 100644 drivers/firmware/efi/rci2_table.c
>
> diff --git a/Documentation/ABI/testing/sysfs-firmware-efi
> b/Documentation/ABI/testing/sysfs-firmware-efi
> index e794eac32a90..cb887b5e10cb 100644
> --- a/Documentation/ABI/testing/sysfs-firmware-efi
> +++ b/Documentation/ABI/testing/sysfs-firmware-efi
> @@ -28,3 +28,12 @@ Description: Displays the physical addresses of all EFI
> Configuration
> versions are always printed first, i.e. ACPI20 comes
> before ACPI.
> Users: dmidecode
> +
> +What: /sys/firmware/efi/tables/rci2
> +Date: June 2019
> +Contact: Narendra K , linux-b...@dell.com
> +Description: Displays the content of the Runtime Configuration Interface
> + Table version 2 on Dell EMC PowerEdge systems in binary format
> +Users: It is used by Dell EMC OpenManage Server Administrator tool to
> + populate BIOS setup page.
> +
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index d2d0d2030620..db07828ca1ed 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -11,7 +11,7 @@
> KASAN_SANITIZE_runtime-wrappers.o := n
>
> obj-$(CONFIG_ACPI_BGRT)+= efi-bgrt.o
> -obj-$(CONFIG_EFI) += efi.o vars.o reboot.o memattr.o
> tpm.o
> +obj-$(CONFIG_EFI) += efi.o vars.o reboot.o memattr.o
> tpm.o rci2_table.o
> obj-$(CONFIG_EFI) += capsule.o memmap.o
> obj-$(CONFIG_EFI_VARS) += efivars.o
> obj-$(CONFIG_EFI_ESRT) += esrt.o
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 16b2137d117c..2fe114ff8149 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -53,6 +53,7 @@ struct efi __read_mostly efi = {
> .rng_seed = EFI_INVALID_TABLE_ADDR,
> .tpm_log= EFI_INVALID_TABLE_ADDR,
> .mem_reserve= EFI_INVALID_TABLE_ADDR,
> + .rci2 = EFI_INVALID_TABLE_ADDR,
Does this really need to live in the efi struct?
> };
> EXPORT_SYMBOL(efi);
>
> @@ -73,6 +74,7 @@ static unsigned long *efi_tables[] = {
> &efi.esrt,
> &efi.properties_table,
> &efi.mem_attr_table,
> + &efi.rci2,
> };
>
AFAICT, this table is only used by memremap_is_efi_data() to decide
whether a page should be map as unencrypted, and if the address is in
boot services data or runtime services data, the test will already
success, regardless of whether it appears in this enumeration.
> struct mm_struct efi_mm = {
> @@ -384,6 +386,9 @@ static int __init efisubsys_init(void)
> goto err_remove_group;
> }
>
> + if (efi_rci2_sysfs_init() != 0)
> + pr_debug("efi rci2: sysfs attribute creation under
> /sys/firmware/efi/ failed");
> +
> return 0;
>
> err_remove_group:
> @@ -488,6 +493,12 @@ static __initdata efi_config_table_type_t
> common_tables[] = {
> {NULL_GUID, NULL, NULL},
> };
>
> +/* OEM Tables */
> +static __initdata efi_config_table_type_t oem_tables[] = {
> + {DELLEMC_EFI_RCI2_TABLE_GUID, "RCI2", &efi.rci2},
Please drop the string. We don't have to print the presence of this
table in the bootlog since it has no significance to the OS itself.
> + {NULL_GUID, NULL, NULL},
> +};
> +
Do we really need a separate oem_tables[
Re: [[efi boot control]] efibc: Replace variable set function in notifier call
On Wed, 12 Jun 2019 at 10:20, wrote:
>
> From: Tian Baofeng
>
> Replace the variable set function from "efivar_entry_set" to
> "efivar_entry_set_safe" in efibc panic notifier.
> In safe function parameter "block" will set to false
> and will call "efivar_entry_set_nonblocking"to set efi variables.
> efivar_entry_set_nonblocking is guaranteed to
> not block and is suitable for calling from crash/panic handlers.
> In UEFI android platform, when warm reset happens,
> with this change, efibc will not block the reboot process.
> Otherwise, set variable will call queue work and send to other offlined
> cpus then cause another panic, finally will cause reboot failure.
>
> Signed-off-by: Tian Baofeng
> Signed-off-by: Luo XinanX
Queued as a fix in efi/urgent
Thanks
> ---
> drivers/firmware/efi/efibc.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
> index 61e099826cbb..35dccc88ac0a 100644
> --- a/drivers/firmware/efi/efibc.c
> +++ b/drivers/firmware/efi/efibc.c
> @@ -43,11 +43,13 @@ static int efibc_set_variable(const char *name, const
> char *value)
> efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
> memcpy(&entry->var.VendorGuid, &guid, sizeof(guid));
>
> - ret = efivar_entry_set(entry,
> - EFI_VARIABLE_NON_VOLATILE
> - | EFI_VARIABLE_BOOTSERVICE_ACCESS
> - | EFI_VARIABLE_RUNTIME_ACCESS,
> - size, entry->var.Data, NULL);
> + ret = efivar_entry_set_safe(entry->var.VariableName,
> + entry->var.VendorGuid,
> + EFI_VARIABLE_NON_VOLATILE
> + | EFI_VARIABLE_BOOTSERVICE_ACCESS
> + | EFI_VARIABLE_RUNTIME_ACCESS,
> + false, size, entry->var.Data);
> +
> if (ret)
> pr_err("failed to set %s EFI variable: 0x%x\n",
>name, ret);
> --
> 2.21.0
>
Re: [PATCH v2] x86/efi: fix a -Wtype-limits compilation warning
On Wed, 19 Jun 2019 at 19:53, Prakhya, Sai Praneeth
wrote:
>
> > Compiling a kernel with W=1 generates this warning,
> >
> > arch/x86/platform/efi/quirks.c:731:16: warning: comparison of unsigned
> > expression >= 0 is always true [-Wtype-limits]
> >
> > Fixes: 3425d934fc03 ("efi/x86: Handle page faults occurring while running
> > EFI
> > runtime services")
> > Signed-off-by: Qian Cai
> > ---
> >
> > v2: Add a "Fixes" tag.
>
> Makes sense.
> Thanks for the fix Qian Cai.
>
Queued as a fix with Sai's ack
Thanks,
