[PATCH v4 0/4] firmware: Add support for Qualcomm UEFI Secure Application

[PATCH v4 0/4] firmware: Add support for Qualcomm UEFI Secure Application

[Maximilian Luz]

This series adds basic support for the QSEECOM interface used to
communicate with secure applications running in the TrustZone on certain
Qualcomm devices. In addition to that, it also provides a driver for
"uefisecapp", the secure application managing access to UEFI variables
on such platforms.

For a more detailed description, see the blurb of v1.

Previous versions:

 - V3: https://lore.kernel.org/lkml/20230305022119.1331495-4-luzmaximilian@gmail.com/t/
 - V2: https://lore.kernel.org/lkml/20230127184650.756795-1-luzmaximilian@gmail.com/
 - V1: https://lore.kernel.org/lkml/20220723224949.1089973-1-luzmaximilian@gmail.com/

Patch 4 of this series depends on commit d86ff3333cb1 ("efivarfs: expose
used and total size") from the "next" branch of

 https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git 

Changes in v4:

 - Integrate the QSEECOM interface into qcom_scm.c instead of
   instantiating a custom device and requiring device-tree bindings for
   it. With that, drop the respective patches exporting SCM call
   functions from qcom_scm.c and the DT bindings.

 - Restructure management of DMA memory and move DMA mapping entirely
   into the app_send() command, removing the need for DMA handling in
   app client drivers.

 - Add support for EFI's query_variable_info() call.

 - Move UCS-2 string helpers to lib/ucs2_string.c (introduces patch 1).

 - Add fix for related cleanup-issue in qcom_scm.c (introduces patch 2).

 (Refer to individual patches for more details.)

Changes in v3:

 - Fix doc comment in qcom_scm.c
 - Rebase on top of latest changes to qcom_scm.

Changes in v2:

 - Bind the qseecom interface to a device.

 - Establish a device link between the new qseecom device and the SCM
   device to ensure proper PM and remove ordering.

 - Remove the compatible for uefisecapp. Instead, introduce a compatible
   for the qseecom device. This directly reflects ACPI tables and the
   QCOM0476 device described therein, which is responsible for the
   secure app / qseecom interface (i.e., the same purpose).

   Client devices representing apps handled by the kernel (such as
   uefisecapp) are now directly instantiated by the qseecom driver,
   based on the respective platform-specific compatible.

 - Rename the base name (qctree -> qseecom) to allow differentiation
   between old (qseecom) and new (smcinvoke) interfaces to the trusted
   execution environment. This directly reflects downstream naming by
   Qualcomm.

Maximilian Luz (4):
  lib/ucs2_string: Add UCS-2 strlcpy function
  firmware: qcom_scm: Clear scm pointer on probe failure
  firmware: qcom_scm: Add support for Qualcomm Secure Execution
    Environment SCM interface
  firmware: Add support for Qualcomm UEFI Secure Application

 MAINTAINERS                                |   6 +
 drivers/firmware/Kconfig                   |  33 +
 drivers/firmware/Makefile                  |   1 +
 drivers/firmware/qcom_qseecom_uefisecapp.c | 885 +++++++++++++++++++++
 drivers/firmware/qcom_scm.c                | 419 +++++++++-
 include/linux/firmware/qcom/qcom_scm.h     |  27 +
 include/linux/ucs2_string.h                |   1 +
 lib/ucs2_string.c                          |  16 +
 8 files changed, 1387 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/qcom_qseecom_uefisecapp.c

-- 
2.40.1

[PATCH v4 1/4] lib/ucs2_string: Add UCS-2 strlcpy function

[Maximilian Luz]

Add a ucs2_strlcpy() function for UCS-2 strings. The behavior is
equivalent to the standard strlcpy() function, just for 16-bit character
UCS-2 strings.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---

Patch introduced in v4

---
 include/linux/ucs2_string.h |  1 +
 lib/ucs2_string.c           | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/ucs2_string.h b/include/linux/ucs2_string.h
index cf3ada3e820e..ffd2a3ed84bb 100644
--- a/include/linux/ucs2_string.h
+++ b/include/linux/ucs2_string.h
@@ -10,6 +10,7 @@ typedef u16 ucs2_char_t;
 unsigned long ucs2_strnlen(const ucs2_char_t *s, size_t maxlength);
 unsigned long ucs2_strlen(const ucs2_char_t *s);
 unsigned long ucs2_strsize(const ucs2_char_t *data, unsigned long maxlength);
+unsigned long ucs2_strlcpy(ucs2_char_t *dst, const ucs2_char_t *src, unsigned long size);
 int ucs2_strncmp(const ucs2_char_t *a, const ucs2_char_t *b, size_t len);
 
 unsigned long ucs2_utf8size(const ucs2_char_t *src);
diff --git a/lib/ucs2_string.c b/lib/ucs2_string.c
index 0a559a42359b..f474c6b2fe9e 100644
--- a/lib/ucs2_string.c
+++ b/lib/ucs2_string.c
@@ -32,6 +32,22 @@ ucs2_strsize(const ucs2_char_t *data, unsigned long maxlength)
 }
 EXPORT_SYMBOL(ucs2_strsize);
 
+unsigned long
+ucs2_strlcpy(ucs2_char_t *dst, const ucs2_char_t *src, unsigned long size)
+{
+	unsigned long ret = ucs2_strlen(src);
+	unsigned long len;
+
+	if (size) {
+		len = (ret >= size) ? size - 1 : ret;
+		memcpy(dst, src, len * sizeof(*src));
+		dst[len] = 0;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(ucs2_strlcpy);
+
 int
 ucs2_strncmp(const ucs2_char_t *a, const ucs2_char_t *b, size_t len)
 {
-- 
2.40.1

[PATCH v4 2/4] firmware: qcom_scm: Clear scm pointer on probe failure

[Maximilian Luz]

When setting-up the IRQ goes wrong, the __scm pointer currently remains
set even though we fail to probe the driver successfully. Due to this,
access to __scm may go wrong since associated resources (clocks, ...)
have been released. Therefore, clear the __scm pointer when setting-up
the IRQ fails.

Fixes: 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling logic")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---

Patch introduced in v4

---
 drivers/firmware/qcom_scm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index fde33acd46b7..d0070b833889 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1488,8 +1488,10 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	} else {
 		ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
 						IRQF_ONESHOT, "qcom-scm", __scm);
-		if (ret < 0)
+		if (ret < 0) {
+			__scm = NULL;
 			return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n");
+		}
 	}
 
 	__get_convention();
-- 
2.40.1

[PATCH v4 3/4] firmware: qcom_scm: Add support for Qualcomm Secure Execution Environment SCM interface

[Maximilian Luz]

Add support for SCM calls to Secure OS and the Secure Execution
Environment (SEE) residing in the TrustZone (TZ) via the QSEECOM
interface. This allows communication with Secure/TZ applications, for
example 'uefisecapp' managing access to UEFI variables.

The added interface attempts to automatically detect known and supported
applications, creating a client (auxiliary) device for each one. The
respective client/auxiliary driver is then responsible for managing and
communicating with the application.

While this patch introduces only a very basic interface without the more
advanced features (such as re-entrant and blocking SCM calls and
listeners/callbacks), this is enough to talk to the aforementioned
'uefisecapp'.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---

Changes in v4:
 - Remove instantiation of dedicated QSEECOM device and load the driver
   via qcom_scm instead. In particular:
   - Add a list of tested devices to ensure that we don't run into any
     issues with the currently unimplemented re-entrant calls.
   - Use the QSEECOM version to check for general availability of the
     interface.
   - Attempt to automatically detect available QSEECOM applications
     (and instantiate respective clients) based on a fixed list.
 - Use auxiliary bus and devices for clients instead of MFD.
 - Restructure DMA allocations: Use dma_map_single() directly inside 
   qcom_scm_qseecom_app_send() instead of requiring clients to allocate
   DMA memory themselves.

Changes in v3:
 - Rebase ontop of latest qcom_scm changes (qcom_scm.h moved).
 - Move qcom_qseecom.h in accordance with qcom_scm.

Changes in v2:
 - Bind the interface to a device.
 - Establish a device link to the SCM device to ensure proper ordering.
 - Register client apps as child devices instead of requiring them to be
   specified in the device tree.
 - Rename (qctree -> qseecom) to allow differentiation between old
   (qseecom) and new (smcinvoke) interfaces to the trusted execution
   environment.

---
 drivers/firmware/Kconfig               |  16 +
 drivers/firmware/qcom_scm.c            | 413 +++++++++++++++++++++++++
 include/linux/firmware/qcom/qcom_scm.h |  27 ++
 3 files changed, 456 insertions(+)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b59e3041fd62..ad59a0ba1f48 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -226,6 +226,22 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
 
 	  Say Y here to enable "download mode" by default.
 
+config QCOM_SCM_QSEECOM
+	bool "Qualcomm QSEECOM interface"
+	depends on QCOM_SCM
+	help
+	  Various Qualcomm SoCs have a Secure Execution Environment (SEE) running
+	  in the Trust Zone. This module provides an interface to that via the
+	  QSEECOM mechanism, using SCM calls.
+
+	  The QSEECOM interface allows, among other things, access to applications
+	  running in the SEE. An example of such an application is 'uefisecapp',
+	  which is required to access UEFI variables on certain systems. If
+	  selected, the interface will also attempt to detect and register client
+	  devices for supported applications.
+
+	  Select Y here to enable the QSEECOM interface.
+
 config SYSFB
 	bool
 	select BOOT_VESA_SUPPORT
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index d0070b833889..1fa846d48795 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -2,6 +2,7 @@
 /* Copyright (c) 2010,2015,2019 The Linux Foundation. All rights reserved.
  * Copyright (C) 2015 Linaro Ltd.
  */
+#include <linux/auxiliary_bus.h>
 #include <linux/platform_device.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -59,6 +60,49 @@ struct qcom_scm_mem_map_info {
 	__le64 mem_size;
 };
 
+/**
+ * struct qcom_scm_qseecom_resp - QSEECOM SCM call response.
+ * @result:    Result or status of the SCM call. See &enum qcom_scm_qseecom_result.
+ * @resp_type: Type of the response. See &enum qcom_scm_qseecom_resp_type.
+ * @data:      Response data. The type of this data is given in @resp_type.
+ */
+struct qcom_scm_qseecom_resp {
+	u64 result;
+	u64 resp_type;
+	u64 data;
+};
+
+enum qcom_scm_qseecom_result {
+	QSEECOM_RESULT_SUCCESS			= 0,
+	QSEECOM_RESULT_INCOMPLETE		= 1,
+	QSEECOM_RESULT_BLOCKED_ON_LISTENER	= 2,
+	QSEECOM_RESULT_FAILURE			= 0xFFFFFFFF,
+};
+
+enum qcom_scm_qseecom_resp_type {
+	QSEECOM_SCM_RES_APP_ID			= 0xEE01,
+	QSEECOM_SCM_RES_QSEOS_LISTENER_ID	= 0xEE02,
+};
+
+enum qcom_scm_qseecom_tz_owner {
+	QSEECOM_TZ_OWNER_SIP			= 2,
+	QSEECOM_TZ_OWNER_TZ_APPS		= 48,
+	QSEECOM_TZ_OWNER_QSEE_OS		= 50
+};
+
+enum qcom_scm_qseecom_tz_svc {
+	QSEECOM_TZ_SVC_APP_ID_PLACEHOLDER	= 0,
+	QSEECOM_TZ_SVC_APP_MGR			= 1,
+	QSEECOM_TZ_SVC_INFO			= 6,
+};
+
+struct qseecom_app_desc {
+	const char *app_name;
+	const char *dev_name;
+};
+
+#define QSEECOM_MAX_APP_NAME_SIZE		64
+
 /* Each bit configures cold/warm boot address for one of the 4 CPUs */
 static const u8 qcom_scm_cpu_cold_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
 	0, BIT(0), BIT(3), BIT(5)
@@ -1325,6 +1369,369 @@ static int qcom_scm_find_dload_address(struct device *dev, u64 *addr)
 	return 0;
 }
 
