| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YgabtFd4Nkpgi+oW@zn.tnic>
Date: Fri, 11 Feb 2022 18:24:04 +0100
From: Borislav Petkov <bp@...en8.de>
To: Ross Philipson <ross.philipson@...cle.com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org,
daniel.kiper@...cle.com, dpsmith@...rtussolutions.com,
tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
luto@...capital.net, kanth.ghatraju@...cle.com,
trenchboot-devel@...glegroups.com
Subject: Re: [PATCH 1/2] x86/boot: Fix memremap of setup_indirect structures
On Thu, Jan 27, 2022 at 12:04:15PM -0500, Ross Philipson wrote:
> As documented, the setup_indirect structure is nested inside
> the setup_data structures in the setup_data list. The code currently
> accesses the fields inside the setup_indirect structure but only
> the sizeof(struct setup_data) is being memremapped. No crash
> occurred but this is just due to how the area is remapped under the
> covers.
>
> The fix is to properly memremap both the setup_data and setup_indirect
s/The fix is to properly/Properly/
> structures in these cases before accessing them.
>
> Fixes: b3c72fc9a78e ("x86/boot: Introduce setup_indirect")
>
No need for that space - Fixes belongs with the rest of the tags.
> Signed-off-by: Ross Philipson <ross.philipson@...cle.com>
> Reviewed-by: Daniel Kiper <daniel.kiper@...cle.com>
> @@ -1015,18 +1019,23 @@ void __init e820__reserve_setup_data(void)
> sizeof(*data) + data->len,
> E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
>
> - if (data->type == SETUP_INDIRECT &&
> - ((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
> - e820__range_update(((struct setup_indirect *)data->data)->addr,
> - ((struct setup_indirect *)data->data)->len,
> - E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> - e820__range_update_kexec(((struct setup_indirect *)data->data)->addr,
> - ((struct setup_indirect *)data->data)->len,
> - E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> + if (data->type == SETUP_INDIRECT) {
> + len += data->len;
> + early_memunmap(data, sizeof(*data));
> + data = early_memremap(pa_data, len);
Do I see it correctly that early_memremap() can return NULL?
> + if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
> + e820__range_update(((struct setup_indirect *)data->data)->addr,
> + ((struct setup_indirect *)data->data)->len,
> + E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> + e820__range_update_kexec(((struct setup_indirect *)data->data)->addr,
> + ((struct setup_indirect *)data->data)->len,
> + E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> + }
> }
>
> - pa_data = data->next;
> - early_memunmap(data, sizeof(*data));
> + pa_data = pa_next;
> + early_memunmap(data, len);
> }
>
> e820__update_table(e820_table);
> diff --git a/arch/x86/kernel/kdebugfs.c b/arch/x86/kernel/kdebugfs.c
> index 64b6da9..e5c72d8 100644
> --- a/arch/x86/kernel/kdebugfs.c
> +++ b/arch/x86/kernel/kdebugfs.c
> @@ -92,7 +92,8 @@ static int __init create_setup_data_nodes(struct dentry *parent)
> struct setup_data *data;
> int error;
> struct dentry *d;
> - u64 pa_data;
> + u64 pa_data, pa_next;
> + u32 len;
> int no = 0;
The tip-tree preferred ordering of variable declarations at the
beginning of a function is reverse fir tree order::
struct long_struct_name *descriptive_name;
unsigned long foo, bar;
unsigned int tmp;
int ret;
The above is faster to parse than the reverse ordering::
int ret;
unsigned int tmp;
unsigned long foo, bar;
struct long_struct_name *descriptive_name;
And even more so than random ordering::
unsigned long foo, bar;
int ret;
struct long_struct_name *descriptive_name;
unsigned int tmp;
Please fix all cases in your patch.
> d = debugfs_create_dir("setup_data", parent);
> @@ -112,12 +113,27 @@ static int __init create_setup_data_nodes(struct dentry *parent)
> error = -ENOMEM;
> goto err_dir;
> }
> -
> - if (data->type == SETUP_INDIRECT &&
> - ((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
> - node->paddr = ((struct setup_indirect *)data->data)->addr;
> - node->type = ((struct setup_indirect *)data->data)->type;
> - node->len = ((struct setup_indirect *)data->data)->len;
> + pa_next = data->next;
> +
> + if (data->type == SETUP_INDIRECT) {
> + len = sizeof(*data) + data->len;
> + memunmap(data);
> + data = memremap(pa_data, len, MEMREMAP_WB);
> + if (!data) {
Yap, you need similar error handling above.
> + kfree(node);
> + error = -ENOMEM;
> + goto err_dir;
> + }
> +
> + if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
> + node->paddr = ((struct setup_indirect *)data->data)->addr;
> + node->type = ((struct setup_indirect *)data->data)->type;
> + node->len = ((struct setup_indirect *)data->data)->len;
Pls use a helper variable here to not have this ugly casting on each line.
> + } else {
> + node->paddr = pa_data;
> + node->type = data->type;
> + node->len = data->len;
> + }
> } else {
> node->paddr = pa_data;
> node->type = data->type;
> @@ -125,7 +141,7 @@ static int __init create_setup_data_nodes(struct dentry *parent)
> }
>
> create_setup_data_node(d, no, node);
> - pa_data = data->next;
> + pa_data = pa_next;
>
> memunmap(data);
> no++;
> diff --git a/arch/x86/kernel/ksysfs.c b/arch/x86/kernel/ksysfs.c
> index d0a1912..4e8b794 100644
> --- a/arch/x86/kernel/ksysfs.c
> +++ b/arch/x86/kernel/ksysfs.c
> @@ -93,24 +93,35 @@ static int __init get_setup_data_size(int nr, size_t *size)
> {
> int i = 0;
> struct setup_data *data;
> - u64 pa_data = boot_params.hdr.setup_data;
> + u64 pa_data = boot_params.hdr.setup_data, pa_next;
> + u32 len;
>
> while (pa_data) {
> data = memremap(pa_data, sizeof(*data), MEMREMAP_WB);
> if (!data)
> return -ENOMEM;
> + pa_next = data->next;
> +
> if (nr == i) {
> - if (data->type == SETUP_INDIRECT &&
> - ((struct setup_indirect *)data->data)->type != SETUP_INDIRECT)
> - *size = ((struct setup_indirect *)data->data)->len;
> - else
> + if (data->type == SETUP_INDIRECT) {
> + len = sizeof(*data) + data->len;
> + memunmap(data);
> + data = memremap(pa_data, len, MEMREMAP_WB);
> + if (!data)
> + return -ENOMEM;
> +
> + if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT)
> + *size = ((struct setup_indirect *)data->data)->len;
Ditto.
> + else
> + *size = data->len;
> + } else
> *size = data->len;
Put the else branch in {} too pls, even if it is a single statement.
Below too.
>
> memunmap(data);
> return 0;
> }
>
> - pa_data = data->next;
> + pa_data = pa_next;
> memunmap(data);
> i++;
> }
> @@ -122,6 +133,7 @@ static ssize_t type_show(struct kobject *kobj,
> {
> int nr, ret;
> u64 paddr;
> + u32 len;
> struct setup_data *data;
>
> ret = kobj_to_setup_data_nr(kobj, &nr);
> @@ -135,9 +147,14 @@ static ssize_t type_show(struct kobject *kobj,
> if (!data)
> return -ENOMEM;
>
> - if (data->type == SETUP_INDIRECT)
> + if (data->type == SETUP_INDIRECT) {
> + len = sizeof(*data) + data->len;
> + memunmap(data);
> + data = memremap(paddr, len, MEMREMAP_WB);
> + if (!data)
> + return -ENOMEM;
<---- newline here.
> ret = sprintf(buf, "0x%x\n", ((struct setup_indirect *)data->data)->type);
> - else
> + } else
> ret = sprintf(buf, "0x%x\n", data->type);
> memunmap(data);
> return ret;
> @@ -165,10 +182,25 @@ static ssize_t setup_data_data_read(struct file *fp,
> if (!data)
> return -ENOMEM;
>
> - if (data->type == SETUP_INDIRECT &&
> - ((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
> - paddr = ((struct setup_indirect *)data->data)->addr;
> - len = ((struct setup_indirect *)data->data)->len;
> + if (data->type == SETUP_INDIRECT) {
> + len = sizeof(*data) + data->len;
> + memunmap(data);
> + data = memremap(paddr, len, MEMREMAP_WB);
> + if (!data)
> + return -ENOMEM;
> +
> + if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
> + paddr = ((struct setup_indirect *)data->data)->addr;
> + len = ((struct setup_indirect *)data->data)->len;
Again a helper var pls.
> + } else {
> + /*
> + * Even though this is technically undefined, return
> + * the data as though it is a normal setup_data struct.
> + * This will at least allow it to be inspected.
> + */
> + paddr += sizeof(*data);
> + len = data->len;
> + }
> } else {
> paddr += sizeof(*data);
> len = data->len;
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index f7a132e..6e29c20 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -370,20 +370,29 @@ static void __init parse_setup_data(void)
> static void __init memblock_x86_reserve_range_setup_data(void)
> {
> struct setup_data *data;
> - u64 pa_data;
> + u64 pa_data, pa_next;
> + u32 len;
>
> pa_data = boot_params.hdr.setup_data;
> while (pa_data) {
> data = early_memremap(pa_data, sizeof(*data));
> + len = sizeof(*data);
> + pa_next = data->next;
> +
> memblock_reserve(pa_data, sizeof(*data) + data->len);
>
> - if (data->type == SETUP_INDIRECT &&
> - ((struct setup_indirect *)data->data)->type != SETUP_INDIRECT)
> - memblock_reserve(((struct setup_indirect *)data->data)->addr,
> - ((struct setup_indirect *)data->data)->len);
> + if (data->type == SETUP_INDIRECT) {
> + len += data->len;
> + early_memunmap(data, sizeof(*data));
> + data = early_memremap(pa_data, len);
>
> - pa_data = data->next;
> - early_memunmap(data, sizeof(*data));
> + if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT)
> + memblock_reserve(((struct setup_indirect *)data->data)->addr,
> + ((struct setup_indirect *)data->data)->len);
Ditto.
> + }
> +
> + pa_data = pa_next;
> + early_memunmap(data, len);
> }
> }
>
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 026031b..b45e86e 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -636,10 +636,15 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
> return true;
> }
>
> - if (data->type == SETUP_INDIRECT &&
> - ((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
> - paddr = ((struct setup_indirect *)data->data)->addr;
> - len = ((struct setup_indirect *)data->data)->len;
> + if (data->type == SETUP_INDIRECT) {
> + memunmap(data);
> + data = memremap(paddr, sizeof(*data) + len,
> + MEMREMAP_WB | MEMREMAP_DEC);
> +
> + if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
> + paddr = ((struct setup_indirect *)data->data)->addr;
> + len = ((struct setup_indirect *)data->data)->len;
Ditto.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Powered by blists - more mailing lists