linux-kernel - Re: [PATCH] media: rkvdec: Fix an error handling path in rkvdec

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <884293c1-f6f4-48b3-a5d9-9b41fa8614a5@wanadoo.fr>
Date: Tue, 29 Jul 2025 21:33:32 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Nicolas Dufresne <nicolas.dufresne@...labora.com>,
 Detlev Casanova <detlev.casanova@...labora.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Heiko Stuebner
 <heiko@...ech.de>, Hans Verkuil <hverkuil@...nel.org>
Cc: linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
 linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] media: rkvdec: Fix an error handling path in
 rkvdec_probe()

Le 29/07/2025 à 00:50, Nicolas Dufresne a écrit :
> Hi,
> 
> Le dimanche 27 juillet 2025 à 12:02 +0200, Christophe JAILLET a écrit :
>> If an error occurs after a successful iommu_paging_domain_alloc() call, it
>> should be undone by a corresponding iommu_domain_free() call, as already
>> done in the remove function.
>>
>> Fixes: ff8c5622f9f7 ("media: rkvdec: Restore iommu addresses on errors")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>> ---
>> Compile tested only
>> ---
>>   drivers/media/platform/rockchip/rkvdec/rkvdec.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
>> b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
>> index d707088ec0dc..eb0d41f85d89 100644
>> --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
>> +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
>> @@ -1169,15 +1169,17 @@ static int rkvdec_probe(struct platform_device *pdev)
>>   	vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
>>   
>>   	irq = platform_get_irq(pdev, 0);
>> -	if (irq <= 0)
>> -		return -ENXIO;
>> +	if (irq <= 0) {
>> +		ret = -ENXIO;
>> +		goto err_free_domain;
>> +	}
>>   
>>   	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
>>   					rkvdec_irq_handler, IRQF_ONESHOT,
>>   					dev_name(&pdev->dev), rkvdec);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "Could not request vdec IRQ\n");
>> -		return ret;
>> +		goto err_free_domain;
>>   	}
>>   
>>   	pm_runtime_set_autosuspend_delay(&pdev->dev, 100);
> 
> Have you considered moving the allocation of the domain right above the above
> line instead ? The empty domain can't possibly be used unless the probe have
> fully completed.

That would not change things much. We still need to handle 
rkvdec_v4l2_init() failure a few lines below.

If it is correct to move it at the very end of the function, after 
rkvdec_v4l2_init(), then the patch would be simpler.


Honestly, I'm not very confident with it. request_threaded_irq() 
documentation states that "From the point this call is made your handler 
function may be invoked."
And rkvdec_irq_handler() may call rkvdec_iommu_restore() which uses 
empty_domain.

Not sure if I'm right and if this can happen, but the existing order 
looks safer to me.


That said, if it is fine for you, I can send a v2.


This would be:

diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.c 
b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
index d707088ec0dc..6eae10e16c73 100644
--- a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
+++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
@@ -1159,13 +1159,6 @@ static int rkvdec_probe(struct platform_device *pdev)
                 return ret;
         }

-       if (iommu_get_domain_for_dev(&pdev->dev)) {
-               rkvdec->empty_domain = 
iommu_paging_domain_alloc(rkvdec->dev);
-
-               if (!rkvdec->empty_domain)
-                       dev_warn(rkvdec->dev, "cannot alloc new empty 
domain\n");
-       }
-
         vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));

         irq = platform_get_irq(pdev, 0);
@@ -1188,6 +1181,13 @@ static int rkvdec_probe(struct platform_device *pdev)
         if (ret)
                 goto err_disable_runtime_pm;

+       if (iommu_get_domain_for_dev(&pdev->dev)) {
+               rkvdec->empty_domain = 
iommu_paging_domain_alloc(rkvdec->dev);
+
+               if (!rkvdec->empty_domain)
+                       dev_warn(rkvdec->dev, "cannot alloc new empty 
domain\n");
+       }
+
         return 0;

  err_disable_runtime_pm:


CJ

> 
> Nicolas
> 
>> @@ -1193,6 +1195,9 @@ static int rkvdec_probe(struct platform_device *pdev)
>>   err_disable_runtime_pm:
>>   	pm_runtime_dont_use_autosuspend(&pdev->dev);
>>   	pm_runtime_disable(&pdev->dev);
>> +err_free_domain:
>> +	if (rkvdec->empty_domain)
>> +		iommu_domain_free(rkvdec->empty_domain);
>>   	return ret;
>>   }
>>