+#ifdef CONFIG_QCOM_SCM_QSEECOM
+
+/* Lock for QSEECOM SCM call executions */
+DEFINE_MUTEX(qcom_scm_qseecom_call_lock);
+
+static int __qcom_scm_qseecom_call(const struct qcom_scm_desc *desc,
+				   struct qcom_scm_qseecom_resp *res)
+{
+	struct qcom_scm_res scm_res = {};
+	int status;
+
+	/*
+	 * QSEECOM SCM calls should not be executed concurrently. Therefore, we
+	 * require the respective call lock to be held.
+	 */
+	lockdep_assert_held(&qcom_scm_qseecom_call_lock);
+
+	status = qcom_scm_call(__scm->dev, desc, &scm_res);
+
+	res->result = scm_res.result[0];
+	res->resp_type = scm_res.result[1];
+	res->data = scm_res.result[2];
+
+	if (status)
+		return status;
+
+	return 0;
+}
+
+/**
+ * qcom_scm_qseecom_call() - Perform a QSEECOM SCM call.
+ * @desc: SCM call descriptor.
+ * @res:  SCM call response (output).
+ *
+ * Performs the QSEECOM SCM call described by @desc, returning the response in
+ * @rsp.
+ *
+ * Return: Returns zero on success, nonzero on failure.
+ */
+static int qcom_scm_qseecom_call(const struct qcom_scm_desc *desc,
+				 struct qcom_scm_qseecom_resp *res)
+{
+	int status;
+
+	/*
+	 * Note: Multiple QSEECOM SCM calls should not be executed same time,
+	 * so lock things here. This needs to be extended to callback/listener
+	 * handling when support for that is implemented.
+	 */
+
+	mutex_lock(&qcom_scm_qseecom_call_lock);
+	status = __qcom_scm_qseecom_call(desc, res);
+	mutex_unlock(&qcom_scm_qseecom_call_lock);
+
+	dev_dbg(__scm->dev, "%s: owner=%x, svc=%x, cmd=%x, result=%lld, type=%llx, data=%llx\n",
+		__func__, desc->owner, desc->svc, desc->cmd, res->result,
+		res->resp_type, res->data);
+
+	if (status) {
+		dev_err(__scm->dev, "qseecom: scm call failed with error %d\n", status);
+		return status;
+	}
+
+	/*
+	 * TODO: Handle incomplete and blocked calls:
+	 *
+	 * Incomplete and blocked calls are not supported yet. Some devices
+	 * and/or commands require those, some don't. Let's warn about them
+	 * prominently in case someone attempts to try these commands with a
+	 * device/command combination that isn't supported yet.
+	 */
+	WARN_ON(res->result == QSEECOM_RESULT_INCOMPLETE);
+	WARN_ON(res->result == QSEECOM_RESULT_BLOCKED_ON_LISTENER);
+
+	return 0;
+}
+
+/**
+ * qcom_scm_qseecom_get_version() - Query the QSEECOM version.
+ * @version: Pointer where the QSEECOM version will be stored.
+ *
+ * Performs the QSEECOM SCM querying the QSEECOM version currently running in
+ * the TrustZone.
+ *
+ * Return: Returns zero on success, nonzero on failure.
+ */
+static int qcom_scm_qseecom_get_version(u32 *version)
+{
+	struct qcom_scm_desc desc = {};
+	struct qcom_scm_qseecom_resp res = {};
+	u32 feature = 10;
+	int ret;
+
+	desc.owner = QSEECOM_TZ_OWNER_SIP;
+	desc.svc = QSEECOM_TZ_SVC_INFO;
+	desc.cmd = 0x03;
+	desc.arginfo = QCOM_SCM_ARGS(1, QCOM_SCM_VAL);
+	desc.args[0] = feature;
+
+	ret = qcom_scm_qseecom_call(&desc, &res);
+	if (ret)
+		return ret;
+
+	*version = res.result;
+	return 0;
+}
+
+/**
+ * qcom_scm_qseecom_app_get_id() - Query the app ID for a given QSEE app name.
+ * @app_name: The name of the app.
+ * @app_id:   The returned app ID.
+ *
+ * Query and return the application ID of the SEE app identified by the given
+ * name. This returned ID is the unique identifier of the app required for
+ * subsequent communication.
+ *
+ * Return: Returns zero on success, nonzero on failure. Returns -ENOENT if the
+ * app has not been loaded or could not be found.
+ */
+static int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id)
+{
+	unsigned long name_buf_size = QSEECOM_MAX_APP_NAME_SIZE;
+	unsigned long app_name_len = strlen(app_name);
+	struct qcom_scm_desc desc = {};
+	struct qcom_scm_qseecom_resp res = {};
+	dma_addr_t name_buf_phys;
+	char *name_buf;
+	int status;
+
+	if (app_name_len >= name_buf_size)
+		return -EINVAL;
+
+	name_buf = kzalloc(name_buf_size, GFP_KERNEL);
+	if (!name_buf)
+		return -ENOMEM;
+
+	memcpy(name_buf, app_name, app_name_len);
+
+	name_buf_phys = dma_map_single(__scm->dev, name_buf, name_buf_size, DMA_TO_DEVICE);
+	if (dma_mapping_error(__scm->dev, name_buf_phys)) {
+		kfree(name_buf);
+		dev_err(__scm->dev, "qseecom: failed to map dma address\n");
+		return -EFAULT;
+	}
+
+	desc.owner = QSEECOM_TZ_OWNER_QSEE_OS;
+	desc.svc = QSEECOM_TZ_SVC_APP_MGR;
+	desc.cmd = 0x03;
+	desc.arginfo = QCOM_SCM_ARGS(2, QCOM_SCM_RW, QCOM_SCM_VAL);
+	desc.args[0] = name_buf_phys;
+	desc.args[1] = app_name_len;
+
+	status = qcom_scm_qseecom_call(&desc, &res);
+	dma_unmap_single(__scm->dev, name_buf_phys, name_buf_size, DMA_TO_DEVICE);
+	kfree(name_buf);
+
+	if (status)
+		return status;
+
+	if (res.result == QSEECOM_RESULT_FAILURE)
+		return -ENOENT;
+
+	if (res.result != QSEECOM_RESULT_SUCCESS)
+		return -EINVAL;
+
+	if (res.resp_type != QSEECOM_SCM_RES_APP_ID)
+		return -EINVAL;
+
+	*app_id = res.data;
+	return 0;
+}
+
+/**
+ * qcom_scm_qseecom_app_send() - Send to and receive data from a given QSEE app.
+ * @client:   The QSEECOM client device corresponding to the target app.
+ * @req:      Request buffer sent to the app (must be DMA-mappable).
+ * @req_size: Size of the request buffer.
+ * @rsp:      Response buffer, written to by the app (must be DMA-mappable).
+ * @rsp_size: Size of the response buffer.
+ *
+ * Sends a request to the QSEE app associated with the given client and read
+ * back its response. The caller must provide two DMA memory regions, one for
+ * the request and one for the response, and fill out the @req region with the
+ * respective (app-specific) request data. The QSEE app reads this and returns
+ * its response in the @rsp region.
+ *
+ * Return: Returns zero on success, nonzero error code on failure.
+ */
+int qcom_scm_qseecom_app_send(struct qseecom_client *client, void *req,
+			      size_t req_size, void *rsp, size_t rsp_size)
+{
+	struct qcom_scm_qseecom_resp res = {};
+	struct qcom_scm_desc desc = {};
+	dma_addr_t req_phys;
+	dma_addr_t rsp_phys;
+	int status;
+
+	/* Map request buffer */
+	req_phys = dma_map_single(__scm->dev, req, req_size, DMA_TO_DEVICE);
+	if (dma_mapping_error(__scm->dev, req_phys)) {
+		dev_err(__scm->dev, "qseecom: failed to map request buffer\n");
+		return -EFAULT;
+	}
+
+	/* Map response buffer */
+	rsp_phys = dma_map_single(__scm->dev, rsp, rsp_size, DMA_FROM_DEVICE);
+	if (dma_mapping_error(__scm->dev, rsp_phys)) {
+		dma_unmap_single(__scm->dev, req_phys, req_size, DMA_TO_DEVICE);
+		dev_err(__scm->dev, "qseecom: failed to map response buffer\n");
+		return -EFAULT;
+	}
+
+	/* Set up SCM call data */
+	desc.owner = QSEECOM_TZ_OWNER_TZ_APPS,
+	desc.svc = QSEECOM_TZ_SVC_APP_ID_PLACEHOLDER,
+	desc.cmd = 0x01,
+	desc.arginfo = QCOM_SCM_ARGS(5, QCOM_SCM_VAL,
+				     QCOM_SCM_RW, QCOM_SCM_VAL,
+				     QCOM_SCM_RW, QCOM_SCM_VAL),
+	desc.args[0] = client->app_id,
+	desc.args[1] = req_phys;
+	desc.args[2] = req_size;
+	desc.args[3] = rsp_phys;
+	desc.args[4] = rsp_size;
+
+	/* Perform call */
+	status = qcom_scm_qseecom_call(&desc, &res);
+
+	/* Unmap buffers */
+	dma_unmap_single(__scm->dev, rsp_phys, rsp_size, DMA_FROM_DEVICE);
+	dma_unmap_single(__scm->dev, req_phys, req_size, DMA_TO_DEVICE);
+
+	if (status)
+		return status;
+
+	if (res.result != QSEECOM_RESULT_SUCCESS)
+		return -EIO;
+
+	return 0;
+}
+EXPORT_SYMBOL(qcom_scm_qseecom_app_send);
+
+static void qseecom_client_release(struct device *dev)
+{
+	struct qseecom_client *client = container_of(dev, struct qseecom_client, aux_dev.dev);
+
+	kfree(client);
+}
+
+static void qseecom_client_remove(void *data)
+{
+	struct qseecom_client *client = data;
+
+	auxiliary_device_delete(&client->aux_dev);
+	auxiliary_device_uninit(&client->aux_dev);
+}
+
+static int qseecom_client_register(const struct qseecom_app_desc *desc)
+{
+	struct qseecom_client *client;
+	u32 app_id;
+	int ret;
+
+	/* Try to find the app ID, skip device if not found */
+	ret = qcom_scm_qseecom_app_get_id(desc->app_name, &app_id);
+	if (ret)
+		return ret == -ENOENT ? 0 : ret;
+
+	dev_info(__scm->dev, "qseecom: setting up client for %s\n", desc->app_name);
+
+	/* Allocate and set-up the client device */
+	client = kzalloc(sizeof(*client), GFP_KERNEL);
+	if (!client)
+		return -ENOMEM;
+
+	client->aux_dev.name = desc->dev_name;
+	client->aux_dev.dev.parent = __scm->dev;
+	client->aux_dev.dev.release = qseecom_client_release;
+	client->app_id = app_id;
+
+	ret = auxiliary_device_init(&client->aux_dev);
+	if (ret) {
+		kfree(client);
+		return ret;
+	}
+
+	ret = auxiliary_device_add(&client->aux_dev);
+	if (ret) {
+		auxiliary_device_uninit(&client->aux_dev);
+		return ret;
+	}
+
+	/*
+	 * Ensure that the device is properly cleaned up in case of removal or
+	 * errors.
+	 */
+	ret = devm_add_action_or_reset(__scm->dev, qseecom_client_remove, client);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * We do not yet support re-entrant calls via the qseecom interface. To prevent
+ + any potential issues with this, only allow validated devices for now.
+ */
+static const struct of_device_id qcom_scm_qseecom_allowlist[] = {
+	{ .compatible = "lenovo,thinkpad-x13s", },
+	{ }
+};
+
+static bool qcom_scm_qseecom_device_allowed(void)
+{
+	struct device_node *np;
+	bool match;
+
+	np = of_find_node_by_path("/");
+	match = of_match_node(qcom_scm_qseecom_allowlist, np);
+	of_node_put(np);
+
+	return match;
+}
+
+static const struct qseecom_app_desc qcom_scm_qseecom_apps[] = {};
+
+static int qcom_scm_qseecom_init(void)
+{
+	u32 version;
+	int ret, i;
+
+	/* Try to query the qseecom version, skip qseecom setup if this fails */
+	ret = qcom_scm_qseecom_get_version(&version);
+	if (ret)
+		return 0;
+
+	dev_info(__scm->dev, "qseecom: found qseecom with version 0x%x\n", version);
+
+	/* Check against tested/verified devices */
+	if (!qcom_scm_qseecom_device_allowed()) {
+		dev_info(__scm->dev, "qseecom: untested device, skipping\n");
+		return 0;
+	}
+
+	/* Set up client devices for each base application */
+	for (i = 0; i < ARRAY_SIZE(qcom_scm_qseecom_apps); i++) {
+		ret = qseecom_client_register(&qcom_scm_qseecom_apps[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+#else /* CONFIG_QCOM_SCM_QSEECOM */
+
+static int qcom_scm_qseecom_init(void)
+{
+	return 0;
+}
+
+#endif /* CONFIG_QCOM_SCM_QSEECOM */
+
 /**
  * qcom_scm_is_available() - Checks if SCM is available
  */
@@ -1496,6 +1903,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
 
 	__get_convention();
 
+	ret = qcom_scm_qseecom_init();
+	if (ret < 0) {
+		__scm = NULL;
+		return dev_err_probe(scm->dev, ret, "Failed to initialize qseecom\n");
+	}
+
 	/*
 	 * If requested enable "download mode", from this point on warmboot
 	 * will cause the boot stages to enter download mode, unless
diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index 250ea4efb7cb..a168110ec55a 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -5,6 +5,7 @@
 #ifndef __QCOM_SCM_H
 #define __QCOM_SCM_H
 
+#include <linux/auxiliary_bus.h>
 #include <linux/err.h>
 #include <linux/types.h>
 #include <linux/cpumask.h>
@@ -122,4 +123,30 @@ extern int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
 extern int qcom_scm_lmh_profile_change(u32 profile_id);
 extern bool qcom_scm_lmh_dcvsh_available(void);
 
+/**
+ * struct qseecom_client - QSEECOM client device.
+ * @aux_dev: Underlying auxiliary device.
+ * @app_id: ID of the loaded application.
+ */
+struct qseecom_client {
+	struct auxiliary_device aux_dev;
+	u32 app_id;
+};
+
+#ifdef CONFIG_QCOM_SCM_QSEECOM
+
+int qcom_scm_qseecom_app_send(struct qseecom_client *client, void *req,
+			      size_t req_size, void *rsp, size_t rsp_size);
+
+#else /* CONFIG_QCOM_SCM_QSEECOM */
+
+static inline int qcom_scm_qseecom_app_send(struct qseecom_client *client,
+					    void *req, size_t req_size,
+					    void *rsp, size_t rsp_size)
+{
+	return 0;
+}
+
+#endif /* CONFIG_QCOM_SCM_QSEECOM */
+
 #endif
-- 
2.40.1

[PATCH v4 4/4] firmware: Add support for Qualcomm UEFI Secure Application

[Maximilian Luz]

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 Qualcomm QSEECOM / 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@gmail.com>
---

Changes in v4:
 - Adapt to changes in DMA allocation in patch 3.
 - Rework alignment: Use native alignment of types instead of a general
   8 byte alignment. While the windows driver uses 8 byte alignment for
   GUIDs, the native (4 byte) alignment seems to work fine here.
 - Add a helper macro for determining size and layout of request and
   response buffers, taking care of proper alignment.
 - Implement support for EFI's query_variable_info() call, which is now
   supported by the kernel (and expected by efivarfs).
 - Move UCS-2 string helpers to lib/ucs2_string.c

Changes in v3:
 - No functional changes.

Changes in v2:
 - Rename (qctree -> qseecom) to allow differentiation between old
   (qseecom) and new (smcinvoke) interfaces to the trusted execution
   environment.

---
 MAINTAINERS                                |   6 +
 drivers/firmware/Kconfig                   |  17 +
 drivers/firmware/Makefile                  |   1 +
 drivers/firmware/qcom_qseecom_uefisecapp.c | 885 +++++++++++++++++++++
 drivers/firmware/qcom_scm.c                |   4 +-
 5 files changed, 912 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/qcom_qseecom_uefisecapp.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 332db39d3ca8..b6a5b4ad441c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17515,6 +17515,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
 F:	drivers/mtd/nand/raw/qcom_nandc.c
 
+QUALCOMM QSEECOM UEFISECAPP DRIVER
+M:	Maximilian Luz <luzmaximilian@gmail.com>
+L:	linux-arm-msm@vger.kernel.org
+S:	Maintained
+F:	drivers/firmware/qcom_qseecom_uefisecapp.c
+
 QUALCOMM RMNET DRIVER
 M:	Subash Abhinov Kasiviswanathan <quic_subashab@quicinc.com>
 M:	Sean Tranchetti <quic_stranche@quicinc.com>
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index ad59a0ba1f48..f5885fdeddd5 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -242,6 +242,23 @@ config QCOM_SCM_QSEECOM
 
 	  Select Y here to enable the QSEECOM interface.
 
+config QCOM_QSEECOM_UEFISECAPP
+	tristate "Qualcomm SEE UEFI Secure App client driver"
+	select QCOM_SCM
+	select QCOM_SCM_QSEECOM
+	depends on EFI
+	help
+	  Various Qualcomm SoCs do not allow direct access to EFI variables.
+	  Instead, these need to be accessed via the UEFI Secure Application
+	  (uefisecapp), residing in the Secure Execution Environment (SEE).
+
+	  This module provides a client driver for uefisecapp, installing efivar
+	  operations to allow the kernel accessing EFI variables, and via that also
+	  provide user-space with access to EFI variables via efivarfs.
+
+	  Select M or Y here to provide access to EFI variables on the
+	  aforementioned platforms.
+
 config SYSFB
 	bool
 	select BOOT_VESA_SUPPORT
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 28fcddcd688f..2acb9711b2d5 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
 obj-$(CONFIG_FW_CFG_SYSFS)	+= qemu_fw_cfg.o
 obj-$(CONFIG_QCOM_SCM)		+= qcom-scm.o
 qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
+obj-$(CONFIG_QCOM_QSEECOM_UEFISECAPP) += qcom_qseecom_uefisecapp.o
 obj-$(CONFIG_SYSFB)		+= sysfb.o
 obj-$(CONFIG_SYSFB_SIMPLEFB)	+= sysfb_simplefb.o
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
diff --git a/drivers/firmware/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom_qseecom_uefisecapp.c
new file mode 100644
index 000000000000..bd3686d5ea1b
--- /dev/null
+++ b/drivers/firmware/qcom_qseecom_uefisecapp.c
@@ -0,0 +1,885 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Client driver for Qualcomm UEFI Secure Application (qcom.tz.uefisecapp).
+ * Provides access to UEFI variables on platforms where they are secured by the
+ * aforementioned Secure Execution Environment (SEE) application.
+ *
+ * Copyright (C) 2023 Maximilian Luz <luzmaximilian@gmail.com>
+ */
+
+#include <linux/efi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/ucs2_string.h>
+
+#include <linux/firmware/qcom/qcom_scm.h>
+
+/* -- Qualcomm "uefisecapp" interface definitions. -------------------------- */
+
+/* Maximum length of name string with null-terminator */
+#define QSEE_MAX_NAME_LEN			1024
+
+#define QSEE_CMD_UEFI(x)			(0x8000 | (x))
+#define QSEE_CMD_UEFI_GET_VARIABLE		QSEE_CMD_UEFI(0)
+#define QSEE_CMD_UEFI_SET_VARIABLE		QSEE_CMD_UEFI(1)
+#define QSEE_CMD_UEFI_GET_NEXT_VARIABLE		QSEE_CMD_UEFI(2)
+#define QSEE_CMD_UEFI_QUERY_VARIABLE_INFO	QSEE_CMD_UEFI(3)
+
+/**
+ * struct qsee_req_uefi_get_variable - Request for GetVariable command.
+ * @command_id:  The ID of the command. Must be %QSEE_CMD_UEFI_GET_VARIABLE.
+ * @length:      Length of the request in bytes, including this struct and any
+ *               parameters (name, GUID) stored after it as well as any padding
+ *               thereof for alignment.
+ * @name_offset: Offset from the start of this struct to where the variable
+ *               name is stored (as utf-16 string), in bytes.
+ * @name_size:   Size of the name parameter in bytes, including null-terminator.
+ * @guid_offset: Offset from the start of this struct to where the GUID
+ *               parameter is stored, in bytes.
+ * @guid_size:   Size of the GUID parameter in bytes, i.e. sizeof(efi_guid_t).
+ * @data_size:   Size of the output buffer, in bytes.
+ */
+struct qsee_req_uefi_get_variable {
+	u32 command_id;
+	u32 length;
+	u32 name_offset;
+	u32 name_size;
+	u32 guid_offset;
+	u32 guid_size;
+	u32 data_size;
+} __packed;
+
+/**
+ * struct qsee_rsp_uefi_get_variable - Response for GetVariable command.
+ * @command_id:  The ID of the command. Should be %QSEE_CMD_UEFI_GET_VARIABLE.
+ * @length:      Length of the response in bytes, including this struct and the
+ *               returned data.
+ * @status:      Status of this command.
+ * @attributes:  EFI variable attributes.
+ * @data_offset: Offset from the start of this struct to where the data is
+ *               stored, in bytes.
+ * @data_size:   Size of the returned data, in bytes. In case status indicates
+ *               that the buffer is too small, this will be the size required
+ *               to store the EFI variable data.
+ */
+struct qsee_rsp_uefi_get_variable {
+	u32 command_id;
+	u32 length;
+	u32 status;
+	u32 attributes;
+	u32 data_offset;
+	u32 data_size;
+} __packed;
+
+/**
+ * struct qsee_req_uefi_set_variable - Request for the SetVariable command.
+ * @command_id:  The ID of the command. Must be %QSEE_CMD_UEFI_SET_VARIABLE.
+ * @length:      Length of the request in bytes, including this struct and any
+ *               parameters (name, GUID, data) stored after it as well as any
+ *               padding thereof required for alignment.
+ * @name_offset: Offset from the start of this struct to where the variable
+ *               name is stored (as utf-16 string), in bytes.
+ * @name_size:   Size of the name parameter in bytes, including null-terminator.
+ * @guid_offset: Offset from the start of this struct to where the GUID
+ *               parameter is stored, in bytes.
+ * @guid_size:   Size of the GUID parameter in bytes, i.e. sizeof(efi_guid_t).
+ * @attributes:  The EFI variable attributes to set for this variable.
+ * @data_offset: Offset from the start of this struct to where the EFI variable
+ *               data is stored, in bytes.
+ * @data_size:   Size of EFI variable data, in bytes.
+ *
+ */
+struct qsee_req_uefi_set_variable {
+	u32 command_id;
+	u32 length;
+	u32 name_offset;
+	u32 name_size;
+	u32 guid_offset;
+	u32 guid_size;
+	u32 attributes;
+	u32 data_offset;
+	u32 data_size;
+} __packed;
+
+/**
+ * struct qsee_rsp_uefi_set_variable - Response for the SetVariable command.
+ * @command_id:  The ID of the command. Should be %QSEE_CMD_UEFI_SET_VARIABLE.
+ * @length:      The length of this response, i.e. the size of this struct in
+ *               bytes.
+ * @status:      Status of this command.
+ * @_unknown1:   Unknown response field.
+ * @_unknown2:   Unknown response field.
+ */
+struct qsee_rsp_uefi_set_variable {
+	u32 command_id;
+	u32 length;
+	u32 status;
+	u32 _unknown1;
+	u32 _unknown2;
+} __packed;
+
+/**
+ * struct qsee_req_uefi_get_next_variable - Request for the
+ * GetNextVariableName command.
+ * @command_id:  The ID of the command. Must be
+ *               %QSEE_CMD_UEFI_GET_NEXT_VARIABLE.
+ * @length:      Length of the request in bytes, including this struct and any
+ *               parameters (name, GUID) stored after it as well as any padding
+ *               thereof for alignment.
+ * @guid_offset: Offset from the start of this struct to where the GUID
+ *               parameter is stored, in bytes.
+ * @guid_size:   Size of the GUID parameter in bytes, i.e. sizeof(efi_guid_t).
+ * @name_offset: Offset from the start of this struct to where the variable
+ *               name is stored (as utf-16 string), in bytes.
+ * @name_size:   Size of the name parameter in bytes, including null-terminator.
+ */
+struct qsee_req_uefi_get_next_variable {
+	u32 command_id;
+	u32 length;
+	u32 guid_offset;
+	u32 guid_size;
+	u32 name_offset;
+	u32 name_size;
+} __packed;
+
+/**
+ * struct qsee_rsp_uefi_get_next_variable - Response for the
+ * GetNextVariableName command.
+ * @command_id:  The ID of the command. Should be
+ *               %QSEE_CMD_UEFI_GET_NEXT_VARIABLE.
+ * @length:      Length of the response in bytes, including this struct and any
+ *               parameters (name, GUID) stored after it as well as any padding
+ *               thereof for alignment.
+ * @status:      Status of this command.
+ * @guid_size:   Size of the GUID parameter in bytes, i.e. sizeof(efi_guid_t).
+ * @name_offset: Offset from the start of this struct to where the variable
+ *               name is stored (as utf-16 string), in bytes.
+ * @name_size:   Size of the name parameter in bytes, including null-terminator.
+ */
+struct qsee_rsp_uefi_get_next_variable {
+	u32 command_id;
+	u32 length;
+	u32 status;
+	u32 guid_offset;
+	u32 guid_size;
+	u32 name_offset;
+	u32 name_size;
+} __packed;
+
+/**
+ * struct qsee_req_uefi_query_variable_info - Response for the
+ * GetNextVariableName command.
+ * @command_id: The ID of the command. Must be
+ *              %QSEE_CMD_UEFI_QUERY_VARIABLE_INFO.
+ * @length:     The length of this request, i.e. the size of this struct in
+ *              bytes.
+ * @attributes: The storage attributes to query the info for.
+ */
+struct qsee_req_uefi_query_variable_info {
+	u32 command_id;
+	u32 length;
+	u32 attributes;
+} __packed;
+
+/**
+ * struct qsee_rsp_uefi_query_variable_info - Response for the
+ * GetNextVariableName command.
+ * @command_id:        The ID of the command. Must be
+ *                     %QSEE_CMD_UEFI_QUERY_VARIABLE_INFO.
+ * @length:            The length of this response, i.e. the size of this
+ *                     struct in bytes.
+ * @status:            Status of this command.
+ * @_pad:              Padding.
+ * @storage_space:     Full storage space size, in bytes.
+ * @remaining_space:   Free storage space available, in bytes.
+ * @max_variable_size: Maximum variable data size, in bytes.
+ */
+struct qsee_rsp_uefi_query_variable_info {
+	u32 command_id;
+	u32 length;
+	u32 status;
+	u32 _pad;
+	u64 storage_space;
+	u64 remaining_space;
+	u64 max_variable_size;
+} __packed;
+
+/* -- Alighment helpers ----------------------------------------------------- */
+
+/*
+ * Helper macro to ensure proper alignment of types (fields and arrays) when
+ * stored in some (contiguous) buffer.
+ *
+ * Note: The driver from which this one has been reverse-engineered expects an
+ * alignment of 8 bytes (64 bits) for GUIDs. Our definition of efi_guid_t,
+ * however, has an alignment of 4 byte (32 bits). So far, this seems to work
+ * fine here. See also the comment on the typedef of efi_guid_t.
+ */
+#define qcuefi_buf_align_fields(fields...)					\
+	({									\
+		size_t __len = 0;						\
+		fields								\
+		__len;								\
+	})
+
+#define __field_impl(size, align, offset)					\
+	({									\
+		size_t *__offset = (offset);					\
+		size_t __aligned;						\
+										\
+		__aligned = ALIGN(__len, align);				\
+		__len = __aligned + (size);					\
+										\
+		if (__offset)							\
+			*__offset = __aligned;					\
+	});
+
+#define __array_offs(type, count, offset)					\
+	__field_impl(sizeof(type) * (count), __alignof__(type), offset)
+
+#define __array(type, count)		__array_offs(type, count, NULL)
+#define __field_offs(type, offset)	__array_offs(type, 1, offset)
+#define __field(type)			__array_offs(type, 1, NULL)
+
+/* -- UEFI app interface. --------------------------------------------------- */
+
+struct qcuefi_client {
+	struct qseecom_client *client;
+	struct efivars efivars;
+};
+
+struct device *qcuefi_dev(struct qcuefi_client *qcuefi)
+{
+	return &qcuefi->client->aux_dev.dev;
+}
+
+static efi_status_t qsee_uefi_status_to_efi(u32 status)
+{
+	u64 category = status & 0xf0000000;
+	u64 code = status & 0x0fffffff;
+
+	return category << (BITS_PER_LONG - 32) | code;
+}
+
+static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const efi_char16_t *name,
+					   const efi_guid_t *guid, u32 *attributes,
+					   unsigned long *data_size, void *data)
+{
+	struct qsee_req_uefi_get_variable *req_data;
+	struct qsee_rsp_uefi_get_variable *rsp_data;
+	unsigned long buffer_size = *data_size;
+	efi_status_t efi_status = EFI_SUCCESS;
+	unsigned long name_length;
+	size_t guid_offs;
+	size_t name_offs;
+	size_t req_size;
+	size_t rsp_size;
+	int status;
+
+	/* Validation: We need a name and GUID. */
+	if (!name || !guid)
+		return EFI_INVALID_PARAMETER;
+
+	/* Get length of name and ensure that the name is not truncated. */
+	name_length = ucs2_strnlen(name, QSEE_MAX_NAME_LEN) + 1;
+	if (name_length > QSEE_MAX_NAME_LEN)
+		return EFI_INVALID_PARAMETER;
+
+	/* Validation: We need a buffer if the buffer_size is nonzero. */
+	if (buffer_size && !data)
+		return EFI_INVALID_PARAMETER;
+
+	/* Compute required buffer size for request. */
+	req_size = qcuefi_buf_align_fields(
+		__field(*req_data)
+		__array_offs(*name, name_length, &name_offs)
+		__field_offs(*guid, &guid_offs)
+	);
+
+	/* Compute required buffer size for response. */
+	rsp_size = qcuefi_buf_align_fields(
+		__field(*rsp_data)
+		__array(u8, buffer_size)
+	);
+
+	/* Allocate request buffer. */
+	req_data = kzalloc(req_size, GFP_KERNEL);
+	if (!req_data) {
+		efi_status = EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
+
+	/* Allocate response buffer. */
+	rsp_data = kzalloc(rsp_size, GFP_KERNEL);
+	if (!rsp_data) {
+		efi_status = EFI_OUT_OF_RESOURCES;
+		goto out_free_req;
+	}
+
+	/* Set up request data. */
+	req_data->command_id = QSEE_CMD_UEFI_GET_VARIABLE;
+	req_data->data_size = buffer_size;
+	req_data->name_offset = name_offs;
+	req_data->name_size = name_length * sizeof(*name);
+	req_data->guid_offset = guid_offs;
+	req_data->guid_size = sizeof(*guid);
+	req_data->length = req_size;
+
+	/* Copy request parameters. */
+	ucs2_strlcpy(((void *)req_data) + req_data->name_offset, name, name_length);
+	memcpy(((void *)req_data) + req_data->guid_offset, guid, req_data->guid_size);
+
+	/* Perform SCM call. */
+	status = qcom_scm_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size);
+
+	/* Check for errors and validate. */
+	if (status) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_VARIABLE) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->length < sizeof(*rsp_data)) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->status) {
+		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
+			__func__, rsp_data->status);
+		efi_status = qsee_uefi_status_to_efi(rsp_data->status);
+
+		/* Update size and attributes in case buffer is too small. */
+		if (efi_status == EFI_BUFFER_TOO_SMALL) {
+			*data_size = rsp_data->data_size;
+			if (attributes)
+				*attributes = rsp_data->attributes;
+		}
+
+		goto out_free;
+	}
+
+	if (rsp_data->length > rsp_size) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->data_offset + rsp_data->data_size > rsp_data->length) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	/* Set attributes and data size even if buffer is too small. */
+	*data_size = rsp_data->data_size;
+	if (attributes)
+		*attributes = rsp_data->attributes;
+
+	/*
+	 * If we have a buffer size of zero and no buffer, just return
+	 * attributes and required size.
+	 */
+	if (buffer_size == 0 && !data) {
+		efi_status = EFI_SUCCESS;
+		goto out_free;
+	}
+
+	/* Validate output buffer size. */
+	if (buffer_size < rsp_data->data_size) {
+		efi_status = EFI_BUFFER_TOO_SMALL;
+		goto out_free;
+	}
+
+	/* Copy to output buffer. Note: We're guaranteed to have one at this point. */
+	memcpy(data, ((void *)rsp_data) + rsp_data->data_offset, rsp_data->data_size);
+
+out_free:
+	kfree(rsp_data);
+out_free_req:
+	kfree(req_data);
+out:
+	return efi_status;
+}
+
+static efi_status_t qsee_uefi_set_variable(struct qcuefi_client *qcuefi, const efi_char16_t *name,
+					   const efi_guid_t *guid, u32 attributes,
+					   unsigned long data_size, const void *data)
+{
+	struct qsee_req_uefi_set_variable *req_data;
+	struct qsee_rsp_uefi_set_variable *rsp_data;
+	efi_status_t efi_status = EFI_SUCCESS;
+	unsigned long name_length;
+	size_t name_offs;
+	size_t guid_offs;
+	size_t data_offs;
+	size_t req_size;
+	int status;
+
+	/* Validate inputs. */
+	if (!name || !guid)
+		return EFI_INVALID_PARAMETER;
+
+	/* Get length of name and ensure that the name is not truncated. */
+	name_length = ucs2_strnlen(name, QSEE_MAX_NAME_LEN) + 1;
+	if (name_length > QSEE_MAX_NAME_LEN)
+		return EFI_INVALID_PARAMETER;
+
+	/*
+	 * Make sure we have some data if data_size is nonzero. Note: Using a
+	 * size of zero is valid and deletes the variable.
+	 */
+	if (data_size && !data)
+		return EFI_INVALID_PARAMETER;
+
+	/* Compute required buffer size for request. */
+	req_size = qcuefi_buf_align_fields(
+		__field(*req_data)
+		__array_offs(*name, name_length, &name_offs)
+		__field_offs(*guid, &guid_offs)
+		__array_offs(u8, data_size, &data_offs)
+	);
+
+	/* Allocate request buffer */
+	req_data = kzalloc(req_size, GFP_KERNEL);
+	if (!req_data) {
+		efi_status = EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
+
+	/* Allocate response buffer */
+	rsp_data = kzalloc(sizeof(*rsp_data), GFP_KERNEL);
+	if (!rsp_data) {
+		efi_status = EFI_OUT_OF_RESOURCES;
+		goto out_free_req;
+	}
+
+	/* Set up request data. */
+	req_data->command_id = QSEE_CMD_UEFI_SET_VARIABLE;
+	req_data->attributes = attributes;
+	req_data->name_offset = name_offs;
+	req_data->name_size = name_length * sizeof(*name);
+	req_data->guid_offset = guid_offs;
+	req_data->guid_size = sizeof(*guid);
+	req_data->data_offset = data_offs;
+	req_data->data_size = data_size;
+	req_data->length = req_size;
+
+	/* Copy request parameters. */
+	ucs2_strlcpy(((void *)req_data) + req_data->name_offset, name, name_length);
+	memcpy(((void *)req_data) + req_data->guid_offset, guid, req_data->guid_size);
+
+	if (data_size)
+		memcpy(((void *)req_data) + req_data->data_offset, data, req_data->data_size);
+
+	/* Perform SCM call. */
+	status = qcom_scm_qseecom_app_send(qcuefi->client, req_data, req_size,
+					   rsp_data, sizeof(*rsp_data));
+
+	/* Check for errors and validate. */
+	if (status) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->command_id != QSEE_CMD_UEFI_SET_VARIABLE) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->length != sizeof(*rsp_data)) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->status) {
+		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
+			__func__, rsp_data->status);
+		efi_status = qsee_uefi_status_to_efi(rsp_data->status);
+	}
+
+out_free:
+	kfree(rsp_data);
+out_free_req:
+	kfree(req_data);
+out:
+	return efi_status;
+}
+
+static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
+						unsigned long *name_size, efi_char16_t *name,
+						efi_guid_t *guid)
+{
+	struct qsee_req_uefi_get_next_variable *req_data;
+	struct qsee_rsp_uefi_get_next_variable *rsp_data;
+	efi_status_t efi_status = EFI_SUCCESS;
+	size_t guid_offs;
+	size_t name_offs;
+	size_t req_size;
+	size_t rsp_size;
+	int status;
+
+	/* We need some buffers. */
+	if (!name_size || !name || !guid)
+		return EFI_INVALID_PARAMETER;
+
+	/* There needs to be at least a single null-character. */
+	if (*name_size == 0)
+		return EFI_INVALID_PARAMETER;
+
+	/* Compute required buffer size for request. */
+	req_size = qcuefi_buf_align_fields(
+		__field(*req_data)
+		__field_offs(*guid, &guid_offs)
+		__array_offs(*name, *name_size / sizeof(*name), &name_offs)
+	);
+
+	/* Compute required buffer size for response. */
+	rsp_size = qcuefi_buf_align_fields(
+		__field(*rsp_data)
+		__field(*guid)
+		__array(*name, *name_size / sizeof(*name))
+	);
+
+	/* Allocate request buffer. */
+	req_data = kzalloc(req_size, GFP_KERNEL);
+	if (!req_data) {
+		efi_status = EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
+
+	/* Allocate response buffer. */
+	rsp_data = kzalloc(rsp_size, GFP_KERNEL);
+	if (!rsp_data) {
+		efi_status = EFI_OUT_OF_RESOURCES;
+		goto out_free_req;
+	}
+
+	/* Set up request data. */
+	req_data->command_id = QSEE_CMD_UEFI_GET_NEXT_VARIABLE;
+	req_data->guid_offset = guid_offs;
+	req_data->guid_size = sizeof(*guid);
+	req_data->name_offset = name_offs;
+	req_data->name_size = *name_size;
+	req_data->length = req_size;
+
+	/* Copy request parameters. */
+	memcpy(((void *)req_data) + req_data->guid_offset, guid, req_data->guid_size);
+	ucs2_strlcpy(((void *)req_data) + req_data->name_offset, name, *name_size / sizeof(*name));
+
+	/* Perform SCM call. */
+	status = qcom_scm_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size);
+
+	/* Check for errors and validate. */
+	if (status) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->length < sizeof(*rsp_data)) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->status) {
+		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
+			__func__, rsp_data->status);
+		efi_status = qsee_uefi_status_to_efi(rsp_data->status);
+
+		/* Update size with required size in case buffer is too small. */
+		if (efi_status == EFI_BUFFER_TOO_SMALL)
+			*name_size = rsp_data->name_size;
+
+		goto out_free;
+	}
+
+	if (rsp_data->length > rsp_size) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->name_size > *name_size) {
+		*name_size = rsp_data->name_size;
+		efi_status = EFI_BUFFER_TOO_SMALL;
+		goto out_free;
+	}
+
+	if (rsp_data->guid_size != sizeof(*guid)) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	/* Copy response fields. */
+	memcpy(guid, ((void *)rsp_data) + rsp_data->guid_offset, rsp_data->guid_size);
+	ucs2_strlcpy(name, ((void *)rsp_data) + rsp_data->name_offset,
+		     rsp_data->name_size / sizeof(*name));
+	*name_size = rsp_data->name_size;
+
+out_free:
+	kfree(rsp_data);
+out_free_req:
+	kfree(req_data);
+out:
+	return efi_status;
+}
+
+static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi, u32 attr,
+						  u64 *storage_space, u64 *remaining_space,
+						  u64 *max_variable_size)
+{
+	struct qsee_req_uefi_query_variable_info *req_data;
+	struct qsee_rsp_uefi_query_variable_info *rsp_data;
+	efi_status_t efi_status = EFI_SUCCESS;
+	int status;
+
+	/* Allocate request buffer. */
+	req_data = kzalloc(sizeof(*req_data), GFP_KERNEL);
+	if (!req_data) {
+		efi_status = EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
+
+	/* Allocate response buffer. */
+	rsp_data = kzalloc(sizeof(*rsp_data), GFP_KERNEL);
+	if (!rsp_data) {
+		efi_status = EFI_OUT_OF_RESOURCES;
+		goto out_free_req;
+	}
+
+	/* Set up request data. */
+	req_data->command_id = QSEE_CMD_UEFI_QUERY_VARIABLE_INFO;
+	req_data->attributes = attr;
+	req_data->length = sizeof(*req_data);
+
+	/* Perform SCM call. */
+	status = qcom_scm_qseecom_app_send(qcuefi->client, req_data, sizeof(*req_data),
+					   rsp_data, sizeof(*rsp_data));
+
+	/* Check for errors and validate. */
+	if (status) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->command_id != QSEE_CMD_UEFI_QUERY_VARIABLE_INFO) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->length != sizeof(*rsp_data)) {
+		efi_status = EFI_DEVICE_ERROR;
+		goto out_free;
+	}
+
+	if (rsp_data->status) {
+		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
+			__func__, rsp_data->status);
+		efi_status = qsee_uefi_status_to_efi(rsp_data->status);
+		goto out_free;
+	}
+
+	/* Copy response fields. */
+	if (storage_space)
+		*storage_space = rsp_data->storage_space;
+
+	if (remaining_space)
+		*remaining_space = rsp_data->remaining_space;
+
+	if (max_variable_size)
+		*max_variable_size = rsp_data->max_variable_size;
+
+out_free:
+	kfree(rsp_data);
+out_free_req:
+	kfree(req_data);
+out:
+	return efi_status;
+}
+
+/* -- Global efivar interface. ---------------------------------------------- */
+
+static struct qcuefi_client *__qcuefi;
+static DEFINE_MUTEX(__qcuefi_lock);
+
+static int qcuefi_set_reference(struct qcuefi_client *qcuefi)
+{
+	mutex_lock(&__qcuefi_lock);
+
+	if (qcuefi && __qcuefi) {
+		mutex_unlock(&__qcuefi_lock);
+		return -EEXIST;
+	}
+
+	__qcuefi = qcuefi;
+
+	mutex_unlock(&__qcuefi_lock);
+	return 0;
+}
+
+static struct qcuefi_client *qcuefi_acquire(void)
+	__acquires(&__qcuefi_lock)
+{
+	mutex_lock(&__qcuefi_lock);
+	return __qcuefi;
+}
+
+static void qcuefi_release(void)
+	__releases(&__qcuefi_lock)
+{
+	mutex_unlock(&__qcuefi_lock);
+}
+
+static efi_status_t qcuefi_get_variable(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
+					unsigned long *data_size, void *data)
+{
+	struct qcuefi_client *qcuefi;
+	efi_status_t status;
+
+	qcuefi = qcuefi_acquire();
+	if (!qcuefi)
+		return EFI_NOT_READY;
+
+	status = qsee_uefi_get_variable(qcuefi, name, vendor, attr, data_size, data);
+
+	qcuefi_release();
+	return status;
+}
+
+static efi_status_t qcuefi_set_variable(efi_char16_t *name, efi_guid_t *vendor,
+					u32 attr, unsigned long data_size, void *data)
+{
+	struct qcuefi_client *qcuefi;
+	efi_status_t status;
+
+	qcuefi = qcuefi_acquire();
+	if (!qcuefi)
+		return EFI_NOT_READY;
+
+	status = qsee_uefi_set_variable(qcuefi, name, vendor, attr, data_size, data);
+
+	qcuefi_release();
+	return status;
+}
+
+static efi_status_t qcuefi_get_next_variable(unsigned long *name_size, efi_char16_t *name,
+					     efi_guid_t *vendor)
+{
+	struct qcuefi_client *qcuefi;
+	efi_status_t status;
+
+	qcuefi = qcuefi_acquire();
+	if (!qcuefi)
+		return EFI_NOT_READY;
+
+	status = qsee_uefi_get_next_variable(qcuefi, name_size, name, vendor);
+
+	qcuefi_release();
+	return status;
+}
+
+static efi_status_t qcuefi_query_variable_info(u32 attr, u64 *storage_space, u64 *remaining_space,
+					       u64 *max_variable_size)
+{
+	struct qcuefi_client *qcuefi;
+	efi_status_t status;
+
+	qcuefi = qcuefi_acquire();
+	if (!qcuefi)
+		return EFI_NOT_READY;
+
+	status = qsee_uefi_query_variable_info(qcuefi, attr, storage_space, remaining_space,
+					       max_variable_size);
+
+	qcuefi_release();
+	return status;
+}
+
+static const struct efivar_operations qcom_efivar_ops = {
+	.get_variable = qcuefi_get_variable,
+	.set_variable = qcuefi_set_variable,
+	.get_next_variable = qcuefi_get_next_variable,
+	.query_variable_info = qcuefi_query_variable_info,
+};
+
+/* -- 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;
+
+	/* Allocate driver data. */
+	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);
+
+	/* Register global reference. */
+	auxiliary_set_drvdata(aux_dev, qcuefi);
+	status = qcuefi_set_reference(qcuefi);
+	if (status)
+		return status;
+
+	/* Register efivar ops. */
+	status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops);
+	if (status)
+		qcuefi_set_reference(NULL);
+
+	return status;
+}
+
+static void qcom_uefisecapp_remove(struct auxiliary_device *aux_dev)
+{
+	struct qcuefi_client *qcuefi = auxiliary_get_drvdata(aux_dev);
+
+	/* Unregister efivar ops. */
+	efivars_unregister(&qcuefi->efivars);
+
+	/* Block on pending calls and unregister global reference. */
+	qcuefi_set_reference(NULL);
+}
+
+static const struct auxiliary_device_id qcom_uefisecapp_id_table[] = {
+	{ .name = "qcom_scm.uefisecapp" },
+	{}
+};
+MODULE_DEVICE_TABLE(auxiliary, qcom_uefisecapp_id_table);
+
+static struct auxiliary_driver qcom_uefisecapp_driver = {
+	.probe = qcom_uefisecapp_probe,
+	.remove = qcom_uefisecapp_remove,
+	.id_table = qcom_uefisecapp_id_table,
+	.driver = {
+		.name = "qcom_qseecom_uefisecapp",
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+};
+module_auxiliary_driver(qcom_uefisecapp_driver);
+
+MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>");
+MODULE_DESCRIPTION("Client driver for Qualcomm SEE UEFI Secure App");
+MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 1fa846d48795..92fad47635ae 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1693,7 +1693,9 @@ static bool qcom_scm_qseecom_device_allowed(void)
 	return match;
 }
 
