Re: [PATCH] firmware: qcom: uefisecapp: Fix memory related IO errors and crashes

[Johan Hovold]

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