[PATCH] mtd: virt_concat: fix use-after-free in mtd_virt_concat_destroy_joins()

[PATCH] mtd: virt_concat: fix use-after-free in mtd_virt_concat_destroy_joins()

[Harshit Mogalapalli]

mtd_concat_destroy() frees item->concat so calling
mtd_virt_concat_put_mtd_devices(item->concat) leads to a use after free.

Fix this by moving mtd_virt_concat_put_mtd_devices() before
mtd_concat_destroy()

Fixes: 43db6366fc2d ("mtd: Add driver for concatenating devices")
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
This is static analysis finding by Smatch, only compile tested.
---
 drivers/mtd/mtd_virt_concat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/mtd_virt_concat.c b/drivers/mtd/mtd_virt_concat.c
index 37075ead0f33..a3fb96788e9d 100644
--- a/drivers/mtd/mtd_virt_concat.c
+++ b/drivers/mtd/mtd_virt_concat.c
@@ -75,8 +75,8 @@ void mtd_virt_concat_destroy_joins(void)
 		if (item->concat) {
 			mtd_device_unregister(mtd);
 			kfree(mtd->name);
-			mtd_concat_destroy(mtd);
 			mtd_virt_concat_put_mtd_devices(item->concat);
+			mtd_concat_destroy(mtd);
 		}
 	}
 }
-- 
2.50.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: virt_concat: fix use-after-free in mtd_virt_concat_destroy_joins()

[Luca Ceresoli]

Hello Harshit, Miquel/Richard/Vignesh,

On Sun Jun 14, 2026 at 10:10 AM CEST, Harshit Mogalapalli wrote:
> mtd_concat_destroy() frees item->concat so calling
> mtd_virt_concat_put_mtd_devices(item->concat) leads to a use after free.
>
> Fix this by moving mtd_virt_concat_put_mtd_devices() before
> mtd_concat_destroy()
>
> Fixes: 43db6366fc2d ("mtd: Add driver for concatenating devices")
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> ---
> This is static analysis finding by Smatch, only compile tested.
> ---
>  drivers/mtd/mtd_virt_concat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/mtd_virt_concat.c b/drivers/mtd/mtd_virt_concat.c
> index 37075ead0f33..a3fb96788e9d 100644
> --- a/drivers/mtd/mtd_virt_concat.c
> +++ b/drivers/mtd/mtd_virt_concat.c
> @@ -75,8 +75,8 @@ void mtd_virt_concat_destroy_joins(void)
>  		if (item->concat) {
>  			mtd_device_unregister(mtd);
>  			kfree(mtd->name);
> -			mtd_concat_destroy(mtd);
>  			mtd_virt_concat_put_mtd_devices(item->concat);
> +			mtd_concat_destroy(mtd);
>  		}
>  	}
>  }

This patch looks OK:

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

@Miquel/Richard/Vignesh:
However while looking at the code to understand it I noticed two possible
issues in the existing code.


Issue 1: the CONCAT() macro implementation looks hacky:

  /*
   * Given a pointer to the MTD object in the mtd_concat structure,
   * we can retrieve the pointer to that structure with this macro.
   */
  #define CONCAT(x)  ((struct mtd_concat *)(x))

Shouldn't it be implemented as a container_of() instead? The current
implementation works just "by chance", i.e. because the struct mtd_info is
the first field in struct mtd_concat.


Issue 2: in mtd_virt_concat_destroy_joins():

	list_for_each_entry_safe(item, tmp, &concat_node_list, head) {
		mtd = &item->concat->mtd; [0]
		if (item->concat) { [1]

At line [0] we dereference item->concat, but at line [1] we apparently
handle the case where item->concat can be NULL. Either [1] is always true
and we can remove the if(), or [1] can be false, so [0] is a bug and should
probably be moved to inside the if().


Do these look like correct findings?


Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: virt_concat: fix use-after-free in mtd_virt_concat_destroy_joins()

[Dan Carpenter]

On Tue, Jun 16, 2026 at 11:51:53AM +0200, Luca Ceresoli wrote:
> Issue 2: in mtd_virt_concat_destroy_joins():
> 
> 	list_for_each_entry_safe(item, tmp, &concat_node_list, head) {
> 		mtd = &item->concat->mtd; [0]
> 		if (item->concat) { [1]
> 
> At line [0] we dereference item->concat, but at line [1] we apparently
> handle the case where item->concat can be NULL. Either [1] is always true
> and we can remove the if(), or [1] can be false, so [0] is a bug and should
> probably be moved to inside the if().

That's not a dereference on line 0, it's pointer math.  So the code
works.  But a lot of people find the distinction confusing.

regards,
dan carpenter


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: virt_concat: fix use-after-free in mtd_virt_concat_destroy_joins()

[Harshit Mogalapalli]

Hi all,

Luca: thanks for the review.

On 14/06/26 13:40, Harshit Mogalapalli wrote:
> mtd_concat_destroy() frees item->concat so calling
> mtd_virt_concat_put_mtd_devices(item->concat) leads to a use after free.
> 
> Fix this by moving mtd_virt_concat_put_mtd_devices() before
> mtd_concat_destroy()
> 
> Fixes: 43db6366fc2d ("mtd: Add driver for concatenating devices")
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> ---
> This is static analysis finding by Smatch, only compile tested.
> ---
>   drivers/mtd/mtd_virt_concat.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/mtd_virt_concat.c b/drivers/mtd/mtd_virt_concat.c
> index 37075ead0f33..a3fb96788e9d 100644
> --- a/drivers/mtd/mtd_virt_concat.c
> +++ b/drivers/mtd/mtd_virt_concat.c
> @@ -75,8 +75,8 @@ void mtd_virt_concat_destroy_joins(void)
>   		if (item->concat) {
>   			mtd_device_unregister(mtd);
>   			kfree(mtd->name);

This diff in this patch is correct, but smatch does report any UAF in 
above few lines which really can't be fixed easily and needs more 
careful review.

drivers/mtd/mtd_virt_concat.c:77 mtd_virt_concat_destroy_joins() error: 
dereferencing freed memory 'mtd' (line 76)

So mtd_device_unregister(mtd) also frees mtd, which I think is incorrect.

-> mtd_device_unregister
--> mtd_virt_concat_destroy
---> this have:

out:
         mutex_lock(&master->master.partitions_lock);
         list_del(&child->part.node);
         mutex_unlock(&master->master.partitions_lock);
         kfree(mtd->name);
         kfree(mtd);

mtd_device_unregister(mtd) calls mtd_virt_concat_destroy(mtd), whose 
error path does kfree(mtd->name); kfree(mtd);, so the later dereference 
in mtd_virt_concat_destroy_joins() is a use after free.


Thoughts ?


Thanks,
harshit


> -			mtd_concat_destroy(mtd);
>   			mtd_virt_concat_put_mtd_devices(item->concat);
> +			mtd_concat_destroy(mtd);
>   		}
>   	}
>   }


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/