-static const struct qseecom_app_desc qcom_scm_qseecom_apps[] = {};
+static const struct qseecom_app_desc qcom_scm_qseecom_apps[] = {
+	{ "qcom.tz.uefisecapp", "uefisecapp" },
+};
 
 static int qcom_scm_qseecom_init(void)
 {
-- 
2.40.1

Re: [PATCH v4 1/4] lib/ucs2_string: Add UCS-2 strlcpy function

[Kees Cook]

On Mon, May 29, 2023 at 01:03:48AM +0200, Maximilian Luz wrote:
> Add a ucs2_strlcpy() function for UCS-2 strings. The behavior is
> equivalent to the standard strlcpy() function, just for 16-bit character
> UCS-2 strings.

Eek, no. strlcpy() is dangerous in multiple ways[1]. Please implement
strscpy() (i.e. use strnlen(), negative error on truncation, etc).
Additionally, it'd be nice of the ucs2 helpers here also implemented the
rest of the CONFIG_FORTIFY_SOURCE mitigations (i.e. checking for source
and destination buffer size overflows at compile-time and run-time with
__builtin_object_size() and __builtin_dynamoc_object_size() respectively).

-Kees

[1] https://docs.kernel.org/process/deprecated.html#strlcpy

-- 
Kees Cook

Re: [PATCH v4 1/4] lib/ucs2_string: Add UCS-2 strlcpy function

[Maximilian Luz]

On 5/30/23 17:25, Kees Cook wrote:
> On Mon, May 29, 2023 at 01:03:48AM +0200, Maximilian Luz wrote:
>> Add a ucs2_strlcpy() function for UCS-2 strings. The behavior is
>> equivalent to the standard strlcpy() function, just for 16-bit character
>> UCS-2 strings.
> 
> Eek, no. strlcpy() is dangerous in multiple ways[1]. Please implement
> strscpy() (i.e. use strnlen(), negative error on truncation, etc).

Right, make sense, thanks. Somehow I missed that the kernel has a better
function than the C stdlib for that...

> Additionally, it'd be nice of the ucs2 helpers here also implemented the
> rest of the CONFIG_FORTIFY_SOURCE mitigations (i.e. checking for source
> and destination buffer size overflows at compile-time and run-time with
> __builtin_object_size() and __builtin_dynamoc_object_size() respectively).

I can certainly try that, but I think this might be better suited for a
follow-up series, given that we then should also add those to the other
helpers.

Regards,
Max

Re: [PATCH v4 1/4] lib/ucs2_string: Add UCS-2 strlcpy function

[Ard Biesheuvel]

On Tue, 30 May 2023 at 18:15, Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> On 5/30/23 17:25, Kees Cook wrote:
> > On Mon, May 29, 2023 at 01:03:48AM +0200, Maximilian Luz wrote:
> >> Add a ucs2_strlcpy() function for UCS-2 strings. The behavior is
> >> equivalent to the standard strlcpy() function, just for 16-bit character
> >> UCS-2 strings.
> >
> > Eek, no. strlcpy() is dangerous in multiple ways[1]. Please implement
> > strscpy() (i.e. use strnlen(), negative error on truncation, etc).
>
> Right, make sense, thanks. Somehow I missed that the kernel has a better
> function than the C stdlib for that...
>
> > Additionally, it'd be nice of the ucs2 helpers here also implemented the
> > rest of the CONFIG_FORTIFY_SOURCE mitigations (i.e. checking for source
> > and destination buffer size overflows at compile-time and run-time with
> > __builtin_object_size() and __builtin_dynamoc_object_size() respectively).
>
> I can certainly try that, but I think this might be better suited for a
> follow-up series, given that we then should also add those to the other
> helpers.
>

Agreed. Let's log the followup work as a kspp work item, no need to
make that part of this series.

Thanks,

Re: [PATCH v4 1/4] lib/ucs2_string: Add UCS-2 strlcpy function

[Kees Cook]

On Tue, May 30, 2023 at 06:17:35PM +0200, Ard Biesheuvel wrote:
> On Tue, 30 May 2023 at 18:15, Maximilian Luz <luzmaximilian@gmail.com> wrote:
> >
> > On 5/30/23 17:25, Kees Cook wrote:
> > > On Mon, May 29, 2023 at 01:03:48AM +0200, Maximilian Luz wrote:
> > >> Add a ucs2_strlcpy() function for UCS-2 strings. The behavior is
> > >> equivalent to the standard strlcpy() function, just for 16-bit character
> > >> UCS-2 strings.
> > >
> > > Eek, no. strlcpy() is dangerous in multiple ways[1]. Please implement
> > > strscpy() (i.e. use strnlen(), negative error on truncation, etc).
> >
> > Right, make sense, thanks. Somehow I missed that the kernel has a better
> > function than the C stdlib for that...
> >
> > > Additionally, it'd be nice of the ucs2 helpers here also implemented the
> > > rest of the CONFIG_FORTIFY_SOURCE mitigations (i.e. checking for source
> > > and destination buffer size overflows at compile-time and run-time with
> > > __builtin_object_size() and __builtin_dynamoc_object_size() respectively).
> >
> > I can certainly try that, but I think this might be better suited for a
> > follow-up series, given that we then should also add those to the other
> > helpers.
> >
> 
> Agreed. Let's log the followup work as a kspp work item, no need to
> make that part of this series.

Yeah, that's fine. Can you please open a KSSP issue for it so we don't
forget?  :)

