Re: [PATCH v3 4/4] firmware: Add support for Qualcomm UEFI Secure Application

[Maximilian Luz <luzmaximilian@gmail.com>]

On 3/9/23 09:36, Dmitry Baryshkov wrote:
> On 08/03/2023 17:02, Maximilian Luz wrote:
>> On 3/7/23 16:51, Dmitry Baryshkov wrote:
>>> On 05/03/2023 04:21, Maximilian Luz wrote:
>>>> On platforms using the Qualcomm UEFI Secure Application (uefisecapp),

[...]

>>
>> As I've elaborated on a previous version: I'm a bit wary of using
>> qseecom_app_get_id() in this way, since the Windows driver I've got this from
>> expects the app to be present when calling that function. So I don't know much
>> about the failure cases, especially when it isn't present.
>>
>> At this point, I'm just assuming that "res.status != QSEECOM_RESULT_SUCCESS"
>> means the app isn't present, but I don't know whether this can fail in other
>> ways. For a proper detection system I'd prefer if we can differentiate between
>> "some internal failure" and "not-present" cases.
> 
> Please take a look at https://git.codelinaro.org/clo/la/kernel/msm-5.10/-/blob/KERNEL.PLATFORM.1.0.r1-13000-kernel.0/drivers/misc/qseecom.c#L2683
> 
> Note, the driver supports loading and unloading applications, we can ignore that for now.
>

Thanks! That looks quite helpful.

[...]

>>>> +static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const efi_char16_t *name,
>>>> +                       const efi_guid_t *guid, u32 *attributes,
>>>> +                       unsigned long *data_size, void *data)
>>>> +{
>>>> +    struct qsee_req_uefi_get_variable *req_data;
>>>> +    struct qsee_rsp_uefi_get_variable *rsp_data;
>>>> +    struct qseecom_dma dma_req;
>>>> +    struct qseecom_dma dma_rsp;
>>>> +    unsigned long name_size = utf16_strsize(name);
>>>> +    unsigned long buffer_size = *data_size;
>>>> +    unsigned long size;
>>>> +    efi_status_t efi_status;
>>>> +    int status;
>>>> +
>>>> +    /* Validation: We need a name and GUID. */
>>>> +    if (!name || !guid)
>>>> +        return EFI_INVALID_PARAMETER;
>>>> +
>>>> +    /* Validation: We need a buffer if the buffer_size is nonzero. */
>>>> +    if (buffer_size && !data)
>>>> +        return EFI_INVALID_PARAMETER;
>>>> +
>>>> +    /* Compute required size (upper limit with alignments). */
>>>> +    size = sizeof(*req_data) + sizeof(*guid) + name_size  /* Inputs. */
>>>> +           + sizeof(*rsp_data) + buffer_size              /* Outputs. */
>>>> +           + 2 * (QSEECOM_DMA_ALIGNMENT - 1)              /* Input parameter alignments. */
>>>> +           + 1 * (QSEECOM_DMA_ALIGNMENT - 1);             /* Output parameter alignments. */
>>>
>>> Do we need to pack everything into a single DMA buffer? Otherwise it would be better to add qseecom_dma_alloc_aligned function, which will take care of the alignment for a single data piece.
>>
>> It may be possible to split this into two buffers, one for input and one for
>> output, but packing of input parameters would still be required (see the
>> assignments to req_data below).
>>
>> For the input, you essentially provide one buffer (address) to qseecom,
>> starting with req_data describing the layout in it. This description is
>> offset-based, so there's no way to specify multiple addresses/buffers as input.
>> The output behaves similarly, it's just the secure OS that does the packing.
>>
>> And since we already have to take care of aligning the input parameters, I'm
>> not sure that it makes sense to split this into two.
> 
> I see, thanks for the explanation. Maybe you can add a wrapping call that will take the sizes of required arguments (as a variadic array?) and will return prepared req and all the pointers and/or offsets? I think that having to specify these alignment 'extras' is errror prone.

I'll give that a try.

[...]

>>>> +static int qcom_uefisecapp_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct qcuefi_client *qcuefi;
>>>> +    int status;
>>>> +
>>>> +    /* Allocate driver data. */
>>>> +    qcuefi = devm_kzalloc(&pdev->dev, sizeof(*qcuefi), GFP_KERNEL);
>>>> +    if (!qcuefi)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    qcuefi->dev = &pdev->dev;
>>>> +
>>>> +    /* We expect the parent to be the QSEECOM device. */
>>>> +    qcuefi->qsee = dev_get_drvdata(pdev->dev.parent);
>>>> +    if (!qcuefi->qsee)
>>>> +        return -EINVAL;
>>>> +
>>>> +    /* Get application id for uefisecapp. */
>>>> +    status = qseecom_app_get_id(qcuefi->qsee, QSEE_UEFISEC_APP_NAME, &qcuefi->app_id);
>>>> +    if (status) {
>>>> +        dev_err(&pdev->dev, "failed to query app ID: %d\n", status);
>>>> +        return status;
>>>> +    }
>>>> +
>>>> +    /* Set up DMA. One page should be plenty to start with. */
>>>
>>> one page?
>>
>> The driver I've reverse-engineered this from allocates the DMA memory for
>> interaction with qseecom in multiples of PAGE_SIZE. I'm following that in this
>> driver, as I don't know whether that's a hard requirement (at least on some
>> platforms) or not. So I pre-allocate one page (1x PAGE_SIZE bytes) here. But as
>> you've mentioned above, it might be better to allocate this on-demand in each
>> call.
> 
> Probably the comment was misplaced. It talks about 1 page, but it is placed right before a call to dma_set_mask rather than dma_alloc.

Ah, it was intended for both the dma_set_mask() and the qseecom_dma_alloc()
below. I see how that is a bit confusing, will fix that.

>>>> +    if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
>>>> +        dev_warn(&pdev->dev, "no suitable DMA available\n");
>>>> +        return -EFAULT;
>>>> +    }
>>>> +
>>>> +    status = qseecom_dma_alloc(&pdev->dev, &qcuefi->dma, PAGE_SIZE, GFP_KERNEL);
>>>> +    if (status)
>>>> +        return status;
>>>> +

[...]

Regards
Max