[PATCH] firmware: qcom: uefisecapp: fix efivars registration race

[PATCH] firmware: qcom: uefisecapp: fix efivars registration race

[Johan Hovold]

Since the conversion to using the TZ allocator, the efivars service is
registered before the memory pool has been allocated, something which
can lead to a NULL-pointer dereference in case of a racing EFI variable
access.

Make sure that all resources have been set up before registering the
efivars.

Fixes: 6612103ec35a ("firmware: qcom: qseecom: convert to using the TZ allocator")
Cc: stable@vger.kernel.org	# 6.11
Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---

Note that commit 40289e35ca52 ("firmware: qcom: scm: enable the TZ mem
allocator") looks equally broken as it allocates the tzmem pool only
after qcom_scm_is_available() returns true and other driver can start
making SCM calls.

That one appears to be a bit harder to fix as qcom_tzmem_enable()
currently depends on SCM being available, but someone should definitely
look into untangling that mess.

Johan




 .../firmware/qcom/qcom_qseecom_uefisecapp.c    | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
index 447246bd04be..98a463e9774b 100644
--- a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
+++ b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
@@ -814,15 +814,6 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
 
 	qcuefi->client = container_of(aux_dev, struct qseecom_client, aux_dev);
 
-	auxiliary_set_drvdata(aux_dev, qcuefi);
-	status = qcuefi_set_reference(qcuefi);
-	if (status)
-		return status;
-
-	status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops);
-	if (status)
-		qcuefi_set_reference(NULL);
-
 	memset(&pool_config, 0, sizeof(pool_config));
 	pool_config.initial_size = SZ_4K;
 	pool_config.policy = QCOM_TZMEM_POLICY_MULTIPLIER;
@@ -833,6 +824,15 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
 	if (IS_ERR(qcuefi->mempool))
 		return PTR_ERR(qcuefi->mempool);
 
+	auxiliary_set_drvdata(aux_dev, qcuefi);
+	status = qcuefi_set_reference(qcuefi);
+	if (status)
+		return status;
+
+	status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops);
+	if (status)
+		qcuefi_set_reference(NULL);
+
 	return status;
 }
 
-- 
2.45.2

Re: [PATCH] firmware: qcom: uefisecapp: fix efivars registration race

[Konrad Dybcio]

On 20.01.2025 4:10 PM, Johan Hovold wrote:
> Since the conversion to using the TZ allocator, the efivars service is
> registered before the memory pool has been allocated, something which
> can lead to a NULL-pointer dereference in case of a racing EFI variable
> access.
> 
> Make sure that all resources have been set up before registering the
> efivars.
> 
> Fixes: 6612103ec35a ("firmware: qcom: qseecom: convert to using the TZ allocator")
> Cc: stable@vger.kernel.org	# 6.11
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH] firmware: qcom: uefisecapp: fix efivars registration race

[Maximilian Luz]

On 1/20/25 4:10 PM, Johan Hovold wrote:
> Since the conversion to using the TZ allocator, the efivars service is
> registered before the memory pool has been allocated, something which
> can lead to a NULL-pointer dereference in case of a racing EFI variable
> access.
> 
> Make sure that all resources have been set up before registering the
> efivars.
> 
> Fixes: 6612103ec35a ("firmware: qcom: qseecom: convert to using the TZ allocator")
> Cc: stable@vger.kernel.org	# 6.11
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Looks good to me.

Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>

Re: [PATCH] firmware: qcom: uefisecapp: fix efivars registration race

[Bartosz Golaszewski]

On Mon, 20 Jan 2025 at 16:10, Johan Hovold <johan+linaro@kernel.org> wrote:
>
> Since the conversion to using the TZ allocator, the efivars service is
> registered before the memory pool has been allocated, something which
> can lead to a NULL-pointer dereference in case of a racing EFI variable
> access.
>
> Make sure that all resources have been set up before registering the
> efivars.
>
> Fixes: 6612103ec35a ("firmware: qcom: qseecom: convert to using the TZ allocator")
> Cc: stable@vger.kernel.org      # 6.11
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

> ---
>
> Note that commit 40289e35ca52 ("firmware: qcom: scm: enable the TZ mem
> allocator") looks equally broken as it allocates the tzmem pool only
> after qcom_scm_is_available() returns true and other driver can start
> making SCM calls.
>
> That one appears to be a bit harder to fix as qcom_tzmem_enable()
> currently depends on SCM being available, but someone should definitely
> look into untangling that mess.
>
> Johan

Yeah, I have it on my TODO list. I'll get to it.

Bartosz

Re: [PATCH] firmware: qcom: uefisecapp: fix efivars registration race

[Bjorn Andersson]

On Mon, 20 Jan 2025 16:10:00 +0100, Johan Hovold wrote:
> Since the conversion to using the TZ allocator, the efivars service is
> registered before the memory pool has been allocated, something which
> can lead to a NULL-pointer dereference in case of a racing EFI variable
> access.
> 
> Make sure that all resources have been set up before registering the
> efivars.
> 
> [...]

Applied, thanks!

[1/1] firmware: qcom: uefisecapp: fix efivars registration race
      commit: da8d493a80993972c427002684d0742560f3be4a

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>