-- 
Kees Cook

Re: [PATCH v4 2/4] firmware: qcom_scm: Clear scm pointer on probe failure

[Johan Hovold]

On Mon, May 29, 2023 at 01:03:49AM +0200, Maximilian Luz wrote:
> When setting-up the IRQ goes wrong, the __scm pointer currently remains
> set even though we fail to probe the driver successfully. Due to this,
> access to __scm may go wrong since associated resources (clocks, ...)
> have been released. Therefore, clear the __scm pointer when setting-up
> the IRQ fails.
> 
> Fixes: 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling logic")
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---
> 
> Patch introduced in v4
> 
> ---
>  drivers/firmware/qcom_scm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index fde33acd46b7..d0070b833889 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -1488,8 +1488,10 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	} else {
>  		ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
>  						IRQF_ONESHOT, "qcom-scm", __scm);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			__scm = NULL;

This looks fragile at best. Clients use qcom_scm_is_available() to see
if __scm is available and do not expect it to go away once it is live.

It looks like you can hold off on initialising __scm until you've
requested the interrupt, either by using IRQ_NOAUTOEN or fixing
qcom_scm_waitq_wakeup() so that it doesn't use __scm directly.

That would also take care of the previous branch which may also leave
__scm set after the structure itself has been released on errors.

