Message-ID: <2b0fdc2d-6457-059b-bbdf-27e7de59abeb@gmail.com>
Date:   Wed, 18 Jan 2023 21:45:18 +0100
From:   Maximilian Luz <luzmaximilian@...il.com>
To:     Johan Hovold <johan@...nel.org>, Ard Biesheuvel <ardb@...nel.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Steev Klimaszewski <steev@...i.org>,
        Shawn Guo <shawn.guo@...aro.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Cristian Marussi <cristian.marussi@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-arm-msm@...r.kernel.org, linux-efi@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] firmware: Add support for Qualcomm UEFI Secure
 Application

On 1/17/23 09:24, Johan Hovold wrote:
> On Sun, Jul 24, 2022 at 12:49:48AM +0200, Maximilian Luz wrote:
>> On platforms using the Qualcomm UEFI Secure Application (uefisecapp),
>> EFI variables cannot be accessed via the standard interface in EFI
>> runtime mode. The respective functions return EFI_UNSUPPORTED. On these
>> platforms, we instead need to talk to uefisecapp. This commit provides
>> support for this and registers the respective efivars operations to
>> access EFI variables from the kernel.
>>
>> Communication with uefisecapp follows the standard Qualcomm Trusted
>> Environment (TEE or TrEE) / Secure OS conventions via the respective SCM
>> call interface. This is also the reason why variable access works
>> normally while boot services are active. During this time, said SCM
>> interface is managed by the boot services. When calling
>> ExitBootServices(), the ownership is transferred to the kernel.
>> Therefore, UEFI must not use that interface itself (as multiple parties
>> accessing this interface at the same time may lead to complications) and
>> cannot access variables for us.
>>
>> Signed-off-by: Maximilian Luz <luzmaximilian@...il.com>
>> ---
> 
>> +static struct platform_driver qcom_uefisecapp_driver = {
>> +	.probe = qcom_uefisecapp_probe,
>> +	.remove = qcom_uefisecapp_remove,
>> +	.driver = {
>> +		.name = "qcom_tee_uefisecapp",
>> +		.of_match_table = qcom_uefisecapp_dt_match,
>> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
>> +	},
>> +};
>> +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.

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

Regards,
Max


>  From 8fecce12d215bd8cab1b8c8f9f0d1e1fe20fe6e7 Mon Sep 17 00:00:00 2001
> From: Johan Hovold <johan+linaro@...nel.org>
> Date: Sun, 15 Jan 2023 15:32:34 +0100
> Subject: [PATCH] firmware: qcom_tee_uefisecapp: register at subsys init
> 
> Register efivars at subsys init time so that it is available when
> efivarfs probes. For the same reason, also prevent building the driver
> as a module.
> 
> This is specifically needed on platforms such as the Lenovo Thinkpad
> X13s where the firmware has cleared the variable services in the RT_PROP
> table so that efi core does not register any efivar callbacks at subsys
> init time (which are later overridden).
> 
> Signed-off-by: Johan Hovold <johan+linaro@...nel.org>
> ---
>   drivers/firmware/Kconfig               | 2 +-
>   drivers/firmware/qcom_tee_uefisecapp.c | 7 ++++++-
>   2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 4e9e2c227899..48e712e363da 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -231,7 +231,7 @@ config QCOM_TEE
>   	select QCOM_SCM
>   
>   config QCOM_TEE_UEFISECAPP
> -	tristate "Qualcomm TrEE UEFI Secure App client driver"
> +	bool "Qualcomm TrEE UEFI Secure App client driver"
>   	select QCOM_TEE
>   	depends on EFI
>   	help
> diff --git a/drivers/firmware/qcom_tee_uefisecapp.c b/drivers/firmware/qcom_tee_uefisecapp.c
> index 65573e4b815a..e83bce4da70a 100644
> --- a/drivers/firmware/qcom_tee_uefisecapp.c
> +++ b/drivers/firmware/qcom_tee_uefisecapp.c
> @@ -754,7 +754,12 @@ static struct platform_driver qcom_uefisecapp_driver = {
>   		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
>   	},
>   };
> -module_platform_driver(qcom_uefisecapp_driver);
> +
> +static int __init qcom_uefisecapp_init(void)
> +{
> +	return platform_driver_register(&qcom_uefisecapp_driver);
> +}
> +subsys_initcall(qcom_uefisecapp_init);
>   
>   MODULE_AUTHOR("Maximilian Luz <luzmaximilian@...il.com>");
>   MODULE_DESCRIPTION("Client driver for Qualcomm TrEE/TZ UEFI Secure App");

Powered by blists - more mailing lists