| Message ID | 20230528230351.168210-3-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | firmware: Add support for Qualcomm UEFI Secure Application | expand |
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
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 --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();
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(-)