You'll have similar problems when registering qseecom which currently
depend on __scm being set, though. Clearing the pointer in that case is
clearly broken as you currently rely on devres for deregistering the aux
clients on errors (i.e. the clients using __scm are still registered
when you clear the pointer in patch 3/4).

>  			return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n");
> +		}
>  	}
>  
>  	__get_convention();

Johan

Re: [PATCH v4 3/4] firmware: qcom_scm: Add support for Qualcomm Secure Execution Environment SCM interface

[Johan Hovold]

On Mon, May 29, 2023 at 01:03:50AM +0200, Maximilian Luz wrote:
> Add support for SCM calls to Secure OS and the Secure Execution
> Environment (SEE) residing in the TrustZone (TZ) via the QSEECOM
> interface. This allows communication with Secure/TZ applications, for
> example 'uefisecapp' managing access to UEFI variables.
> 
> The added interface attempts to automatically detect known and supported
> applications, creating a client (auxiliary) device for each one. The
> respective client/auxiliary driver is then responsible for managing and
> communicating with the application.
> 
> While this patch introduces only a very basic interface without the more
> advanced features (such as re-entrant and blocking SCM calls and
> listeners/callbacks), this is enough to talk to the aforementioned
> 'uefisecapp'.
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---
> 
> Changes in v4:
>  - Remove instantiation of dedicated QSEECOM device and load the driver
>    via qcom_scm instead. In particular:
>    - Add a list of tested devices to ensure that we don't run into any
>      issues with the currently unimplemented re-entrant calls.
>    - Use the QSEECOM version to check for general availability of the
>      interface.
>    - Attempt to automatically detect available QSEECOM applications
>      (and instantiate respective clients) based on a fixed list.
>  - Use auxiliary bus and devices for clients instead of MFD.
>  - Restructure DMA allocations: Use dma_map_single() directly inside 
>    qcom_scm_qseecom_app_send() instead of requiring clients to allocate
>    DMA memory themselves.
 
> +#ifdef CONFIG_QCOM_SCM_QSEECOM
> +
> +/* Lock for QSEECOM SCM call executions */
> +DEFINE_MUTEX(qcom_scm_qseecom_call_lock);

Missing static keyword.

> +/**
> + * qcom_scm_qseecom_call() - Perform a QSEECOM SCM call.
> + * @desc: SCM call descriptor.
> + * @res:  SCM call response (output).
> + *
> + * Performs the QSEECOM SCM call described by @desc, returning the response in
> + * @rsp.
> + *
> + * Return: Returns zero on success, nonzero on failure.

Nit: You should drop "Returns" here and capitalise Zero. Similar below.

> + */

> +/**
> + * qcom_scm_qseecom_get_version() - Query the QSEECOM version.
> + * @version: Pointer where the QSEECOM version will be stored.
> + *
> + * Performs the QSEECOM SCM querying the QSEECOM version currently running in
> + * the TrustZone.
> + *
> + * Return: Returns zero on success, nonzero on failure.
> + */
> +static int qcom_scm_qseecom_get_version(u32 *version)
> +{
> +	struct qcom_scm_desc desc = {};
> +	struct qcom_scm_qseecom_resp res = {};
> +	u32 feature = 10;
> +	int ret;
> +
> +	desc.owner = QSEECOM_TZ_OWNER_SIP;
> +	desc.svc = QSEECOM_TZ_SVC_INFO;
> +	desc.cmd = 0x03;

I know this has been reverse engineered, but is it possible to name also
these cmd codes?

> +	desc.arginfo = QCOM_SCM_ARGS(1, QCOM_SCM_VAL);
> +	desc.args[0] = feature;
> +
> +	ret = qcom_scm_qseecom_call(&desc, &res);
> +	if (ret)
> +		return ret;
> +
> +	*version = res.result;
> +	return 0;
> +}
> +
> +/**
> + * qcom_scm_qseecom_app_get_id() - Query the app ID for a given QSEE app name.
> + * @app_name: The name of the app.
> + * @app_id:   The returned app ID.
> + *
> + * Query and return the application ID of the SEE app identified by the given
> + * name. This returned ID is the unique identifier of the app required for
> + * subsequent communication.
> + *
> + * Return: Returns zero on success, nonzero on failure. Returns -ENOENT if the
> + * app has not been loaded or could not be found.
> + */
> +static int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id)
> +{
> +	unsigned long name_buf_size = QSEECOM_MAX_APP_NAME_SIZE;
> +	unsigned long app_name_len = strlen(app_name);
> +	struct qcom_scm_desc desc = {};
> +	struct qcom_scm_qseecom_resp res = {};
> +	dma_addr_t name_buf_phys;
> +	char *name_buf;
> +	int status;
> +
> +	if (app_name_len >= name_buf_size)
> +		return -EINVAL;
> +
> +	name_buf = kzalloc(name_buf_size, GFP_KERNEL);
> +	if (!name_buf)
> +		return -ENOMEM;
> +
> +	memcpy(name_buf, app_name, app_name_len);
> +
> +	name_buf_phys = dma_map_single(__scm->dev, name_buf, name_buf_size, DMA_TO_DEVICE);
> +	if (dma_mapping_error(__scm->dev, name_buf_phys)) {
> +		kfree(name_buf);
> +		dev_err(__scm->dev, "qseecom: failed to map dma address\n");
> +		return -EFAULT;

This should be -ENOMEM (you can also just use the return value from
dma_mapping_error()). Similar below.

> +	}
> +
> +	desc.owner = QSEECOM_TZ_OWNER_QSEE_OS;
> +	desc.svc = QSEECOM_TZ_SVC_APP_MGR;
> +	desc.cmd = 0x03;
> +	desc.arginfo = QCOM_SCM_ARGS(2, QCOM_SCM_RW, QCOM_SCM_VAL);
> +	desc.args[0] = name_buf_phys;
> +	desc.args[1] = app_name_len;
> +
> +	status = qcom_scm_qseecom_call(&desc, &res);
> +	dma_unmap_single(__scm->dev, name_buf_phys, name_buf_size, DMA_TO_DEVICE);
> +	kfree(name_buf);
> +
> +	if (status)
> +		return status;
> +
> +	if (res.result == QSEECOM_RESULT_FAILURE)
> +		return -ENOENT;
> +
> +	if (res.result != QSEECOM_RESULT_SUCCESS)
> +		return -EINVAL;
> +
> +	if (res.resp_type != QSEECOM_SCM_RES_APP_ID)
> +		return -EINVAL;
> +
> +	*app_id = res.data;
> +	return 0;
> +}
> +
> +/**
> + * qcom_scm_qseecom_app_send() - Send to and receive data from a given QSEE app.
> + * @client:   The QSEECOM client device corresponding to the target app.
> + * @req:      Request buffer sent to the app (must be DMA-mappable).
> + * @req_size: Size of the request buffer.
> + * @rsp:      Response buffer, written to by the app (must be DMA-mappable).
> + * @rsp_size: Size of the response buffer.
> + *
> + * Sends a request to the QSEE app associated with the given client and read
> + * back its response. The caller must provide two DMA memory regions, one for
> + * the request and one for the response, and fill out the @req region with the
> + * respective (app-specific) request data. The QSEE app reads this and returns
> + * its response in the @rsp region.
> + *
> + * Return: Returns zero on success, nonzero error code on failure.
> + */
> +int qcom_scm_qseecom_app_send(struct qseecom_client *client, void *req,
> +			      size_t req_size, void *rsp, size_t rsp_size)
> +{

> +}
> +EXPORT_SYMBOL(qcom_scm_qseecom_app_send);

Should this not be EXPORT_SYMBOL_GPL()?

