| Message ID | 1417351863-3812-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 92b633a8a482c4bc1ff3b7cffdcace7836861554 |
| Headers | show |
On 30 November 2014 at 13:51, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On device remove, when testing the cmtd field of an of_flash > struct to decide whether it is a concatenated device or not, > we get a false positive on cmtd == NULL, and dereference it > subsequently. This may occur if of_flash_remove() is called > from the cleanup path of of_flash_probe(). > > Instead, test for NULL first, and only then perform the test > for a concatenated device. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > drivers/mtd/maps/physmap_of.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > Ping? > diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c > index c1d21cb501ca..e48930424091 100644 > --- a/drivers/mtd/maps/physmap_of.c > +++ b/drivers/mtd/maps/physmap_of.c > @@ -47,14 +47,12 @@ static int of_flash_remove(struct platform_device *dev) > return 0; > dev_set_drvdata(&dev->dev, NULL); > > - if (info->cmtd != info->list[0].mtd) { > + if (info->cmtd) { > mtd_device_unregister(info->cmtd); > - mtd_concat_destroy(info->cmtd); > + if (info->cmtd != info->list[0].mtd) > + mtd_concat_destroy(info->cmtd); > } > > - if (info->cmtd) > - mtd_device_unregister(info->cmtd); > - > for (i = 0; i < info->list_size; i++) { > if (info->list[i].mtd) > map_destroy(info->list[i].mtd); > -- > 1.8.3.2 >
On 13 December 2014 at 04:11, Brian Norris <computersforpeace@gmail.com> wrote: > On Sun, Nov 30, 2014 at 01:51:03PM +0100, Ard Biesheuvel wrote: >> On device remove, when testing the cmtd field of an of_flash >> struct to decide whether it is a concatenated device or not, >> we get a false positive on cmtd == NULL, and dereference it >> subsequently. This may occur if of_flash_remove() is called >> from the cleanup path of of_flash_probe(). > > Did you catch this on real hardware, or just by inspection? Just > wondering if this should be marked for -stable. > I am not aware of this issue occurring in the field, but I caught it while working on the EFI support for arm64 in the kernel. One of the changes I am making is ensuring all iomem ranges owned by the firmware are properly marked as busy in the iomem resource map. Once the NOR flash holding the EFI variable store is reserved in the iomem resource map, this issue will be triggered because the probe fails and has to clean up after itself. Those changes are aimed for 3.20, so it would be nice if this fix could have made it in by then. Cheers, Ard. >> Instead, test for NULL first, and only then perform the test >> for a concatenated device. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> drivers/mtd/maps/physmap_of.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c >> index c1d21cb501ca..e48930424091 100644 >> --- a/drivers/mtd/maps/physmap_of.c >> +++ b/drivers/mtd/maps/physmap_of.c >> @@ -47,14 +47,12 @@ static int of_flash_remove(struct platform_device *dev) >> return 0; >> dev_set_drvdata(&dev->dev, NULL); >> >> - if (info->cmtd != info->list[0].mtd) { >> + if (info->cmtd) { >> mtd_device_unregister(info->cmtd); >> - mtd_concat_destroy(info->cmtd); >> + if (info->cmtd != info->list[0].mtd) >> + mtd_concat_destroy(info->cmtd); >> } >> >> - if (info->cmtd) >> - mtd_device_unregister(info->cmtd); >> - >> for (i = 0; i < info->list_size; i++) { >> if (info->list[i].mtd) >> map_destroy(info->list[i].mtd); > > Brian
diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c index c1d21cb501ca..e48930424091 100644 --- a/drivers/mtd/maps/physmap_of.c +++ b/drivers/mtd/maps/physmap_of.c @@ -47,14 +47,12 @@ static int of_flash_remove(struct platform_device *dev) return 0; dev_set_drvdata(&dev->dev, NULL); - if (info->cmtd != info->list[0].mtd) { + if (info->cmtd) { mtd_device_unregister(info->cmtd); - mtd_concat_destroy(info->cmtd); + if (info->cmtd != info->list[0].mtd) + mtd_concat_destroy(info->cmtd); } - if (info->cmtd) - mtd_device_unregister(info->cmtd); - for (i = 0; i < info->list_size; i++) { if (info->list[i].mtd) map_destroy(info->list[i].mtd);
On device remove, when testing the cmtd field of an of_flash struct to decide whether it is a concatenated device or not, we get a false positive on cmtd == NULL, and dereference it subsequently. This may occur if of_flash_remove() is called from the cleanup path of of_flash_probe(). Instead, test for NULL first, and only then perform the test for a concatenated device. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- drivers/mtd/maps/physmap_of.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)