| Message ID | 20250120151000.13870-1-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: qcom: uefisecapp: fix efivars registration race | expand |
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
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>
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
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; }
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(-)