> +
> +static void qseecom_client_release(struct device *dev)
> +{
> +	struct qseecom_client *client = container_of(dev, struct qseecom_client, aux_dev.dev);
> +
> +	kfree(client);
> +}
> +
> +static void qseecom_client_remove(void *data)
> +{
> +	struct qseecom_client *client = data;
> +
> +	auxiliary_device_delete(&client->aux_dev);
> +	auxiliary_device_uninit(&client->aux_dev);
> +}
> +
> +static int qseecom_client_register(const struct qseecom_app_desc *desc)
> +{
> +	struct qseecom_client *client;
> +	u32 app_id;
> +	int ret;
> +
> +	/* Try to find the app ID, skip device if not found */
> +	ret = qcom_scm_qseecom_app_get_id(desc->app_name, &app_id);
> +	if (ret)
> +		return ret == -ENOENT ? 0 : ret;
> +
> +	dev_info(__scm->dev, "qseecom: setting up client for %s\n", desc->app_name);
> +
> +	/* Allocate and set-up the client device */
> +	client = kzalloc(sizeof(*client), GFP_KERNEL);
> +	if (!client)
> +		return -ENOMEM;
> +
> +	client->aux_dev.name = desc->dev_name;
> +	client->aux_dev.dev.parent = __scm->dev;
> +	client->aux_dev.dev.release = qseecom_client_release;
> +	client->app_id = app_id;
> +
> +	ret = auxiliary_device_init(&client->aux_dev);
> +	if (ret) {
> +		kfree(client);
> +		return ret;
> +	}
> +
> +	ret = auxiliary_device_add(&client->aux_dev);
> +	if (ret) {
> +		auxiliary_device_uninit(&client->aux_dev);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Ensure that the device is properly cleaned up in case of removal or
> +	 * errors.
> +	 */
> +	ret = devm_add_action_or_reset(__scm->dev, qseecom_client_remove, client);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +/*
> + * We do not yet support re-entrant calls via the qseecom interface. To prevent
> + + any potential issues with this, only allow validated devices for now.
> + */
> +static const struct of_device_id qcom_scm_qseecom_allowlist[] = {
> +	{ .compatible = "lenovo,thinkpad-x13s", },
> +	{ }
> +};
> +
> +static bool qcom_scm_qseecom_device_allowed(void)
> +{
> +	struct device_node *np;
> +	bool match;
> +
> +	np = of_find_node_by_path("/");
> +	match = of_match_node(qcom_scm_qseecom_allowlist, np);
> +	of_node_put(np);
> +
> +	return match;
> +}
> +
> +static const struct qseecom_app_desc qcom_scm_qseecom_apps[] = {};
> +
> +static int qcom_scm_qseecom_init(void)
> +{
> +	u32 version;
> +	int ret, i;
> +
> +	/* Try to query the qseecom version, skip qseecom setup if this fails */
> +	ret = qcom_scm_qseecom_get_version(&version);
> +	if (ret)
> +		return 0;
> +
> +	dev_info(__scm->dev, "qseecom: found qseecom with version 0x%x\n", version);
> +
> +	/* Check against tested/verified devices */
> +	if (!qcom_scm_qseecom_device_allowed()) {
> +		dev_info(__scm->dev, "qseecom: untested device, skipping\n");

Perhaps call the helper machine_allowed() and update the commit message
to match (cf. of_machine_is_compatible())?

> +		return 0;
> +	}
> +
> +	/* Set up client devices for each base application */
> +	for (i = 0; i < ARRAY_SIZE(qcom_scm_qseecom_apps); i++) {
> +		ret = qseecom_client_register(&qcom_scm_qseecom_apps[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +#else /* CONFIG_QCOM_SCM_QSEECOM */
> +
> +static int qcom_scm_qseecom_init(void)
> +{
> +	return 0;
> +}
> +
> +#endif /* CONFIG_QCOM_SCM_QSEECOM */
> +
>  /**
>   * qcom_scm_is_available() - Checks if SCM is available
>   */
> @@ -1496,6 +1903,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  
>  	__get_convention();
>  
> +	ret = qcom_scm_qseecom_init();
> +	if (ret < 0) {
> +		__scm = NULL;

So as I mentioned in my reply to 2/4, you can still have clients
registered here when you clear the __scm pointer which they rely on
after an error.

Not sure how best to handle this, but perhaps registering a qseecom
platform device here and have it's driver probe defer until scm is
available would work?

That way you could also separate out the qseecom implementation in a
separate file (driver) rather than having the ifdef above.

> +		return dev_err_probe(scm->dev, ret, "Failed to initialize qseecom\n");
> +	}
> +
>  	/*
>  	 * If requested enable "download mode", from this point on warmboot
>  	 * will cause the boot stages to enter download mode, unless

Overall this looks really good. Nice job!

Johan

Re: [PATCH v4 3/4] firmware: qcom_scm: Add support for Qualcomm Secure Execution Environment SCM interface

[Johan Hovold]

On Wed, Jun 28, 2023 at 02:11:07PM +0200, Johan Hovold wrote:
> On Mon, May 29, 2023 at 01:03:50AM +0200, Maximilian Luz wrote:

> > @@ -1496,6 +1903,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
> >  
> >  	__get_convention();
> >  
> > +	ret = qcom_scm_qseecom_init();
> > +	if (ret < 0) {
> > +		__scm = NULL;
> 
> So as I mentioned in my reply to 2/4, you can still have clients
> registered here when you clear the __scm pointer which they rely on
> after an error.
> 
> Not sure how best to handle this, but perhaps registering a qseecom
> platform device here and have it's driver probe defer until scm is
> available would work?
> 
> That way you could also separate out the qseecom implementation in a
> separate file (driver) rather than having the ifdef above.

An alternative may be to just warn and continue if
qcom_scm_qseecom_init() fails. It should never return -EPROBE_DEFER
anyway, right?

Johan

Re: [PATCH v4 4/4] firmware: Add support for Qualcomm UEFI Secure Application

[Johan Hovold]

On Mon, May 29, 2023 at 01:03:51AM +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 Qualcomm QSEECOM / 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@gmail.com>
> ---
> 
> Changes in v4:
>  - Adapt to changes in DMA allocation in patch 3.
>  - Rework alignment: Use native alignment of types instead of a general
>    8 byte alignment. While the windows driver uses 8 byte alignment for
>    GUIDs, the native (4 byte) alignment seems to work fine here.
>  - Add a helper macro for determining size and layout of request and
>    response buffers, taking care of proper alignment.
>  - Implement support for EFI's query_variable_info() call, which is now
>    supported by the kernel (and expected by efivarfs).
>  - Move UCS-2 string helpers to lib/ucs2_string.c

> +config QCOM_QSEECOM_UEFISECAPP
> +	tristate "Qualcomm SEE UEFI Secure App client driver"
> +	select QCOM_SCM
> +	select QCOM_SCM_QSEECOM

This should just be

	depends on QCOM_SCM_QSEECOM

Using select should generally be avoided.

> +	depends on EFI
> +	help
> +	  Various Qualcomm SoCs do not allow direct access to EFI variables.
> +	  Instead, these need to be accessed via the UEFI Secure Application
> +	  (uefisecapp), residing in the Secure Execution Environment (SEE).
> +
> +	  This module provides a client driver for uefisecapp, installing efivar
> +	  operations to allow the kernel accessing EFI variables, and via that also
> +	  provide user-space with access to EFI variables via efivarfs.
> +
> +	  Select M or Y here to provide access to EFI variables on the
> +	  aforementioned platforms.

We still have not really sorted how best to handle modular efivars
implementations as userspace may try to mount efivarfs before the module
has been loaded...

Perhaps require it to be built-in for now?

> +
>  config SYSFB
>  	bool
>  	select BOOT_VESA_SUPPORT

> +/* -- Alighment helpers ----------------------------------------------------- */

typo: Alignment

> +
> +/*
> + * Helper macro to ensure proper alignment of types (fields and arrays) when
> + * stored in some (contiguous) buffer.
> + *
> + * Note: The driver from which this one has been reverse-engineered expects an
> + * alignment of 8 bytes (64 bits) for GUIDs. Our definition of efi_guid_t,
> + * however, has an alignment of 4 byte (32 bits). So far, this seems to work
> + * fine here. See also the comment on the typedef of efi_guid_t.
> + */
> +#define qcuefi_buf_align_fields(fields...)					\
> +	({									\
> +		size_t __len = 0;						\
> +		fields								\
> +		__len;								\
> +	})
> +
> +#define __field_impl(size, align, offset)					\
> +	({									\
> +		size_t *__offset = (offset);					\
> +		size_t __aligned;						\
> +										\
> +		__aligned = ALIGN(__len, align);				\
> +		__len = __aligned + (size);					\
> +										\
> +		if (__offset)							\
> +			*__offset = __aligned;					\
> +	});
> +
> +#define __array_offs(type, count, offset)					\
> +	__field_impl(sizeof(type) * (count), __alignof__(type), offset)
> +
> +#define __array(type, count)		__array_offs(type, count, NULL)
> +#define __field_offs(type, offset)	__array_offs(type, 1, offset)
> +#define __field(type)			__array_offs(type, 1, NULL)

Heh. This is some nice macro magic. :)

> +static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const efi_char16_t *name,
> +					   const efi_guid_t *guid, u32 *attributes,
> +					   unsigned long *data_size, void *data)
> +{
> +	struct qsee_req_uefi_get_variable *req_data;
> +	struct qsee_rsp_uefi_get_variable *rsp_data;
> +	unsigned long buffer_size = *data_size;
> +	efi_status_t efi_status = EFI_SUCCESS;
> +	unsigned long name_length;
> +	size_t guid_offs;
> +	size_t name_offs;
> +	size_t req_size;
> +	size_t rsp_size;
> +	int status;
> +
> +	/* Validation: We need a name and GUID. */
> +	if (!name || !guid)
> +		return EFI_INVALID_PARAMETER;
> +
> +	/* Get length of name and ensure that the name is not truncated. */
> +	name_length = ucs2_strnlen(name, QSEE_MAX_NAME_LEN) + 1;
> +	if (name_length > QSEE_MAX_NAME_LEN)
> +		return EFI_INVALID_PARAMETER;
> +
> +	/* Validation: We need a buffer if the buffer_size is nonzero. */
> +	if (buffer_size && !data)
> +		return EFI_INVALID_PARAMETER;
> +
> +	/* Compute required buffer size for request. */
> +	req_size = qcuefi_buf_align_fields(
> +		__field(*req_data)
> +		__array_offs(*name, name_length, &name_offs)
> +		__field_offs(*guid, &guid_offs)
> +	);
> +
> +	/* Compute required buffer size for response. */
> +	rsp_size = qcuefi_buf_align_fields(
> +		__field(*rsp_data)
> +		__array(u8, buffer_size)
> +	);
> +
> +	/* Allocate request buffer. */
> +	req_data = kzalloc(req_size, GFP_KERNEL);
> +	if (!req_data) {
> +		efi_status = EFI_OUT_OF_RESOURCES;
> +		goto out;
> +	}
> +
> +	/* Allocate response buffer. */
> +	rsp_data = kzalloc(rsp_size, GFP_KERNEL);
> +	if (!rsp_data) {
> +		efi_status = EFI_OUT_OF_RESOURCES;
> +		goto out_free_req;
> +	}
> +
> +	/* Set up request data. */
> +	req_data->command_id = QSEE_CMD_UEFI_GET_VARIABLE;
> +	req_data->data_size = buffer_size;
> +	req_data->name_offset = name_offs;
> +	req_data->name_size = name_length * sizeof(*name);
> +	req_data->guid_offset = guid_offs;
> +	req_data->guid_size = sizeof(*guid);
> +	req_data->length = req_size;
> +
> +	/* Copy request parameters. */
> +	ucs2_strlcpy(((void *)req_data) + req_data->name_offset, name, name_length);
> +	memcpy(((void *)req_data) + req_data->guid_offset, guid, req_data->guid_size);
> +
> +	/* Perform SCM call. */
> +	status = qcom_scm_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size);
> +
> +	/* Check for errors and validate. */
> +	if (status) {
> +		efi_status = EFI_DEVICE_ERROR;
> +		goto out_free;
> +	}
> +
> +	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_VARIABLE) {
> +		efi_status = EFI_DEVICE_ERROR;
> +		goto out_free;
> +	}
> +
> +	if (rsp_data->length < sizeof(*rsp_data)) {
> +		efi_status = EFI_DEVICE_ERROR;
> +		goto out_free;
> +	}
> +
> +	if (rsp_data->status) {
> +		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
> +			__func__, rsp_data->status);
> +		efi_status = qsee_uefi_status_to_efi(rsp_data->status);
> +
> +		/* Update size and attributes in case buffer is too small. */
> +		if (efi_status == EFI_BUFFER_TOO_SMALL) {
> +			*data_size = rsp_data->data_size;
> +			if (attributes)
> +				*attributes = rsp_data->attributes;
> +		}
> +
> +		goto out_free;
> +	}
> +
> +	if (rsp_data->length > rsp_size) {
> +		efi_status = EFI_DEVICE_ERROR;
> +		goto out_free;
> +	}
> +
> +	if (rsp_data->data_offset + rsp_data->data_size > rsp_data->length) {
> +		efi_status = EFI_DEVICE_ERROR;
> +		goto out_free;
> +	}
> +
> +	/* Set attributes and data size even if buffer is too small. */
> +	*data_size = rsp_data->data_size;
> +	if (attributes)
> +		*attributes = rsp_data->attributes;
> +
> +	/*
> +	 * If we have a buffer size of zero and no buffer, just return
> +	 * attributes and required size.
> +	 */
> +	if (buffer_size == 0 && !data) {
> +		efi_status = EFI_SUCCESS;
> +		goto out_free;
> +	}
> +
> +	/* Validate output buffer size. */
> +	if (buffer_size < rsp_data->data_size) {
> +		efi_status = EFI_BUFFER_TOO_SMALL;
> +		goto out_free;
> +	}
> +
> +	/* Copy to output buffer. Note: We're guaranteed to have one at this point. */
> +	memcpy(data, ((void *)rsp_data) + rsp_data->data_offset, rsp_data->data_size);
> +
> +out_free:
> +	kfree(rsp_data);
> +out_free_req:
> +	kfree(req_data);
> +out:
> +	return efi_status;
> +}

The above reads very easily as it stands, but generally we try to avoid
adding comments inside functions unless doing something unexpected or
similar.

Comments like "Allocate response buffer" and "Perform SCM call" should
not be needed as it should be clear from the code (given apt names of
variables and function which you already seem to have chosen).

> +/* -- Global efivar interface. ---------------------------------------------- */
> +
> +static struct qcuefi_client *__qcuefi;
> +static DEFINE_MUTEX(__qcuefi_lock);
> +
> +static int qcuefi_set_reference(struct qcuefi_client *qcuefi)
> +{
> +	mutex_lock(&__qcuefi_lock);
> +
> +	if (qcuefi && __qcuefi) {
> +		mutex_unlock(&__qcuefi_lock);
> +		return -EEXIST;
> +	}
> +
> +	__qcuefi = qcuefi;
> +
> +	mutex_unlock(&__qcuefi_lock);
> +	return 0;
> +}
> +
> +static struct qcuefi_client *qcuefi_acquire(void)
> +	__acquires(&__qcuefi_lock)

I noticed that someone told you to add sparse annotation here but that
was not correct.

The mutex implementation does not use sparse annotation so this actually
introduces sparse warnings such as:

/home/johan/work/linaro/src/linux/drivers/firmware/qcom_qseecom_uefisecapp.c:741:29: warning: context imbalance in 'qcuefi_acquire' - wrong count at exit

Just drop the annotation again.

> +{
> +	mutex_lock(&__qcuefi_lock);
> +	return __qcuefi;
> +}
> +
> +static void qcuefi_release(void)
> +	__releases(&__qcuefi_lock)
> +{
> +	mutex_unlock(&__qcuefi_lock);
> +}

> +static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
> +				 const struct auxiliary_device_id *aux_dev_id)
> +{
> +	struct qcuefi_client *qcuefi;
> +	int status;
> +
> +	/* Allocate driver data. */

As mentioned above, I suggest dropping comments like this throughout.

> +	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);
> +
> +	/* Register global reference. */
> +	auxiliary_set_drvdata(aux_dev, qcuefi);
> +	status = qcuefi_set_reference(qcuefi);
> +	if (status)
> +		return status;
> +
> +	/* Register efivar ops. */
> +	status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops);
> +	if (status)
> +		qcuefi_set_reference(NULL);
> +
> +	return status;
> +}

