| Message ID | 20211119222412.1092763-4-lizhi.hou@xilinx.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | XRT Alveo driver infrastructure overview | expand |
On Fri, Nov 19, 2021 at 02:24:06PM -0800, Lizhi Hou wrote: > Add alignment check to of_fdt_unflatten_tree(). If it is not aligned, > allocate a aligned buffer and copy the fdt blob. So the caller does not > have to deal with the buffer alignment before calling this function. > XRT uses this function to unflatten fdt which is from Alveo firmware. > > Signed-off-by: Sonal Santan <sonal.santan@xilinx.com> > Signed-off-by: Max Zhen <max.zhen@xilinx.com> > Signed-off-by: Lizhi Hou <lizhi.hou@xilinx.com> > --- > drivers/of/fdt.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > index 4546572af24b..d64445e43ceb 100644 > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -455,13 +455,28 @@ void *of_fdt_unflatten_tree(const unsigned long *blob, > struct device_node *dad, > struct device_node **mynodes) > { > + void *new_fdt = NULL, *fdt_align; > void *mem; > > + if (fdt_check_header(blob)) { > + pr_err("Invalid fdt blob\n"); > + return NULL; > + } > + fdt_align = (void *)PTR_ALIGN(blob, FDT_ALIGN_SIZE); > + if (fdt_align != blob) { > + new_fdt = kmalloc(fdt_totalsize(blob) + FDT_ALIGN_SIZE, GFP_KERNEL); > + if (!new_fdt) > + return NULL; > + fdt_align = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE); Where's the copy? > + } > + > mutex_lock(&of_fdt_unflatten_mutex); > - mem = __unflatten_device_tree(blob, dad, mynodes, &kernel_tree_alloc, > + mem = __unflatten_device_tree(fdt_align, dad, mynodes, &kernel_tree_alloc, > true); > mutex_unlock(&of_fdt_unflatten_mutex); > > + kfree(new_fdt); You know the unflattened DT just references strings and property values from the flattened DT. So you just caused a use after free. Fix your firmware to align the DT. Rob
On 11/29/21 5:57 PM, Rob Herring wrote: > On Fri, Nov 19, 2021 at 02:24:06PM -0800, Lizhi Hou wrote: >> Add alignment check to of_fdt_unflatten_tree(). If it is not aligned, >> allocate a aligned buffer and copy the fdt blob. So the caller does not >> have to deal with the buffer alignment before calling this function. >> XRT uses this function to unflatten fdt which is from Alveo firmware. >> >> Signed-off-by: Sonal Santan <sonal.santan@xilinx.com> >> Signed-off-by: Max Zhen <max.zhen@xilinx.com> >> Signed-off-by: Lizhi Hou <lizhi.hou@xilinx.com> >> --- >> drivers/of/fdt.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c >> index 4546572af24b..d64445e43ceb 100644 >> --- a/drivers/of/fdt.c >> +++ b/drivers/of/fdt.c >> @@ -455,13 +455,28 @@ void *of_fdt_unflatten_tree(const unsigned long *blob, >> struct device_node *dad, >> struct device_node **mynodes) >> { >> + void *new_fdt = NULL, *fdt_align; >> void *mem; >> >> + if (fdt_check_header(blob)) { >> + pr_err("Invalid fdt blob\n"); >> + return NULL; >> + } >> + fdt_align = (void *)PTR_ALIGN(blob, FDT_ALIGN_SIZE); >> + if (fdt_align != blob) { >> + new_fdt = kmalloc(fdt_totalsize(blob) + FDT_ALIGN_SIZE, GFP_KERNEL); >> + if (!new_fdt) >> + return NULL; >> + fdt_align = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE); > Where's the copy? > >> + } >> + >> mutex_lock(&of_fdt_unflatten_mutex); >> - mem = __unflatten_device_tree(blob, dad, mynodes, &kernel_tree_alloc, >> + mem = __unflatten_device_tree(fdt_align, dad, mynodes, &kernel_tree_alloc, >> true); >> mutex_unlock(&of_fdt_unflatten_mutex); >> >> + kfree(new_fdt); > You know the unflattened DT just references strings and property values > from the flattened DT. So you just caused a use after free. > > Fix your firmware to align the DT. Thanks a lot for pointing this out. I did not aware the direct reference of strings and values from the flattened DT. And I will fix this. Lizhi > > Rob
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index 4546572af24b..d64445e43ceb 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -455,13 +455,28 @@ void *of_fdt_unflatten_tree(const unsigned long *blob, struct device_node *dad, struct device_node **mynodes) { + void *new_fdt = NULL, *fdt_align; void *mem; + if (fdt_check_header(blob)) { + pr_err("Invalid fdt blob\n"); + return NULL; + } + fdt_align = (void *)PTR_ALIGN(blob, FDT_ALIGN_SIZE); + if (fdt_align != blob) { + new_fdt = kmalloc(fdt_totalsize(blob) + FDT_ALIGN_SIZE, GFP_KERNEL); + if (!new_fdt) + return NULL; + fdt_align = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE); + } + mutex_lock(&of_fdt_unflatten_mutex); - mem = __unflatten_device_tree(blob, dad, mynodes, &kernel_tree_alloc, + mem = __unflatten_device_tree(fdt_align, dad, mynodes, &kernel_tree_alloc, true); mutex_unlock(&of_fdt_unflatten_mutex); + kfree(new_fdt); + return mem; } EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);