Re: [PATCH 1/2] x86/boot: Fix memremap of setup_indirect structures

From: Borislav Petkov
Date: Fri Feb 11 2022 - 12:24:15 EST

Next message: Sebastian Andrzej Siewior: "Re: [PATCH v7] random: defer fast pool mixing to worker"
Previous message: Arnaldo Carvalho de Melo: "Re: [PATCH v3 05/22] perf maps: Use a pointer for kmaps"
Next in thread: Ross Philipson: "Re: [PATCH 1/2] x86/boot: Fix memremap of setup_indirect structures"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
> Reviewed-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>

> @@ -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

Next message: Sebastian Andrzej Siewior: "Re: [PATCH v7] random: defer fast pool mixing to worker"
Previous message: Arnaldo Carvalho de Melo: "Re: [PATCH v3 05/22] perf maps: Use a pointer for kmaps"
Next in thread: Ross Philipson: "Re: [PATCH 1/2] x86/boot: Fix memremap of setup_indirect structures"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]