Johan

Re: [PATCH v4 2/4] firmware: qcom_scm: Clear scm pointer on probe failure

[Maximilian Luz]

First off, sorry again for the long delay and thanks for being patient
with me (and for the review of course). I'm finally getting back to
finding some time for Linux things again, so I think I've mostly settled
in by now.

On 6/28/23 13:20, Johan Hovold wrote:
> On Mon, May 29, 2023 at 01:03:49AM +0200, Maximilian Luz wrote:
>> When setting-up the IRQ goes wrong, the __scm pointer currently remains
>> set even though we fail to probe the driver successfully. Due to this,
>> access to __scm may go wrong since associated resources (clocks, ...)
>> have been released. Therefore, clear the __scm pointer when setting-up
>> the IRQ fails.
>>
>> Fixes: 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling logic")
>> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
>> ---
>>
>> Patch introduced in v4
>>
>> ---
>>   drivers/firmware/qcom_scm.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index fde33acd46b7..d0070b833889 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -1488,8 +1488,10 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>   	} else {
>>   		ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
>>   						IRQF_ONESHOT, "qcom-scm", __scm);
>> -		if (ret < 0)
>> +		if (ret < 0) {
>> +			__scm = NULL;
> 
> This looks fragile at best. Clients use qcom_scm_is_available() to see
> if __scm is available and do not expect it to go away once it is live.

Hmm, you're right. The whole situation is probably not ideal and that
fix is really just a bad band-aid.

> It looks like you can hold off on initialising __scm until you've
> requested the interrupt, either by using IRQ_NOAUTOEN or fixing
> qcom_scm_waitq_wakeup() so that it doesn't use __scm directly.
> 
> That would also take care of the previous branch which may also leave
> __scm set after the structure itself has been released on errors.

Agreed.

> You'll have similar problems when registering qseecom which currently
> depend on __scm being set, though. Clearing the pointer in that case is
> clearly broken as you currently rely on devres for deregistering the aux
> clients on errors (i.e. the clients using __scm are still registered
> when you clear the pointer in patch 3/4).

Oh right, I hadn't thought of that. I'll have to rework that.

>>   			return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n");
>> +		}
>>   	}
>>   
>>   	__get_convention();
> 
> Johan

Re: [PATCH v4 3/4] firmware: qcom_scm: Add support for Qualcomm Secure Execution Environment SCM interface

[Maximilian Luz]

On 6/28/23 14:11, Johan Hovold wrote:
> On Mon, May 29, 2023 at 01:03:50AM +0200, Maximilian Luz wrote:
>> Add support for SCM calls to Secure OS and the Secure Execution
>> Environment (SEE) residing in the TrustZone (TZ) via the QSEECOM
>> interface. This allows communication with Secure/TZ applications, for
>> example 'uefisecapp' managing access to UEFI variables.
>>
>> The added interface attempts to automatically detect known and supported
>> applications, creating a client (auxiliary) device for each one. The
>> respective client/auxiliary driver is then responsible for managing and
>> communicating with the application.
>>
>> While this patch introduces only a very basic interface without the more
>> advanced features (such as re-entrant and blocking SCM calls and
>> listeners/callbacks), this is enough to talk to the aforementioned
>> 'uefisecapp'.
>>
>> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
>> ---
>>
>> Changes in v4:
>>   - Remove instantiation of dedicated QSEECOM device and load the driver
>>     via qcom_scm instead. In particular:
>>     - Add a list of tested devices to ensure that we don't run into any
>>       issues with the currently unimplemented re-entrant calls.
>>     - Use the QSEECOM version to check for general availability of the
>>       interface.
>>     - Attempt to automatically detect available QSEECOM applications
>>       (and instantiate respective clients) based on a fixed list.
>>   - Use auxiliary bus and devices for clients instead of MFD.
>>   - Restructure DMA allocations: Use dma_map_single() directly inside
>>     qcom_scm_qseecom_app_send() instead of requiring clients to allocate
>>     DMA memory themselves.
>   
>> +#ifdef CONFIG_QCOM_SCM_QSEECOM
>> +
>> +/* Lock for QSEECOM SCM call executions */
>> +DEFINE_MUTEX(qcom_scm_qseecom_call_lock);
> 
> Missing static keyword.

Right.
  
>> +/**
>> + * qcom_scm_qseecom_call() - Perform a QSEECOM SCM call.
>> + * @desc: SCM call descriptor.
>> + * @res:  SCM call response (output).
>> + *
>> + * Performs the QSEECOM SCM call described by @desc, returning the response in
>> + * @rsp.
>> + *
>> + * Return: Returns zero on success, nonzero on failure.
> 
> Nit: You should drop "Returns" here and capitalise Zero. Similar below.

Okay, will do.
  
>> + */
> 
>> +/**
>> + * qcom_scm_qseecom_get_version() - Query the QSEECOM version.
>> + * @version: Pointer where the QSEECOM version will be stored.
>> + *
>> + * Performs the QSEECOM SCM querying the QSEECOM version currently running in
>> + * the TrustZone.
>> + *
>> + * Return: Returns zero on success, nonzero on failure.
>> + */
>> +static int qcom_scm_qseecom_get_version(u32 *version)
>> +{
>> +	struct qcom_scm_desc desc = {};
>> +	struct qcom_scm_qseecom_resp res = {};
>> +	u32 feature = 10;
>> +	int ret;
>> +
>> +	desc.owner = QSEECOM_TZ_OWNER_SIP;
>> +	desc.svc = QSEECOM_TZ_SVC_INFO;
>> +	desc.cmd = 0x03;
> 
> I know this has been reverse engineered, but is it possible to name also
> these cmd codes?

Sure, I can try.

>> +	desc.arginfo = QCOM_SCM_ARGS(1, QCOM_SCM_VAL);
>> +	desc.args[0] = feature;
>> +
>> +	ret = qcom_scm_qseecom_call(&desc, &res);
>> +	if (ret)
>> +		return ret;
>> +
>> +	*version = res.result;
>> +	return 0;
>> +}
>> +
>> +/**
>> + * qcom_scm_qseecom_app_get_id() - Query the app ID for a given QSEE app name.
>> + * @app_name: The name of the app.
>> + * @app_id:   The returned app ID.
>> + *
>> + * Query and return the application ID of the SEE app identified by the given
>> + * name. This returned ID is the unique identifier of the app required for
>> + * subsequent communication.
>> + *
>> + * Return: Returns zero on success, nonzero on failure. Returns -ENOENT if the
>> + * app has not been loaded or could not be found.
>> + */
>> +static int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id)
>> +{
>> +	unsigned long name_buf_size = QSEECOM_MAX_APP_NAME_SIZE;
>> +	unsigned long app_name_len = strlen(app_name);
>> +	struct qcom_scm_desc desc = {};
>> +	struct qcom_scm_qseecom_resp res = {};
>> +	dma_addr_t name_buf_phys;
>> +	char *name_buf;
>> +	int status;
>> +
>> +	if (app_name_len >= name_buf_size)
>> +		return -EINVAL;
>> +
>> +	name_buf = kzalloc(name_buf_size, GFP_KERNEL);
>> +	if (!name_buf)
>> +		return -ENOMEM;
>> +
>> +	memcpy(name_buf, app_name, app_name_len);
>> +
>> +	name_buf_phys = dma_map_single(__scm->dev, name_buf, name_buf_size, DMA_TO_DEVICE);
>> +	if (dma_mapping_error(__scm->dev, name_buf_phys)) {
>> +		kfree(name_buf);
>> +		dev_err(__scm->dev, "qseecom: failed to map dma address\n");
>> +		return -EFAULT;
> 
> This should be -ENOMEM (you can also just use the return value from
> dma_mapping_error()). Similar below.

Somehow I missed that dma_mapping_error() returned an actuall error
code. Will use that.

>> +	}
>> +
>> +	desc.owner = QSEECOM_TZ_OWNER_QSEE_OS;
>> +	desc.svc = QSEECOM_TZ_SVC_APP_MGR;
>> +	desc.cmd = 0x03;
>> +	desc.arginfo = QCOM_SCM_ARGS(2, QCOM_SCM_RW, QCOM_SCM_VAL);
>> +	desc.args[0] = name_buf_phys;
>> +	desc.args[1] = app_name_len;
>> +
>> +	status = qcom_scm_qseecom_call(&desc, &res);
>> +	dma_unmap_single(__scm->dev, name_buf_phys, name_buf_size, DMA_TO_DEVICE);
>> +	kfree(name_buf);
>> +
>> +	if (status)
>> +		return status;
>> +
>> +	if (res.result == QSEECOM_RESULT_FAILURE)
>> +		return -ENOENT;
>> +
>> +	if (res.result != QSEECOM_RESULT_SUCCESS)
>> +		return -EINVAL;
>> +
>> +	if (res.resp_type != QSEECOM_SCM_RES_APP_ID)
>> +		return -EINVAL;
>> +
>> +	*app_id = res.data;
>> +	return 0;
>> +}
>> +
>> +/**
>> + * qcom_scm_qseecom_app_send() - Send to and receive data from a given QSEE app.
>> + * @client:   The QSEECOM client device corresponding to the target app.
>> + * @req:      Request buffer sent to the app (must be DMA-mappable).
>> + * @req_size: Size of the request buffer.
>> + * @rsp:      Response buffer, written to by the app (must be DMA-mappable).
>> + * @rsp_size: Size of the response buffer.
>> + *
>> + * Sends a request to the QSEE app associated with the given client and read
>> + * back its response. The caller must provide two DMA memory regions, one for
>> + * the request and one for the response, and fill out the @req region with the
>> + * respective (app-specific) request data. The QSEE app reads this and returns
>> + * its response in the @rsp region.
>> + *
>> + * Return: Returns zero on success, nonzero error code on failure.
>> + */
>> +int qcom_scm_qseecom_app_send(struct qseecom_client *client, void *req,
>> +			      size_t req_size, void *rsp, size_t rsp_size)
>> +{
> 
>> +}
>> +EXPORT_SYMBOL(qcom_scm_qseecom_app_send);
> 
> Should this not be EXPORT_SYMBOL_GPL()?

Indeed it should.
  
