Re: [PATCH] firmware: efi: add NULL pointer checks in efivars api functions
On 11/15/2018 2:54 PM, Ard Biesheuvel wrote:
On Tue, 13 Nov 2018 at 15:04, Arend van Spriel
wrote:
On 11/13/2018 11:50 PM, Ard Biesheuvel wrote:
Hi Arend,
On Tue, 13 Nov 2018 at 12:51, Arend van Spriel
wrote:
Since commit ce2e6db554fa ("brcmfmac: Add support for getting nvram
contents from EFI variables") we have a device driver accessing the
efivars api (since next-20181107). Several functions in efivars api
assume __efivars is set, ie. will be accessed after efivars_register()
has been called. However, following NULL pointer access was reported
calling efivar_entry_size() from the brcmfmac device driver.
[ 14.177769] Unable to handle kernel NULL pointer dereference at
virtual address 0008
[ 14.197303] pgd = 60bfa5f1
[ 14.211842] [0008] *pgd=
[ 14.227373] Internal error: Oops: 5 [#1] SMP ARM
[ 14.244244] Modules linked in: brcmfmac sha256_generic sha256_arm snd
cfg80211 brcmutil soundcore snd_soc_tegra30_ahub tegra_wdt
[ 14.269109] CPU: 1 PID: 114 Comm: kworker/1:2 Not tainted
4.20.0-rc1-next-20181107-gd881de3 #1
[ 14.269114] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[ 14.269154] Workqueue: events request_firmware_work_func
[ 14.269177] PC is at efivar_entry_size+0x28/0x90
[ 14.269362] LR is at brcmf_fw_complete_request+0x3f8/0x8d4 [brcmfmac]
[ 14.269369] pc : []lr : []psr: a00d0113
[ 14.269374] sp : ede7fe28 ip : ee983410 fp : c1787f30
[ 14.269378] r10: r9 : r8 : bf2b2258
[ 14.269384] r7 : ee983000 r6 : c1604c48 r5 : ede7fe88 r4 : edf337c0
[ 14.269389] r3 : r2 : r1 : ede7fe88 r0 : c17712c8
[ 14.269398] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
[ 14.269404] Control: 10c5387d Table: ad16804a DAC: 0051
Disassembly showed that the local static variable __efivars is NULL. Likely
because efivars_register() is not called on this Tegra platform. So adding
a NULL pointer check in efivar_entry_size() and similar functions while at
it. In efivars_register() a couple of sanity checks have been added.
Cc: Hans de Goede
Reported-by: Jon Hunter
Signed-off-by: Arend van Spriel
---
drivers/firmware/efi/vars.c | 108 +++-
1 file changed, 87 insertions(+), 21 deletions(-)
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 9336ffd..5fbd8e7 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -318,7 +318,12 @@ struct variable_validate {
static efi_status_t
check_var_size(u32 attributes, unsigned long size)
{
- const struct efivar_operations *fops = __efivars->ops;
+ const struct efivar_operations *fops;
+
+ if (!__efivars)
+ return EFI_UNSUPPORTED;
+
+ fops = __efivars->ops;
if (!fops->query_variable_store)
return EFI_UNSUPPORTED;
@@ -329,7 +334,12 @@ struct variable_validate {
static efi_status_t
check_var_size_nonblocking(u32 attributes, unsigned long size)
{
- const struct efivar_operations *fops = __efivars->ops;
+ const struct efivar_operations *fops;
+
+ if (!__efivars)
+ return EFI_UNSUPPORTED;
+
+ fops = __efivars->ops;
if (!fops->query_variable_store)
return EFI_UNSUPPORTED;
@@ -429,13 +439,18 @@ static void dup_variable_bug(efi_char16_t *str16,
efi_guid_t *vendor_guid,
int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
void *data, bool duplicates, struct list_head *head)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
unsigned long variable_name_size = 1024;
efi_char16_t *variable_name;
efi_status_t status;
efi_guid_t vendor_guid;
int err = 0;
+ if (!__efivars)
+ return -EFAULT;
+
+ ops = __efivars->ops;
+
variable_name = kzalloc(variable_name_size, GFP_KERNEL);
if (!variable_name) {
printk(KERN_ERR "efivars: Memory allocation failed.\n");
@@ -583,12 +598,14 @@ static void efivar_entry_list_del_unlock(struct
efivar_entry *entry)
*/
int __efivar_entry_delete(struct efivar_entry *entry)
{
- const struct efivar_operations *ops = __efivars->ops;
efi_status_t status;
- status = ops->set_variable(entry->var.VariableName,
- &entry->var.VendorGuid,
- 0, 0, NULL);
+ if (!__efivars)
+ return -EINVAL;
+
+ status = __efivars->ops->set_variable(entry->var.VariableName,
+ &entry->var.VendorGuid,
+ 0, 0, NULL);
return efi_status_to_err(status);
}
@@ -607,12 +624,17 @@ int __efivar_entry_delete(struct efivar_entry *entry)
*/
int efivar_entry_delete(struct efivar_entry *entry)
{
- const struct efivar_operat
Re: [PATCH v11 5/5] x86/boot/KASLR: Walk srat tables to filter immovable memory
Subject: Re: [PATCH v11 5/5] x86/boot/KASLR: Walk srat tables to filter
immovable memory
s/srat/SRAT/g
On Mon, Nov 12, 2018 at 05:46:45PM +0800, Chao Fan wrote:
> KASLR may randomly chooses some positions which are located in movable
choose
> memory regions. This will break memory hotplug feature and make the
> movable memory chosen by KASLR can't be removed.
by KASLR practically immovable.
:)
> The solution is limite KASLR to choose memory regions in immovable
limite?
"to limit"
> node according to SRAT tables.
>
> If CONFIG_MEMORY_HOTREMOVE enabled, walk through the SRAT memory
*is* enabled,
> tables and store those immovable memory regions so that KASLR can get
> where to choose for randomization.
>
> If the amount of immovable memory regions is not zero, which
> means the immovable memory regions existing. Calculate the intersection
> between memory regions from e820/efi memory table and immovable memory
> regions.
This is explaining *what* the patch does and generally doesn't need to
be in the commit messge as people can read it in the patch itself.
> Signed-off-by: Chao Fan
> ---
> arch/x86/boot/compressed/kaslr.c | 77 +++-
> 1 file changed, 66 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c
> b/arch/x86/boot/compressed/kaslr.c
> index b251572e77af..174d2114045e 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -97,6 +97,11 @@ static bool memmap_too_large;
> /* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
> static unsigned long long mem_limit = ULLONG_MAX;
>
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +/* Store the immovable memory regions */
> +extern struct mem_vector immovable_mem[MAX_NUMNODES*2];
> +#endif
For this and the other occurrences of ifdef CONFIG_MEMORY_HOTREMOVE,
define empty stubs for those functions in a header and remove the
ifdeffery at the call sites.
> +
>
> enum mem_avoid_index {
> MEM_AVOID_ZO_RANGE = 0,
> @@ -413,6 +418,11 @@ static void mem_avoid_init(unsigned long input, unsigned
> long input_size,
> /* Mark the memmap regions we need to avoid */
> handle_mem_options();
>
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> + /* Mark the immovable regions we need to choose */
> + get_immovable_mem();
> +#endif
> +
> #ifdef CONFIG_X86_VERBOSE_BOOTUP
> /* Make sure video RAM can be used. */
> add_identity_map(0, PMD_SIZE);
> @@ -568,9 +578,9 @@ static unsigned long slots_fetch_random(void)
> return 0;
> }
>
> -static void process_mem_region(struct mem_vector *entry,
> -unsigned long minimum,
> -unsigned long image_size)
> +static void slots_count(struct mem_vector *entry,
That's a strange rename.
__process_mem_region() makes more sense to me.
> + unsigned long minimum,
> + unsigned long image_size)
> {
> struct mem_vector region, overlap;
> unsigned long start_orig, end;
> @@ -646,6 +656,57 @@ static void process_mem_region(struct mem_vector *entry,
> }
> }
>
> +static bool process_mem_region(struct mem_vector *region,
> +unsigned long long minimum,
> +unsigned long long image_size)
> +{
> + int i;
> + /*
> + * If no immovable memory found, or MEMORY_HOTREMOVE disabled,
> + * walk all the regions, so use region directely.
"directly"
> + */
> + if (num_immovable_mem == 0) {
if (!...
> + slots_count(region, minimum, image_size);
> +
> + if (slot_area_index == MAX_SLOT_AREA) {
> + debug_putstr("Aborted e820/efi memmap scan (slot_areas
> full)!\n");
> + return 1;
> + }
> + return 0;
> + }
> +
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory
On Mon, Nov 12, 2018 at 05:46:44PM +0800, Chao Fan wrote:
> To avoid KASLR extracting kernel on movable memory, slove the
^
Please introduce a spellchecker into your patch creation workflow.
> conflict between KASLR and movable_node feature, dig the SRAT tables
s/dig/determine/ or "compute SRAT table's address" or so.
Also, replace "dig" with a more suitable verb in all your patches.
> from RSDP pointer. Walk the SRAT tables and store the immovable
> memory regions in immovable_mem[].
"... in an array called immovable_mem[]."
> There are three methods to get RSDP pointer: KEXEC condition,
> EFI confition, BIOS condition.
"condition" is not the right word here.
> If KEXEC add 'acpi_rsdp' to cmdline, use it.
> Otherwise, parse EFI table for RSDP.
> Then, search memory for RSDP.
>
> Imitate from ACPI code, based on acpi_os_get_root_pointer().
> Process: RSDP->RSDT/XSDT->ACPI root table->SRAT.
What?!
This looks like a comment you've added as a note for yourself but not
part of the final commit message. If you wanna explain the process, then
write it out in plain english as if you're explaining it to someone who
doesn't know what you're doing.
>
> Signed-off-by: Chao Fan
> ---
> arch/x86/boot/compressed/Makefile | 4 +
> arch/x86/boot/compressed/acpitb.c | 139 ++
> arch/x86/boot/compressed/kaslr.c | 4 -
> arch/x86/boot/compressed/misc.h | 15
> 4 files changed, 158 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/Makefile
> b/arch/x86/boot/compressed/Makefile
> index 466f66c8a7f8..b51f7629b8ef 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -84,6 +84,10 @@ ifdef CONFIG_X86_64
> vmlinux-objs-y += $(obj)/pgtable_64.o
> endif
>
> +#if (defined CONFIG_MEMORY_HOTREMOVE) && (defined CONFIG_RANDOMIZE_BASE)
> +vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
> +#endif
Right, as previously pointed out, this needs that CONFIG_ symbol and
then you can save yourself most (if not all) of the ifdeffery in the
rest of the patchset.
> +
> $(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone
>
> vmlinux-objs-$(CONFIG_EFI_STUB) += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \
> diff --git a/arch/x86/boot/compressed/acpitb.c
> b/arch/x86/boot/compressed/acpitb.c
> index 5cfb4efa5a19..161f21a7fb3b 100644
> --- a/arch/x86/boot/compressed/acpitb.c
> +++ b/arch/x86/boot/compressed/acpitb.c
> @@ -14,6 +14,11 @@
> #define BOOT_STRING
> #include "../string.h"
>
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +/* Store the immovable memory regions */
> +struct mem_vector immovable_mem[MAX_NUMNODES*2];
> +#endif
> +
> /* Search EFI table for RSDP table. */
> static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
> {
> @@ -226,3 +231,137 @@ static void get_acpi_rsdp(acpi_physical_address
> *rsdp_addr)
> }
> #endif
> }
> +
> +/*
> + * Used to dig RSDP table from EFI table or BIOS.
> + * If RSDP table found in EFI table, use it. Or search BIOS.
> + * Based on acpi_os_get_root_pointer().
> + */
> +static acpi_physical_address get_rsdp_addr(void)
> +{
> + acpi_physical_address pa = 0;
> +
> + get_acpi_rsdp(&pa);
> +
> + if (!pa)
> + efi_get_rsdp_addr(&pa);
> +
> + if (!pa)
> + bios_get_rsdp_addr(&pa);
> +
> + return pa;
> +}
> +
> +static struct acpi_table_header *get_acpi_srat_table(void)
> +{
> + acpi_physical_address acpi_table;
> + acpi_physical_address root_table;
> + struct acpi_table_header *header;
> + struct acpi_table_rsdp *rsdp;
> + bool acpi_use_rsdt = false;
> + char *signature;
> + char arg[10];
> + u8 *entry;
> + u32 count;
> + u32 size;
> + int i, j;
> + int ret;
> + u32 len;
> +
> + rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
> + if (!rsdp)
> + return NULL;
> +
> + ret = cmdline_find_option("acpi", arg, sizeof(arg));
> + if (ret == 4 && !strncmp(arg, "rsdt", 4))
> + acpi_use_rsdt = true;
> +
> + /* Get RSDT or XSDT from RSDP. */
> + if (!acpi_use_rsdt &&
> + rsdp->xsdt_physical_address && rsdp->revision > 1) {
> + root_table = rsdp->xsdt_physical_address;
> + size = ACPI_XSDT_ENTRY_SIZE;
> + } else {
> + root_table = rsdp->rsdt_physical_address;
> + size = ACPI_RSDT_ENTRY_SIZE;
> + }
Reorganize that code here to get rid of acpi_use_rsdt.
> +
> + /* Get ACPI root table from RSDT or XSDT.*/
> + header = (struct acpi_table_header *)root_table;
> + len = header->length;
No checking of that header pointer before dereffing it?
If it is NUL, that gives you a very nasty bug to try to debug in the
early code.
> + count = (u32)((len - sizeof(struct acpi_table_header)) / size);
Uuh, no checking for count wrapping around here due to wrong len? That
would give you
