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;
Date:   Thu, 19 Jan 2023 17:47:57 +0100
From:   Johan Hovold <johan@kernel.org>
To:     Maximilian Luz <luzmaximilian@gmail.com>
Cc:     Ard Biesheuvel <ardb@kernel.org>, Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Steev Klimaszewski <steev@kali.org>,
        Shawn Guo <shawn.guo@linaro.org>,
        Sudeep Holla <sudeep.holla@arm.com>,
        Cristian Marussi <cristian.marussi@arm.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-arm-msm@vger.kernel.org, linux-efi@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] firmware: Add support for Qualcomm UEFI Secure
 Application
Message-ID: <Y8l0PdZlXLym//xS@hovoldconsulting.com>
References: <20220723224949.1089973-1-luzmaximilian@gmail.com>
 <20220723224949.1089973-4-luzmaximilian@gmail.com>
 <Y8ZbN5LNn2fk0/xi@hovoldconsulting.com>
 <2b0fdc2d-6457-059b-bbdf-27e7de59abeb@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2b0fdc2d-6457-059b-bbdf-27e7de59abeb@gmail.com>
Precedence: bulk

On Wed, Jan 18, 2023 at 09:45:18PM +0100, Maximilian Luz wrote:
> On 1/17/23 09:24, Johan Hovold wrote:
> > On Sun, Jul 24, 2022 at 12:49:48AM +0200, Maximilian Luz wrote:

> >> +module_platform_driver(qcom_uefisecapp_driver);
> > 
> > I noticed that for efivarfs to work, you're currently relying on having
> > the firmware still claim that the variable services are supported in the
> > RT_PROP table so that efi core registers the default ops at subsys init
> > time (which are later overridden by this driver).
> > 
> > Otherwise efivarfs may fail to initialise when built in:
> > 
> > 	static __init int efivarfs_init(void)
> > 	{
> > 		if (!efivars_kobject())
> > 			return -ENODEV;
> > 
> > 		return register_filesystem(&efivarfs_type);
> > 	}
> > 
> > 	module_init(efivarfs_init);
> > 
> > With recent X13s firmware the corresponding bit in the RT_PROP table has
> > been cleared so that efivarfs would fail to initialise. Similar problem
> > when booting with 'efi=noruntime'.
> > 
> > One way to handle this is to register also the qcom_uefisecapp_driver at
> > subsys init time and prevent it from being built as a module (e.g. as is
> > done for the SCM driver). I'm using the below patch for this currently.
> 
> So I've had another look and I'm not sure this will work reliably:
> 
> First, you are correct in case the RT_PROP table is cleared. In that
> case, using subsys_initcall() will move the efivar registration before
> the efivarfs_init() call.
> 
> However, in case EFI indicates support for variables, we will then have
> generic_ops_register() and the uefisecapp's driver call running both in
> subsys_initcall(). So if I'm not mistaken, this could cause the generic
> ops to be registered after the uefisecapp ones, which we want to avoid.

Good catch, I was using 'efi=noruntime' on the CRD so I did not notice
that race.

> One solution is bumping uefisecapp to fs_initcall(). Or do you have any
> other suggestions?

I think it would be best to avoid that if we can, but that should work.

The problem here is that the firmware claims to support the EFI variable
services even when it clearly does not and the corresponding callbacks
just return EFI_UNSUPPORTED. As far as I understand, this is still spec
compliant though so we just need to handle that.

One way to address this could be to have efi core not register the
default efivars ops in this case. That would require checking that the
services are indeed available by making one of those calls during
initialisation.

This would however expose the fact that the Google SMI implementation
implicitly relies on overriding the default ops, but I think that's a
good thing as what we have now is racy in multiple ways.

Instead I think we should move the efivarfs availability check from
module init to mount time. That should allow the Google driver, and your
SCM implementation, to continue to be built as modules.

Any consumers (e.g. the Qualcomm RTC driver) would instead need to
check if efivars is available or else defer probe.

Alternatively, it seems all efivars implementation would need to be
always-built in which is not ideal for generic kernels.

I just posted a series here as food for thought:

	https://lore.kernel.org/r/20230119164255.28091-1-johan+linaro@kernel.org

Johan