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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 4 Aug 2023 12:56:55 +0200
From:   Ard Biesheuvel <ardb@...nel.org>
To:     Maximilian Luz <luzmaximilian@...il.com>
Cc:     Bjorn Andersson <andersson@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Johan Hovold <johan@...nel.org>,
        Steev Klimaszewski <steev@...i.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/3] firmware: Add support for Qualcomm UEFI Secure Application

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

I take it this will go via the QCOM tree?