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

[Maximilian Luz <luzmaximilian@gmail.com>]

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@gmail.com>
> 
> 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@kernel.org>
> 
> 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@vger.kernel.org	# 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@kernel.org>
> 
> Johan