On Fri, Mar 29, 2024 at 09:18:25PM +0100, Maximilian Luz wrote:
> It turns out that while the QSEECOM APP_SEND command has specific fields
> for request and response buffers, uefisecapp expects them both to be in
> a single memory region. Failure to adhere to this has (so far) resulted
> in either no response being written to the response buffer (causing an
> EIO to be emitted down the line), the SCM call to fail with EINVAL
> (i.e., directly from TZ/firmware), or the device to be hard-reset.
>
> While this issue can be triggered deterministically, in the current form
> it seems to happen rather sporadically (which is why it has gone
> unnoticed during earlier testing). This is likely due to the two
> kzalloc() calls (for request and response) being directly after each
> other. Which means that those likely return consecutive regions most of
> the time, especially when not much else is going on in the system.
>
> Fix this by allocating a single memory region for both request and
> response buffers, properly aligning both structs inside it. This
> unfortunately also means that the qcom_scm_qseecom_app_send() interface
> needs to be restructured, as it should no longer map the DMA regions
> separately. Therefore, move the responsibility of DMA allocation (or
> mapping) to the caller.
>
> Fixes: 759e7a2b62eb ("firmware: Add support for Qualcomm UEFI Secure Application")
> Signed-off-by: Maximilian Luz <luzmaximilian@xxxxxxxxx>
Thanks for tracking this down. I can confirm that this fixes the
hypervisor reset I can trigger by repeatedly reading an EFI variable on
the X13s. Before it triggered within minutes, now I ran it for 3 hours
without any issues:
Tested-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
Even if the patch is a bit large it's straight-forward and fixes a
critical bug so I think this needs to go to stable as well:
Cc: stable@xxxxxxxxxxxxxxx # 6.7
A couple of comments below.
> @@ -277,10 +296,15 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
> unsigned long buffer_size = *data_size;
> efi_status_t efi_status = EFI_SUCCESS;
> unsigned long name_length;
> + dma_addr_t cmd_buf_phys;
Throughout the patch, could you please refer to DMA addresses as DMA
addresses rather than physical addresses, for example, by using a "_dma"
rather than "_phys" suffix here?
> + size_t cmd_buf_size;
> + void *cmd_buf_virt;
I'd also prefer if you dropped the "_virt" suffix from the buffer
pointers.
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 49ddbcab06800..fc59688227f43 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -1579,46 +1579,26 @@ EXPORT_SYMBOL_GPL(qcom_scm_qseecom_app_get_id);
> /**
> * qcom_scm_qseecom_app_send() - Send to and receive data from a given QSEE app.
> * @app_id: The ID of the target app.
> - * @req: Request buffer sent to the app (must be DMA-mappable).
> + * @req: Physical address of the request buffer sent to the app.
DMA address
> * @req_size: Size of the request buffer.
> - * @rsp: Response buffer, written to by the app (must be DMA-mappable).
> + * @rsp: Physical address of the response buffer, written to by the app.
DMA address
> * @rsp_size: Size of the response buffer.
> *
> * Sends a request to the QSEE app associated with the given ID and read back
> - * its response. The caller must provide two DMA memory regions, one for the
> - * request and one for the response, and fill out the @req region with the
> + * its response. The caller must provide two physical memory regions, one for
DMA memory
> + * the request and one for the response, and fill out the @req region with the
> * respective (app-specific) request data. The QSEE app reads this and returns
> * its response in the @rsp region.
> *
> * Return: Zero on success, nonzero on failure.
> */
> -int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
> - size_t rsp_size)
> +int qcom_scm_qseecom_app_send(u32 app_id, dma_addr_t req, size_t req_size,
> + dma_addr_t rsp, size_t rsp_size)
And similar throughout.
With that fixed, feel free to add:
Reviewed-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
Johan
