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@xxxxxxxxxx>
> ---
> 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);
>   		}
>   	}
>   }