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

[Maximilian Luz <luzmaximilian@gmail.com>]

On 8/3/23 17:44, Ard Biesheuvel wrote:
> On Sun, 30 Jul 2023 at 18:19, Maximilian Luz <luzmaximilian@gmail.com> 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.

Regards
Max