Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b8b82aee-45a5-5e56-1737-4ec78f6279c2@gmail.com>
Date:   Thu, 3 Aug 2023 19:09:34 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 3/3] firmware: Add support for Qualcomm UEFI Secure
 Application
To:     Ard Biesheuvel <ardb@kernel.org>
Cc:     Bjorn Andersson <andersson@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Ilias Apalodimas <ilias.apalodimas@linaro.org>,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Sudeep Holla <sudeep.holla@arm.com>,
        Johan Hovold <johan@kernel.org>,
        Steev Klimaszewski <steev@kali.org>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230730161906.606163-1-luzmaximilian@gmail.com>
 <20230730161906.606163-4-luzmaximilian@gmail.com>
 <CAMj1kXHOaEuP2Wds9ZU4RLx9oKhthvE=yR-Ju_Ka2boqTmTYNw@mail.gmail.com>
Content-Language: en-US
From:   Maximilian Luz <luzmaximilian@gmail.com>
In-Reply-To: <CAMj1kXHOaEuP2Wds9ZU4RLx9oKhthvE=yR-Ju_Ka2boqTmTYNw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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