>> +
>> +static void qseecom_client_release(struct device *dev)
>> +{
>> +	struct qseecom_client *client = container_of(dev, struct qseecom_client, aux_dev.dev);
>> +
>> +	kfree(client);
>> +}
>> +
>> +static void qseecom_client_remove(void *data)
>> +{
>> +	struct qseecom_client *client = data;
>> +
>> +	auxiliary_device_delete(&client->aux_dev);
>> +	auxiliary_device_uninit(&client->aux_dev);
>> +}
>> +
>> +static int qseecom_client_register(const struct qseecom_app_desc *desc)
>> +{
>> +	struct qseecom_client *client;
>> +	u32 app_id;
>> +	int ret;
>> +
>> +	/* Try to find the app ID, skip device if not found */
>> +	ret = qcom_scm_qseecom_app_get_id(desc->app_name, &app_id);
>> +	if (ret)
>> +		return ret == -ENOENT ? 0 : ret;
>> +
>> +	dev_info(__scm->dev, "qseecom: setting up client for %s\n", desc->app_name);
>> +
>> +	/* Allocate and set-up the client device */
>> +	client = kzalloc(sizeof(*client), GFP_KERNEL);
>> +	if (!client)
>> +		return -ENOMEM;
>> +
>> +	client->aux_dev.name = desc->dev_name;
>> +	client->aux_dev.dev.parent = __scm->dev;
>> +	client->aux_dev.dev.release = qseecom_client_release;
>> +	client->app_id = app_id;
>> +
>> +	ret = auxiliary_device_init(&client->aux_dev);
>> +	if (ret) {
>> +		kfree(client);
>> +		return ret;
>> +	}
>> +
>> +	ret = auxiliary_device_add(&client->aux_dev);
>> +	if (ret) {
>> +		auxiliary_device_uninit(&client->aux_dev);
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * Ensure that the device is properly cleaned up in case of removal or
>> +	 * errors.
>> +	 */
>> +	ret = devm_add_action_or_reset(__scm->dev, qseecom_client_remove, client);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * We do not yet support re-entrant calls via the qseecom interface. To prevent
>> + + any potential issues with this, only allow validated devices for now.
>> + */
>> +static const struct of_device_id qcom_scm_qseecom_allowlist[] = {
>> +	{ .compatible = "lenovo,thinkpad-x13s", },
>> +	{ }
>> +};
>> +
>> +static bool qcom_scm_qseecom_device_allowed(void)
>> +{
>> +	struct device_node *np;
>> +	bool match;
>> +
>> +	np = of_find_node_by_path("/");
>> +	match = of_match_node(qcom_scm_qseecom_allowlist, np);
>> +	of_node_put(np);
>> +
>> +	return match;
>> +}
>> +
>> +static const struct qseecom_app_desc qcom_scm_qseecom_apps[] = {};
>> +
>> +static int qcom_scm_qseecom_init(void)
>> +{
>> +	u32 version;
>> +	int ret, i;
>> +
>> +	/* Try to query the qseecom version, skip qseecom setup if this fails */
>> +	ret = qcom_scm_qseecom_get_version(&version);
>> +	if (ret)
>> +		return 0;
>> +
>> +	dev_info(__scm->dev, "qseecom: found qseecom with version 0x%x\n", version);
>> +
>> +	/* Check against tested/verified devices */
>> +	if (!qcom_scm_qseecom_device_allowed()) {
>> +		dev_info(__scm->dev, "qseecom: untested device, skipping\n");
> 
> Perhaps call the helper machine_allowed() and update the commit message
> to match (cf. of_machine_is_compatible())?

Thanks, I didn't know about of_machine_is_compatible(). Makes sense to
keep the naming aligned.

>> +		return 0;
>> +	}
>> +
>> +	/* Set up client devices for each base application */
>> +	for (i = 0; i < ARRAY_SIZE(qcom_scm_qseecom_apps); i++) {
>> +		ret = qseecom_client_register(&qcom_scm_qseecom_apps[i]);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +#else /* CONFIG_QCOM_SCM_QSEECOM */
>> +
>> +static int qcom_scm_qseecom_init(void)
>> +{
>> +	return 0;
>> +}
>> +
>> +#endif /* CONFIG_QCOM_SCM_QSEECOM */
>> +
>>   /**
>>    * qcom_scm_is_available() - Checks if SCM is available
>>    */
>> @@ -1496,6 +1903,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>   
>>   	__get_convention();
>>   
>> +	ret = qcom_scm_qseecom_init();
>> +	if (ret < 0) {
>> +		__scm = NULL;
> 
> So as I mentioned in my reply to 2/4, you can still have clients
> registered here when you clear the __scm pointer which they rely on
> after an error.
> 
> Not sure how best to handle this, but perhaps registering a qseecom
> platform device here and have it's driver probe defer until scm is
> available would work?
> 
> That way you could also separate out the qseecom implementation in a
> separate file (driver) rather than having the ifdef above.

The main reason it's in one file is because people were against
exporting core SCM functions in v3, fearing abuse by allowing GKI
compatibility through implementation of SCM clients in modules (i.e.
vendors loosing the last reasons to push their code upstream).

I believe I used a dedicated parent device in v3 and I tend to agree
that it's conceptually a bit nicer to separate these things (with the
downside that it needs a bit more code). I'll play around with it a bit
on the weekend and see if I can't find something better, but I think
some code will still need to stay here.

>> +		return dev_err_probe(scm->dev, ret, "Failed to initialize qseecom\n");
>> +	}
>> +
>>   	/*
>>   	 * If requested enable "download mode", from this point on warmboot
>>   	 * will cause the boot stages to enter download mode, unless
> 
> Overall this looks really good. Nice job!

Thanks!

Regards
Max

Re: [PATCH v4 3/4] firmware: qcom_scm: Add support for Qualcomm Secure Execution Environment SCM interface

[Maximilian Luz]

On 6/28/23 14:50, Johan Hovold wrote:
> On Wed, Jun 28, 2023 at 02:11:07PM +0200, Johan Hovold wrote:
>> On Mon, May 29, 2023 at 01:03:50AM +0200, Maximilian Luz wrote:
> 
>>> @@ -1496,6 +1903,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>>   
>>>   	__get_convention();
>>>   
>>> +	ret = qcom_scm_qseecom_init();
>>> +	if (ret < 0) {
>>> +		__scm = NULL;
>>
>> So as I mentioned in my reply to 2/4, you can still have clients
>> registered here when you clear the __scm pointer which they rely on
>> after an error.
>>
>> Not sure how best to handle this, but perhaps registering a qseecom
>> platform device here and have it's driver probe defer until scm is
>> available would work?
>>
>> That way you could also separate out the qseecom implementation in a
>> separate file (driver) rather than having the ifdef above.
> 
> An alternative may be to just warn and continue if
> qcom_scm_qseecom_init() fails. It should never return -EPROBE_DEFER
> anyway, right?

You're correct. That would be the simplest option. Any error returned by
qcom_scm_qseecom_init() comes from the client registration part
(qseecom_client_register()) and is either -ENOMEM or whatever
auxiliary_device_[init|add]() returns. As far as I can tell, the latter
errors out either on invalid inputs or on OOM, so it should be completely
fine to just warn about it failing.

Regards
Max

Re: [PATCH v4 4/4] firmware: Add support for Qualcomm UEFI Secure Application

[Maximilian Luz]

On 6/29/23 14:12, Johan Hovold wrote:
> On Mon, May 29, 2023 at 01:03:51AM +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 Qualcomm QSEECOM / 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@gmail.com>
>> ---
>>
>> Changes in v4:
>>   - Adapt to changes in DMA allocation in patch 3.
>>   - Rework alignment: Use native alignment of types instead of a general
>>     8 byte alignment. While the windows driver uses 8 byte alignment for
>>     GUIDs, the native (4 byte) alignment seems to work fine here.
>>   - Add a helper macro for determining size and layout of request and
>>     response buffers, taking care of proper alignment.
>>   - Implement support for EFI's query_variable_info() call, which is now
>>     supported by the kernel (and expected by efivarfs).
>>   - Move UCS-2 string helpers to lib/ucs2_string.c
> 
>> +config QCOM_QSEECOM_UEFISECAPP
>> +	tristate "Qualcomm SEE UEFI Secure App client driver"
>> +	select QCOM_SCM
>> +	select QCOM_SCM_QSEECOM
> 
> This should just be
> 
> 	depends on QCOM_SCM_QSEECOM
> 
> Using select should generally be avoided.

Okay.

>> +	depends on EFI
>> +	help
>> +	  Various Qualcomm SoCs do not allow direct access to EFI variables.
>> +	  Instead, these need to be accessed via the UEFI Secure Application
>> +	  (uefisecapp), residing in the Secure Execution Environment (SEE).
>> +
>> +	  This module provides a client driver for uefisecapp, installing efivar
>> +	  operations to allow the kernel accessing EFI variables, and via that also
>> +	  provide user-space with access to EFI variables via efivarfs.
>> +
>> +	  Select M or Y here to provide access to EFI variables on the
>> +	  aforementioned platforms.
> 
> We still have not really sorted how best to handle modular efivars
> implementations as userspace may try to mount efivarfs before the module
> has been loaded...
> 
> Perhaps require it to be built-in for now?

Sure, I can do that.

>> +
>>   config SYSFB
>>   	bool
>>   	select BOOT_VESA_SUPPORT
> 
>> +/* -- Alighment helpers ----------------------------------------------------- */
> 
> typo: Alignment

Oh, thanks for spotting that.
  
>> +
>> +/*
>> + * Helper macro to ensure proper alignment of types (fields and arrays) when
>> + * stored in some (contiguous) buffer.
>> + *
>> + * Note: The driver from which this one has been reverse-engineered expects an
>> + * alignment of 8 bytes (64 bits) for GUIDs. Our definition of efi_guid_t,
>> + * however, has an alignment of 4 byte (32 bits). So far, this seems to work
>> + * fine here. See also the comment on the typedef of efi_guid_t.
>> + */
>> +#define qcuefi_buf_align_fields(fields...)					\
>> +	({									\
>> +		size_t __len = 0;						\
>> +		fields								\
>> +		__len;								\
>> +	})
>> +
>> +#define __field_impl(size, align, offset)					\
>> +	({									\
>> +		size_t *__offset = (offset);					\
>> +		size_t __aligned;						\
>> +										\
>> +		__aligned = ALIGN(__len, align);				\
>> +		__len = __aligned + (size);					\
>> +										\
>> +		if (__offset)							\
>> +			*__offset = __aligned;					\
>> +	});
>> +
>> +#define __array_offs(type, count, offset)					\
>> +	__field_impl(sizeof(type) * (count), __alignof__(type), offset)
>> +
>> +#define __array(type, count)		__array_offs(type, count, NULL)
>> +#define __field_offs(type, offset)	__array_offs(type, 1, offset)
>> +#define __field(type)			__array_offs(type, 1, NULL)
> 
> Heh. This is some nice macro magic. :)
> 
>> +static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const efi_char16_t *name,
>> +					   const efi_guid_t *guid, u32 *attributes,
>> +					   unsigned long *data_size, void *data)
>> +{
>> +	struct qsee_req_uefi_get_variable *req_data;
>> +	struct qsee_rsp_uefi_get_variable *rsp_data;
>> +	unsigned long buffer_size = *data_size;
>> +	efi_status_t efi_status = EFI_SUCCESS;
>> +	unsigned long name_length;
>> +	size_t guid_offs;
>> +	size_t name_offs;
>> +	size_t req_size;
>> +	size_t rsp_size;
>> +	int status;
>> +
>> +	/* Validation: We need a name and GUID. */
>> +	if (!name || !guid)
>> +		return EFI_INVALID_PARAMETER;
>> +
>> +	/* Get length of name and ensure that the name is not truncated. */
>> +	name_length = ucs2_strnlen(name, QSEE_MAX_NAME_LEN) + 1;
>> +	if (name_length > QSEE_MAX_NAME_LEN)
>> +		return EFI_INVALID_PARAMETER;
>> +
>> +	/* Validation: We need a buffer if the buffer_size is nonzero. */
>> +	if (buffer_size && !data)
>> +		return EFI_INVALID_PARAMETER;
>> +
>> +	/* Compute required buffer size for request. */
>> +	req_size = qcuefi_buf_align_fields(
>> +		__field(*req_data)
>> +		__array_offs(*name, name_length, &name_offs)
>> +		__field_offs(*guid, &guid_offs)
>> +	);
>> +
>> +	/* Compute required buffer size for response. */
>> +	rsp_size = qcuefi_buf_align_fields(
>> +		__field(*rsp_data)
>> +		__array(u8, buffer_size)
>> +	);
>> +
>> +	/* Allocate request buffer. */
>> +	req_data = kzalloc(req_size, GFP_KERNEL);
>> +	if (!req_data) {
>> +		efi_status = EFI_OUT_OF_RESOURCES;
>> +		goto out;
>> +	}
>> +
>> +	/* Allocate response buffer. */
>> +	rsp_data = kzalloc(rsp_size, GFP_KERNEL);
>> +	if (!rsp_data) {
>> +		efi_status = EFI_OUT_OF_RESOURCES;
>> +		goto out_free_req;
>> +	}
>> +
>> +	/* Set up request data. */
>> +	req_data->command_id = QSEE_CMD_UEFI_GET_VARIABLE;
>> +	req_data->data_size = buffer_size;
>> +	req_data->name_offset = name_offs;
>> +	req_data->name_size = name_length * sizeof(*name);
>> +	req_data->guid_offset = guid_offs;
>> +	req_data->guid_size = sizeof(*guid);
>> +	req_data->length = req_size;
>> +
>> +	/* Copy request parameters. */
>> +	ucs2_strlcpy(((void *)req_data) + req_data->name_offset, name, name_length);
>> +	memcpy(((void *)req_data) + req_data->guid_offset, guid, req_data->guid_size);
>> +
>> +	/* Perform SCM call. */
>> +	status = qcom_scm_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size);
>> +
>> +	/* Check for errors and validate. */
>> +	if (status) {
>> +		efi_status = EFI_DEVICE_ERROR;
>> +		goto out_free;
>> +	}
>> +
>> +	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_VARIABLE) {
>> +		efi_status = EFI_DEVICE_ERROR;
>> +		goto out_free;
>> +	}
>> +
>> +	if (rsp_data->length < sizeof(*rsp_data)) {
>> +		efi_status = EFI_DEVICE_ERROR;
>> +		goto out_free;
>> +	}
>> +
>> +	if (rsp_data->status) {
>> +		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
>> +			__func__, rsp_data->status);
>> +		efi_status = qsee_uefi_status_to_efi(rsp_data->status);
>> +
>> +		/* Update size and attributes in case buffer is too small. */
>> +		if (efi_status == EFI_BUFFER_TOO_SMALL) {
>> +			*data_size = rsp_data->data_size;
>> +			if (attributes)
>> +				*attributes = rsp_data->attributes;
>> +		}
>> +
>> +		goto out_free;
>> +	}
>> +
>> +	if (rsp_data->length > rsp_size) {
>> +		efi_status = EFI_DEVICE_ERROR;
>> +		goto out_free;
>> +	}
>> +
>> +	if (rsp_data->data_offset + rsp_data->data_size > rsp_data->length) {
>> +		efi_status = EFI_DEVICE_ERROR;
>> +		goto out_free;
>> +	}
>> +
>> +	/* Set attributes and data size even if buffer is too small. */
>> +	*data_size = rsp_data->data_size;
>> +	if (attributes)
>> +		*attributes = rsp_data->attributes;
>> +
>> +	/*
>> +	 * If we have a buffer size of zero and no buffer, just return
>> +	 * attributes and required size.
>> +	 */
>> +	if (buffer_size == 0 && !data) {
>> +		efi_status = EFI_SUCCESS;
>> +		goto out_free;
>> +	}
>> +
>> +	/* Validate output buffer size. */
>> +	if (buffer_size < rsp_data->data_size) {
>> +		efi_status = EFI_BUFFER_TOO_SMALL;
>> +		goto out_free;
>> +	}
>> +
>> +	/* Copy to output buffer. Note: We're guaranteed to have one at this point. */
>> +	memcpy(data, ((void *)rsp_data) + rsp_data->data_offset, rsp_data->data_size);
>> +
>> +out_free:
>> +	kfree(rsp_data);
>> +out_free_req:
>> +	kfree(req_data);
>> +out:
>> +	return efi_status;
>> +}
> 
> The above reads very easily as it stands, but generally we try to avoid
> adding comments inside functions unless doing something unexpected or
> similar.
> 
> Comments like "Allocate response buffer" and "Perform SCM call" should
> not be needed as it should be clear from the code (given apt names of
> variables and function which you already seem to have chosen).

Okay, I'll drop those.

>> +/* -- Global efivar interface. ---------------------------------------------- */
>> +
>> +static struct qcuefi_client *__qcuefi;
>> +static DEFINE_MUTEX(__qcuefi_lock);
>> +
>> +static int qcuefi_set_reference(struct qcuefi_client *qcuefi)
>> +{
>> +	mutex_lock(&__qcuefi_lock);
>> +
>> +	if (qcuefi && __qcuefi) {
>> +		mutex_unlock(&__qcuefi_lock);
>> +		return -EEXIST;
>> +	}
>> +
>> +	__qcuefi = qcuefi;
>> +
>> +	mutex_unlock(&__qcuefi_lock);
>> +	return 0;
>> +}
>> +
>> +static struct qcuefi_client *qcuefi_acquire(void)
>> +	__acquires(&__qcuefi_lock)
> 
> I noticed that someone told you to add sparse annotation here but that
> was not correct.
> 
> The mutex implementation does not use sparse annotation so this actually
> introduces sparse warnings such as:
> 
> /home/johan/work/linaro/src/linux/drivers/firmware/qcom_qseecom_uefisecapp.c:741:29: warning: context imbalance in 'qcuefi_acquire' - wrong count at exit
> 
> Just drop the annotation again.

Sure, will do. I might also want to look into actually running sparse at
some point I guess...
  
>> +{
>> +	mutex_lock(&__qcuefi_lock);
>> +	return __qcuefi;
>> +}
>> +
>> +static void qcuefi_release(void)
>> +	__releases(&__qcuefi_lock)
>> +{
>> +	mutex_unlock(&__qcuefi_lock);
>> +}
> 
>> +static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
>> +				 const struct auxiliary_device_id *aux_dev_id)
>> +{
>> +	struct qcuefi_client *qcuefi;
>> +	int status;
>> +
>> +	/* Allocate driver data. */
> 
> As mentioned above, I suggest dropping comments like this throughout.

Sure.
  
>> +	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);
>> +
>> +	/* Register global reference. */
>> +	auxiliary_set_drvdata(aux_dev, qcuefi);
>> +	status = qcuefi_set_reference(qcuefi);
>> +	if (status)
>> +		return status;
>> +
>> +	/* Register efivar ops. */
>> +	status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops);
>> +	if (status)
>> +		qcuefi_set_reference(NULL);
>> +
>> +	return status;
>> +}

Again, thanks for the review and the patience. I'll try to prepare a new
version over the weekend.

Regards
Max