Messages in this thread | | | Date | Tue, 15 Feb 2022 06:34:43 -0500 | Subject | Re: [PATCH 1/2] x86/boot: Fix memremap of setup_indirect structures | From | Ross Philipson <> |
| |
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@oracle.com> >> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.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?
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
| |