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

[Maximilian Luz]

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