diff mbox series

[v4,2/4] firmware: qcom_scm: Clear scm pointer on probe failure

Message ID 20230528230351.168210-3-luzmaximilian@gmail.com
State New
Headers
Series firmware: Add support for Qualcomm UEFI Secure Application |

Commit Message

Maximilian Luz May 28, 2023, 11:03 p.m. UTC 
  When setting-up the IRQ goes wrong, the __scm pointer currently remains
set even though we fail to probe the driver successfully. Due to this,
access to __scm may go wrong since associated resources (clocks, ...)
have been released. Therefore, clear the __scm pointer when setting-up
the IRQ fails.

Fixes: 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling logic")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---

Patch introduced in v4

---
 drivers/firmware/qcom_scm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Johan Hovold June 28, 2023, 11:20 a.m. UTC | #1 
On Mon, May 29, 2023 at 01:03:49AM +0200, Maximilian Luz wrote:
> When setting-up the IRQ goes wrong, the __scm pointer currently remains
> set even though we fail to probe the driver successfully. Due to this,
> access to __scm may go wrong since associated resources (clocks, ...)
> have been released. Therefore, clear the __scm pointer when setting-up
> the IRQ fails.
> 
> Fixes: 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling logic")
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---
> 
> Patch introduced in v4
> 
> ---
>  drivers/firmware/qcom_scm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index fde33acd46b7..d0070b833889 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -1488,8 +1488,10 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	} else {
>  		ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
>  						IRQF_ONESHOT, "qcom-scm", __scm);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			__scm = NULL;

This looks fragile at best. Clients use qcom_scm_is_available() to see
if __scm is available and do not expect it to go away once it is live.

It looks like you can hold off on initialising __scm until you've
requested the interrupt, either by using IRQ_NOAUTOEN or fixing
qcom_scm_waitq_wakeup() so that it doesn't use __scm directly.

That would also take care of the previous branch which may also leave
__scm set after the structure itself has been released on errors.

You'll have similar problems when registering qseecom which currently
depend on __scm being set, though. Clearing the pointer in that case is
clearly broken as you currently rely on devres for deregistering the aux
clients on errors (i.e. the clients using __scm are still registered
when you clear the pointer in patch 3/4).

>  			return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n");
> +		}
>  	}
>  
>  	__get_convention();

Johan
Maximilian Luz July 20, 2023, 6:55 p.m. UTC | #2 
First off, sorry again for the long delay and thanks for being patient
with me (and for the review of course). I'm finally getting back to
finding some time for Linux things again, so I think I've mostly settled
in by now.

On 6/28/23 13:20, Johan Hovold wrote:
> On Mon, May 29, 2023 at 01:03:49AM +0200, Maximilian Luz wrote:
>> When setting-up the IRQ goes wrong, the __scm pointer currently remains
>> set even though we fail to probe the driver successfully. Due to this,
>> access to __scm may go wrong since associated resources (clocks, ...)
>> have been released. Therefore, clear the __scm pointer when setting-up
>> the IRQ fails.
>>
>> Fixes: 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling logic")
>> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
>> ---
>>
>> Patch introduced in v4
>>
>> ---
>>   drivers/firmware/qcom_scm.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index fde33acd46b7..d0070b833889 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -1488,8 +1488,10 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>   	} else {
>>   		ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
>>   						IRQF_ONESHOT, "qcom-scm", __scm);
>> -		if (ret < 0)
>> +		if (ret < 0) {
>> +			__scm = NULL;
> 
> This looks fragile at best. Clients use qcom_scm_is_available() to see
> if __scm is available and do not expect it to go away once it is live.

Hmm, you're right. The whole situation is probably not ideal and that
fix is really just a bad band-aid.

> It looks like you can hold off on initialising __scm until you've
> requested the interrupt, either by using IRQ_NOAUTOEN or fixing
> qcom_scm_waitq_wakeup() so that it doesn't use __scm directly.
> 
> That would also take care of the previous branch which may also leave
> __scm set after the structure itself has been released on errors.

Agreed.

> You'll have similar problems when registering qseecom which currently
> depend on __scm being set, though. Clearing the pointer in that case is
> clearly broken as you currently rely on devres for deregistering the aux
> clients on errors (i.e. the clients using __scm are still registered
> when you clear the pointer in patch 3/4).

Oh right, I hadn't thought of that. I'll have to rework that.

>>   			return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n");
>> +		}
>>   	}
>>   
>>   	__get_convention();
> 
> Johan
diff mbox series

Patch

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index fde33acd46b7..d0070b833889 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1488,8 +1488,10 @@  static int qcom_scm_probe(struct platform_device *pdev)
 	} else {
 		ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
 						IRQF_ONESHOT, "qcom-scm", __scm);
-		if (ret < 0)
+		if (ret < 0) {
+			__scm = NULL;
 			return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n");
+		}
 	}
 
 	__get_convention();