[PATCH] mtd: physmap_of: fix potential NULL dereference

[PATCH] mtd: physmap_of: fix potential NULL dereference

[Ard Biesheuvel]

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(-)

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

Re: [PATCH] mtd: physmap_of: fix potential NULL dereference

[Ard Biesheuvel]

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
>

Re: [PATCH] mtd: physmap_of: fix potential NULL dereference

[Brian Norris]

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.

> 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

Re: [PATCH] mtd: physmap_of: fix potential NULL dereference

[Ard Biesheuvel]

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

Re: [PATCH] mtd: physmap_of: fix potential NULL dereference

[Brian Norris]

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().
> 
> Instead, test for NULL first, and only then perform the test
> for a concatenated device.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Applied to l2-mtd.git. Thanks!

Brian