Re: [PATCH v5 3/3] firmware: Add support for Qualcomm UEFI Secure Application

[Ard Biesheuvel]

On Thu, 3 Aug 2023 at 19:09, Maximilian Luz <luzmaximilian@xxxxxxxxx> wrote:
>
> On 8/3/23 17:44, Ard Biesheuvel wrote:
> > On Sun, 30 Jul 2023 at 18:19, Maximilian Luz <luzmaximilian@xxxxxxxxx> wrote:
>
> [...]
>
> >> +/* -- Driver setup. --------------------------------------------------------- */
> >> +
> >> +static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
> >> +                                const struct auxiliary_device_id *aux_dev_id)
> >> +{
> >> +       struct qcuefi_client *qcuefi;
> >> +       int status;
> >> +
> >> +       qcuefi = devm_kzalloc(&aux_dev->dev, sizeof(*qcuefi), GFP_KERNEL);
> >> +       if (!qcuefi)
> >> +               return -ENOMEM;
> >> +
> >> +       qcuefi->client = container_of(aux_dev, struct qseecom_client, aux_dev);
> >> +
> >> +       auxiliary_set_drvdata(aux_dev, qcuefi);
> >> +       status = qcuefi_set_reference(qcuefi);
> >> +       if (status)
> >> +               return status;
> >> +
> >> +       status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops);
> >
> > Will this also work if the EFI runtime services were already
> > registered by the time we reach this point?
>
> That's actually a good question. In short: No. However, let me explain
> that a bit:
>
> First, we assume that we're the only other non-generic provider
> (arguably, multiple non-generic providers don't make much sense on a
> single platform anyway, so I'd say in that case it's okay to fail here).
>
> Second, we assume that the generic ops are not going to be registered at
> all on the platforms that this implementation is used. In particular, on
> the platforms I've tested and heard reports from so far, "standard"
> efivars either aren't actively advertised as "supported" or they return
> EFI_UNSUPPORTED for all calls. So we assume that either the check in
> efisubsys_init() or in generic_ops_supported() prevents registration
> of the generic ops.
>
> Further, I'd hope that the uefisecapp would not be loaded if generic ops
> would be supported on such a platform, thus preventing instantiation of
> the respective client device.
>
> So the only issue that I can see is that if uefisecapp is loaded and
> generic ops are supported, we would need a way to choose one over the
> other. But I think that is fairly unlikely to happen and I think it
> would probably be best to sort that out then (e.g. by refusing to load
> this new driver with some additional check).
>
> Apart from that case, there should not be any timing issues that could
> cause registration to fail spuriously.
>

Fair enough.

The series looks good to me.

Acked-by: Ard Biesheuvel <ardb@xxxxxxxxxx>

I take it this will go via the QCOM tree?