[PATCH] mtd: physmap_of: fix potential NULL dereference

[Ard Biesheuvel]

On 13 December 2014 at 04:11, Brian Norris <computersforpeace at 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 at 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