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;
MIME-Version: 1.0
References: <20230730161906.606163-1-luzmaximilian@gmail.com>
 <20230730161906.606163-4-luzmaximilian@gmail.com> <CAMj1kXHOaEuP2Wds9ZU4RLx9oKhthvE=yR-Ju_Ka2boqTmTYNw@mail.gmail.com>
 <b8b82aee-45a5-5e56-1737-4ec78f6279c2@gmail.com>
In-Reply-To: <b8b82aee-45a5-5e56-1737-4ec78f6279c2@gmail.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Fri, 4 Aug 2023 12:56:55 +0200
Message-ID: <CAMj1kXEWf=Wd8kgt=3AHUquE+NOZ6DMXUZam75_44qD8YPXHkg@mail.gmail.com>
Subject: Re: [PATCH v5 3/3] firmware: Add support for Qualcomm UEFI Secure Application
To:     Maximilian Luz <luzmaximilian@gmail.com>
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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 3 Aug 2023 at 19:09, Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> 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.
>

Fair enough.

The series looks good to me.

Acked-by: Ard Biesheuvel <ardb@kernel.org>

I take it this will go via the QCOM tree?