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

From: Ross Philipson
Date: Tue Feb 15 2022 - 06:35:19 EST

On 2/11/22 12:24, Borislav Petkov wrote:
> 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/

Ack.

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

Got it. Will fix in both.

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

It can if you run out of slots in the fixed map. The only reason I did
not check it for NULL was because it was not checked elsewhere for NULL.
I guess there are two questions:

1. Should I also fix it elsewhere in the code I am touching?
2. What should I do on an allocation failure? In a routine like this it
seems to be a critical early boot failure.

I guess the original intention might have been to let it just blow up
since there is no recovery but that is just conjecture...

>
>> + 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.

Will do.

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

Will fix here and below.

>
>> + } 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.

Ok.

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

Thank you very much for the review.

Ross Philipson