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

[Maximilian Luz]

On 4/2/24 2:30 PM, Johan Hovold wrote:
> 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?

Sure. I've applied this and the changes below (as well as similar ones
for qcom_qseecom.h) for v2.

Thanks!

Best regards,
Max

> > +	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