Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
Hi Boris, Masa, and Baoquan,
On Tue, Nov 13, 2018 at 10:51:56PM +0100, Borislav Petkov wrote:
>On Tue, Nov 13, 2018 at 03:06:16PM -0500, Masayoshi Mizuma wrote:
>> I just felt the BOOT_STRING thing in lib/kstrtox.c confuses...
>> I'm OK for now if it's applied your below comment.
>
>Well, actually, upon a second look, I don't think that including a .c
>file into a header is ok:
>
>diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
>index 3d78e27077f4..0ff3edb888e4 100644
>--- a/arch/x86/boot/string.h
>+++ b/arch/x86/boot/string.h
>@@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp,
>char **endp,
> unsigned int base);
>
> #endif /* BOOT_STRING_H */
>+
>+#ifdef BOOT_STRING
>+#include "../../../lib/kstrtox.c"
>+#endif
>
>Chao, why isn't this part of arch/x86/boot/compressed/misc.c ?
>
Fine, I have put it to misc.c:
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 8dd1d5ccae58..714b05b65a33 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -426,3 +426,7 @@ void fortify_panic(const char *name)
{
error("detected buffer overflow");
}
+
+#ifdef BOOT_STRING
+#include "../../../../lib/kstrtox.c"
+#endif
And define it in misc.h:
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 4a3645fda0ed..98e28c4281ee 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -131,3 +131,5 @@ int num_immovable_mem;
void get_immovable_mem(void);
#endif
#endif
+#define BOOT_STRING
+extern int kstrtoull(const char *s, unsigned int base, unsigned long long
*res);
But isdigit() would be redefine, so:
diff --git a/include/linux/ctype.h b/include/linux/ctype.h
index 363b004426db..aba01c385232 100644
--- a/include/linux/ctype.h
+++ b/include/linux/ctype.h
@@ -23,10 +23,12 @@ extern const unsigned char _ctype[];
#define isalnum(c) ((__ismask(c)&(_U|_L|_D)) != 0)
#define isalpha(c) ((__ismask(c)&(_U|_L)) != 0)
#define iscntrl(c) ((__ismask(c)&(_C)) != 0)
+#ifndef BOOT_STRING
static inline int isdigit(int c)
{
return '0' <= c && c <= '9';
}
+#endif
#define isgraph(c) ((__ismask(c)&(_P|_U|_L|_D)) != 0)
#define islower(c) ((__ismask(c)&(_L)) != 0)
#define isprint(c) ((__ismask(c)&(_P|_U|_L|_D|_SP)) != 0)
Now I can make it.
I wonder whether this is OK to cover isdigit() with 'BOOT_STRING' tag.
Thanks,
Chao Fan
>--
>Regards/Gruss,
>Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>
Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
On Wed, Nov 14, 2018 at 09:54:50AM +0800, Chao Fan wrote:
>On Tue, Nov 13, 2018 at 06:51:50PM +0100, Borislav Petkov wrote:
>>On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
>>> Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
>>> KEXEC writes the RSDP pointer to cmdline for EFI booting.
>>> So if 'acpi_rsdp' found in cmdline, use it directely.
>>>
>>> Since function kstrtoull() is needed, include it in
>>> arch/x86/boot/string.h. To solve the definition conflict
>>> problem, set BOOT_STRING tag to expose only kstrtoull() and
>>> functions used by it. Other functions in lib/kstrtox.c will
>>> be covered.
>>>
>>> Signed-off-by: Chao Fan
>>> ---
>>> arch/x86/boot/compressed/acpitb.c | 26 ++
>>> arch/x86/boot/string.h| 4
>>> lib/kstrtox.c | 4
>>> 3 files changed, 34 insertions(+)
>>>
>>> diff --git a/arch/x86/boot/compressed/acpitb.c
>>> b/arch/x86/boot/compressed/acpitb.c
>>> index 50fa65cf824d..5cfb4efa5a19 100644
>>> --- a/arch/x86/boot/compressed/acpitb.c
>>> +++ b/arch/x86/boot/compressed/acpitb.c
>>> @@ -8,6 +8,12 @@
>>> #include
>>> #include
>>>
>>> +#define STATIC
>>> +#include
>>> +
>>> +#define BOOT_STRING
>>> +#include "../string.h"
>>> +
>>> /* Search EFI table for RSDP table. */
>>> static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>> {
>>> @@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address
>>> *rsdp_addr)
>>> *rsdp_addr = (acpi_physical_address)address;
>>> }
>>> }
>>> +
>>> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>>> +{
>>> +#ifdef CONFIG_KEXEC
>>
>>Ok, why is that CONFIG_KEXEC dependency needed now too?
>>
>
>CONFIG_KEXEC is only needed in this function.
>
>When searching RSDP, there are three methods in order:
>1. When booting from KEXEC, 'acpi_rsdp' is added to cmdline by KEXEC,
> so it can be parsed and used. CONFIG_KEXEC is needed here.
>2. When booting from EFI, parse EFI table and find RSDP.
>3. When booting from BIOS, search memory for RSDP just like
> acpi_find_root_pointer() in drivers/acpi/acpica/tbxfroot.c did.
>
>So, CONFIG_KEXEC is only needed in 1, exactly in this function
>get_acpi_rsdp() of my PATCH.
>
>Thanks,
>Chao Fan
>
That means, CONFIG_KEXEC is needed by a little part of the whole PATCHSET.
Without CONFIG_KEXEC, RSDP can only be found in other methods.
Thanks,
Chao Fan
>>Ok, let's recap: so far, for your use case you need:
>>
>>CONFIG_MEMORY_HOTREMOVE
>>CONFIG_RANDOMIZE_BASE
>>and now
>>CONFIG_KEXEC
>>
>>So, you can clean up all that ifdeffery by defining a new config item
>>CONFIG_EARLY_PARSE_RSDP or so which depends on all those three items and
>>then you can do
>>
>>vmlinux-objs-$(CONFIG_EARLY_PARSE_RSDP) += $(obj)/acpitb.o
>>
>>and get rid of the most of the ifdeffery.
>>
>>Yes?
>>
>>> + unsigned long long res;
>>> + int len = 0;
>>> + char *val;
>>> +
>>> + val = malloc(19);
>>> + len = cmdline_find_option("acpi_rsdp", val, 19);
>>> +
>>
>>^ Superfluous newline.
>>
>>> + if (len == -1)
>>> + return;
>>
>>That check is not needed since you do > 0 below.
>>
>>> +
>>> + if (len > 0) {
>>> + val[len] = 0;
>>> + *rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, );
>>> + }
>>> +#endif
>>> +}
>>> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
>>> index 3d78e27077f4..0ff3edb888e4 100644
>>> --- a/arch/x86/boot/string.h
>>> +++ b/arch/x86/boot/string.h
>>> @@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp,
>>> char **endp,
>>> unsigned int base);
>>>
>>> #endif /* BOOT_STRING_H */
>>> +
>>> +#ifdef BOOT_STRING
>>> +#include "../../../lib/kstrtox.c"
>>> +#endif
>>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>>> index 1006bf70bf74..3804db9eed56 100644
>>> --- a/lib/kstrtox.c
>>> +++ b/lib/kstrtox.c
>>> @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base,
>>> unsigned long long *res)
>>> }
>>> EXPORT_SYMBOL(kstrtoull);
>>
>>This needs a comment to explain what is that guard used for.
>>
>>> +#ifndef BOOT_STRING
>>> +
>>> /**
>>> * kstrtoll - convert a string to a long long
>>> * @s: The start of the string. The string must be null-terminated, and
>>> may also
>>> @@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user, kstrtou16,
>>> u16);
>>> kstrto_from_user(kstrtos16_from_user, kstrtos16, s16);
>>> kstrto_from_user(kstrtou8_from_user, kstrtou8, u8);
>>> kstrto_from_user(kstrtos8_from_user, kstrtos8, s8);
>>> +
>>> +#endif
>>
>>#endif /* BOOT_STRING */
>>
>>--
>>Regards/Gruss,
>>Boris.
>>
>>Good mailing practices for 400: avoid top-posting and trim the reply.
>>
>>
Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
On Tue, Nov 13, 2018 at 06:51:50PM +0100, Borislav Petkov wrote:
>On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
>> Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
>> KEXEC writes the RSDP pointer to cmdline for EFI booting.
>> So if 'acpi_rsdp' found in cmdline, use it directely.
>>
>> Since function kstrtoull() is needed, include it in
>> arch/x86/boot/string.h. To solve the definition conflict
>> problem, set BOOT_STRING tag to expose only kstrtoull() and
>> functions used by it. Other functions in lib/kstrtox.c will
>> be covered.
>>
>> Signed-off-by: Chao Fan
>> ---
>> arch/x86/boot/compressed/acpitb.c | 26 ++
>> arch/x86/boot/string.h| 4
>> lib/kstrtox.c | 4
>> 3 files changed, 34 insertions(+)
>>
>> diff --git a/arch/x86/boot/compressed/acpitb.c
>> b/arch/x86/boot/compressed/acpitb.c
>> index 50fa65cf824d..5cfb4efa5a19 100644
>> --- a/arch/x86/boot/compressed/acpitb.c
>> +++ b/arch/x86/boot/compressed/acpitb.c
>> @@ -8,6 +8,12 @@
>> #include
>> #include
>>
>> +#define STATIC
>> +#include
>> +
>> +#define BOOT_STRING
>> +#include "../string.h"
>> +
>> /* Search EFI table for RSDP table. */
>> static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>> {
>> @@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address
>> *rsdp_addr)
>> *rsdp_addr = (acpi_physical_address)address;
>> }
>> }
>> +
>> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>> +{
>> +#ifdef CONFIG_KEXEC
>
>Ok, why is that CONFIG_KEXEC dependency needed now too?
>
CONFIG_KEXEC is only needed in this function.
When searching RSDP, there are three methods in order:
1. When booting from KEXEC, 'acpi_rsdp' is added to cmdline by KEXEC,
so it can be parsed and used. CONFIG_KEXEC is needed here.
2. When booting from EFI, parse EFI table and find RSDP.
3. When booting from BIOS, search memory for RSDP just like
acpi_find_root_pointer() in drivers/acpi/acpica/tbxfroot.c did.
So, CONFIG_KEXEC is only needed in 1, exactly in this function
get_acpi_rsdp() of my PATCH.
Thanks,
Chao Fan
>Ok, let's recap: so far, for your use case you need:
>
>CONFIG_MEMORY_HOTREMOVE
>CONFIG_RANDOMIZE_BASE
>and now
>CONFIG_KEXEC
>
>So, you can clean up all that ifdeffery by defining a new config item
>CONFIG_EARLY_PARSE_RSDP or so which depends on all those three items and
>then you can do
>
>vmlinux-objs-$(CONFIG_EARLY_PARSE_RSDP) += $(obj)/acpitb.o
>
>and get rid of the most of the ifdeffery.
>
>Yes?
>
>> +unsigned long long res;
>> +int len = 0;
>> +char *val;
>> +
>> +val = malloc(19);
>> +len = cmdline_find_option("acpi_rsdp", val, 19);
>> +
>
>^ Superfluous newline.
>
>> +if (len == -1)
>> +return;
>
>That check is not needed since you do > 0 below.
>
>> +
>> +if (len > 0) {
>> +val[len] = 0;
>> +*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, );
>> +}
>> +#endif
>> +}
>> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
>> index 3d78e27077f4..0ff3edb888e4 100644
>> --- a/arch/x86/boot/string.h
>> +++ b/arch/x86/boot/string.h
>> @@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp,
>> char **endp,
>>unsigned int base);
>>
>> #endif /* BOOT_STRING_H */
>> +
>> +#ifdef BOOT_STRING
>> +#include "../../../lib/kstrtox.c"
>> +#endif
>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>> index 1006bf70bf74..3804db9eed56 100644
>> --- a/lib/kstrtox.c
>> +++ b/lib/kstrtox.c
>> @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned
>> long long *res)
>> }
>> EXPORT_SYMBOL(kstrtoull);
>
>This needs a comment to explain what is that guard used for.
>
>> +#ifndef BOOT_STRING
>> +
>> /**
>> * kstrtoll - convert a string to a long long
>> * @s: The start of the string. The string must be null-terminated, and may
>> also
>> @@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user,kstrtou16,
>> u16);
>> kstrto_from_user(kstrtos16_from_user, kstrtos16, s16);
>> kstrto_from_user(kstrtou8_from_user,kstrtou8, u8);
>> kstrto_from_user(kstrtos8_from_user,kstrtos8, s8);
>> +
>> +#endif
>
>#endif /* BOOT_STRING */
>
>--
>Regards/Gruss,
>Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>
Re: [PATCH] firmware: efi: add NULL pointer checks in efivars api functions
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,
- >var.VendorGuid,
- 0, 0, NULL);
+ if (!__efivars)
+ return -EINVAL;
+
+ status = __efivars->ops->set_variable(entry->var.VariableName,
+ >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_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
efi_status_t status;
if
Re: [PATCH] firmware: efi: add NULL pointer checks in efivars api functions
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,
> - >var.VendorGuid,
> - 0, 0, NULL);
> + if (!__efivars)
> + return -EINVAL;
> +
> + status = __efivars->ops->set_variable(entry->var.VariableName,
> + >var.VendorGuid,
> + 0, 0, NULL);
>
> return efi_status_to_err(status);
> }
> @@ -607,12 +624,17 @@ int __efivar_entry_delete(struct efivar_entry *entry)
> */
> int
Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
On Tue, Nov 13, 2018 at 03:06:16PM -0500, Masayoshi Mizuma wrote: > I just felt the BOOT_STRING thing in lib/kstrtox.c confuses... > I'm OK for now if it's applied your below comment. Well, actually, upon a second look, I don't think that including a .c file into a header is ok: diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h index 3d78e27077f4..0ff3edb888e4 100644 --- a/arch/x86/boot/string.h +++ b/arch/x86/boot/string.h @@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base); #endif /* BOOT_STRING_H */ + +#ifdef BOOT_STRING +#include "../../../lib/kstrtox.c" +#endif Chao, why isn't this part of arch/x86/boot/compressed/misc.c ? -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
[PATCH] firmware: efi: add NULL pointer checks in efivars api functions
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,
- >var.VendorGuid,
- 0, 0, NULL);
+ if (!__efivars)
+ return -EINVAL;
+
+ status = __efivars->ops->set_variable(entry->var.VariableName,
+ >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_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
efi_status_t status;
if (down_interruptible(_lock))
return -EINTR;
+ if (!__efivars) {
+
Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
On Tue, Nov 13, 2018 at 06:54:13PM +0100, Borislav Petkov wrote: > On Tue, Nov 13, 2018 at 11:11:11AM -0500, Masayoshi Mizuma wrote: > > I think it's not very good idea to use kstrtoull() in > > arch/x86/boot/compressed/* because some tricks are needed to > > use the function, looks like Chao is trying... > > Ok, I had a look at the patch. And frankly, I don't see anything wrong > with the aspect of using kstrtoull() in the compressed stage too and > getting rid of simple_strtoull(). > > So what are your reservations? Thank you for your checking. I just felt the BOOT_STRING thing in lib/kstrtox.c confuses... I'm OK for now if it's applied your below comment. > > @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, > > unsigned long long *res) > > } > > EXPORT_SYMBOL(kstrtoull); > > This needs a comment to explain what is that guard used for. Thanks, Masa
Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
On Tue, Nov 13, 2018 at 11:11:11AM -0500, Masayoshi Mizuma wrote: > I think it's not very good idea to use kstrtoull() in > arch/x86/boot/compressed/* because some tricks are needed to > use the function, looks like Chao is trying... Ok, I had a look at the patch. And frankly, I don't see anything wrong with the aspect of using kstrtoull() in the compressed stage too and getting rid of simple_strtoull(). So what are your reservations? -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
> Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
> KEXEC writes the RSDP pointer to cmdline for EFI booting.
> So if 'acpi_rsdp' found in cmdline, use it directely.
>
> Since function kstrtoull() is needed, include it in
> arch/x86/boot/string.h. To solve the definition conflict
> problem, set BOOT_STRING tag to expose only kstrtoull() and
> functions used by it. Other functions in lib/kstrtox.c will
> be covered.
>
> Signed-off-by: Chao Fan
> ---
> arch/x86/boot/compressed/acpitb.c | 26 ++
> arch/x86/boot/string.h| 4
> lib/kstrtox.c | 4
> 3 files changed, 34 insertions(+)
>
> diff --git a/arch/x86/boot/compressed/acpitb.c
> b/arch/x86/boot/compressed/acpitb.c
> index 50fa65cf824d..5cfb4efa5a19 100644
> --- a/arch/x86/boot/compressed/acpitb.c
> +++ b/arch/x86/boot/compressed/acpitb.c
> @@ -8,6 +8,12 @@
> #include
> #include
>
> +#define STATIC
> +#include
> +
> +#define BOOT_STRING
> +#include "../string.h"
> +
> /* Search EFI table for RSDP table. */
> static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
> {
> @@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address
> *rsdp_addr)
> *rsdp_addr = (acpi_physical_address)address;
> }
> }
> +
> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
> +{
> +#ifdef CONFIG_KEXEC
Ok, why is that CONFIG_KEXEC dependency needed now too?
Ok, let's recap: so far, for your use case you need:
CONFIG_MEMORY_HOTREMOVE
CONFIG_RANDOMIZE_BASE
and now
CONFIG_KEXEC
So, you can clean up all that ifdeffery by defining a new config item
CONFIG_EARLY_PARSE_RSDP or so which depends on all those three items and
then you can do
vmlinux-objs-$(CONFIG_EARLY_PARSE_RSDP) += $(obj)/acpitb.o
and get rid of the most of the ifdeffery.
Yes?
> + unsigned long long res;
> + int len = 0;
> + char *val;
> +
> + val = malloc(19);
> + len = cmdline_find_option("acpi_rsdp", val, 19);
> +
^ Superfluous newline.
> + if (len == -1)
> + return;
That check is not needed since you do > 0 below.
> +
> + if (len > 0) {
> + val[len] = 0;
> + *rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, );
> + }
> +#endif
> +}
> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
> index 3d78e27077f4..0ff3edb888e4 100644
> --- a/arch/x86/boot/string.h
> +++ b/arch/x86/boot/string.h
> @@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp,
> char **endp,
> unsigned int base);
>
> #endif /* BOOT_STRING_H */
> +
> +#ifdef BOOT_STRING
> +#include "../../../lib/kstrtox.c"
> +#endif
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index 1006bf70bf74..3804db9eed56 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned
> long long *res)
> }
> EXPORT_SYMBOL(kstrtoull);
This needs a comment to explain what is that guard used for.
> +#ifndef BOOT_STRING
> +
> /**
> * kstrtoll - convert a string to a long long
> * @s: The start of the string. The string must be null-terminated, and may
> also
> @@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user, kstrtou16,
> u16);
> kstrto_from_user(kstrtos16_from_user,kstrtos16, s16);
> kstrto_from_user(kstrtou8_from_user, kstrtou8, u8);
> kstrto_from_user(kstrtos8_from_user, kstrtos8, s8);
> +
> +#endif
#endif /* BOOT_STRING */
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
On Tue, Nov 13, 2018 at 11:11:11AM -0500, Masayoshi Mizuma wrote: > I know checkpatch.pl says an warning about simple_strtoull(), > however, I believe the warning is for simple_strtoull() defined > in lib/vsprintf.c. simple_strtoull is deprecated for various reasons. I'll take a look at Chao's patch soon. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
Hi Chao and Boris,
On Tue, Nov 13, 2018 at 10:12:18AM +0800, Chao Fan wrote:
> On Mon, Nov 12, 2018 at 12:43:44PM -0500, Masayoshi Mizuma wrote:
> >How about the following get_acpi_rsdp()...? It doesn't use kstrtoull().
> >
> >static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
> >{
> >#ifdef CONFIG_KEXEC
> >unsigned long addr;
> >char val[32];
> >
> >if (cmdline_find_option("acpi_rsdp", val, sizeof(val)) > 0) {
> >char *e;
> >
> >if (!strncmp(val, "0x", 2)) {
> >addr = simple_strtoull(val + 2, , 16);
> >if ((addr == 0) || ((val + 2) == e))
> >return;
> >*rsdp_addr = (acpi_physical_address)addr;
> >}
> >}
> >#endif
> >}
>
> Thanks for the suggestion.
> I used this function. In the old version, Boris said simple_strtoull()
> is the old function and told me use the new kstrtoull().
I think it's not very good idea to use kstrtoull() in
arch/x86/boot/compressed/* because some tricks are needed to
use the function, looks like Chao is trying...
It is the simple way here to use simple_strtoull() defined
in arch/x86/boot/boot.h, I think.
I know checkpatch.pl says an warning about simple_strtoull(),
however, I believe the warning is for simple_strtoull() defined
in lib/vsprintf.c.
Thanks,
Masa
Re: [PATCH v11 2/5] x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory
On Tue, Nov 13, 2018 at 10:10:16AM +0800, Chao Fan wrote: > The 'rover' was named as 'mem_rover', but the length of this line is too > long. So shorten it as 'rever' so that they can keep in one line. I still have no clue what "rover" or "rever" means... > This name came from ACPI driver code, acpi_find_root_pointer(). > Used for the loop. If you have a better name, please tell me. I would if I knew what it meant. If you don't know either, then name it to something descriptive so that it is clear what it is when reading the code. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
