| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 4 Aug 2023 18:48:57 +0200
From: Johan Hovold <johan@...nel.org>
To: Maximilian Luz <luzmaximilian@...il.com>
Cc: Bjorn Andersson <andersson@...nel.org>,
Andy Gross <agross@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Ard Biesheuvel <ardb@...nel.org>,
Ilias Apalodimas <ilias.apalodimas@...aro.org>,
Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
Sudeep Holla <sudeep.holla@....com>,
Steev Klimaszewski <steev@...i.org>,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/3] firmware: qcom_scm: Add support for Qualcomm
Secure Execution Environment SCM interface
On Sun, Jul 30, 2023 at 06:19:03PM +0200, Maximilian Luz wrote:
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for Qualcomm Secure Execution Environment (SEE) interface (QSEECOM).
> + * Responsible for setting up and managing QSEECOM client devices.
> + *
> + * Copyright (C) 2023 Maximilian Luz <luzmaximilian@...il.com>
> + */
> +#include <linux/auxiliary_bus.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
Looks like you're missing some includes like module.h and slab.h.
> +
> +#include <linux/firmware/qcom/qcom_qseecom.h>
> +#include <linux/firmware/qcom/qcom_scm.h>
> +static void qseecom_client_release(struct device *dev)
> +{
> + struct qseecom_client *client = container_of(dev, struct qseecom_client, aux_dev.dev);
Nit: Perhaps you can separate declaration and initialisation here to
stay within 80 columns.
> +
> + kfree(client);
> +}
> +static int qcom_qseecom_remove(struct platform_device *qseecom_dev)
> +{
> + return 0; /* Nothing to do here, all is managed via devm. */
> +}
You should just drop this one (even if it serves as documentation).
> +static struct platform_driver qcom_qseecom_driver = {
> + .driver = {
> + .name = "qcom_qseecom",
> + },
> + .probe = qcom_qseecom_probe,
> + .remove = qcom_qseecom_remove,
> +};
> +
> +static int __init qcom_qseecom_init(void)
> +{
> + return platform_driver_register(&qcom_qseecom_driver);
> +}
> +subsys_initcall(qcom_qseecom_init);
> +
> +static void __exit qcom_qseecom_exit(void)
> +{
> + platform_driver_unregister(&qcom_qseecom_driver);
> +}
> +module_exit(qcom_qseecom_exit);
No need for this one either since this driver can only be built-in now.
> +MODULE_AUTHOR("Maximilian Luz <luzmaximilian@...il.com>");
> +MODULE_DESCRIPTION("Driver for the Qualcomm SEE (QSEECOM) interface");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:qcom_qseecom");
No need for MODULE_ALIAS() either.
> +static void qcom_scm_qseecom_free(void *data)
> +{
> + struct platform_device *qseecom_dev = data;
> +
> + platform_device_unregister(qseecom_dev);
Perhaps use platform_device_del() and platform_device_put() for symmetry
as you're not using platform_device_register() below.
> +}
> +
> +static int qcom_scm_qseecom_init(struct qcom_scm *scm)
> +{
> + struct platform_device *qseecom_dev;
> + u32 version;
> + int ret;
> +
> + /*
> + * Note: We do two steps of validation here: First, we try to query the
> + * QSEECOM version as a check to see if the interface exists on this
> + * device. Second, we check against known good devices due to current
> + * driver limitations (see comment in qcom_scm_qseecom_allowlist).
> + *
> + * Note that we deliberately do the machine check after the version
> + * check so that we can log potentially supported devices. This should
> + * be safe as downstream sources indicate that the version query is
> + * neither blocking nor reentrant.
> + */
> + ret = qcom_scm_qseecom_get_version(&version);
> + if (ret)
> + return 0;
> +
> + dev_info(scm->dev, "qseecom: found qseecom with version 0x%x\n", version);
> +
> + if (!qcom_scm_qseecom_machine_is_allowed()) {
> + dev_info(scm->dev, "qseecom: untested device, skipping\n");
untested "machine"?
> + return 0;
> + }
> +
> + /*
> + * Set up QSEECOM interface device. All application clients will be
> + * set up and managed by the corresponding driver for it.
> + */
> + qseecom_dev = platform_device_alloc("qcom_qseecom", -1);
> + if (!qseecom_dev)
> + return -ENOMEM;
> +
> + qseecom_dev->dev.parent = scm->dev;
> +
> + ret = platform_device_add(qseecom_dev);
> + if (ret) {
> + platform_device_put(qseecom_dev);
> + return ret;
> + }
> +
> + return devm_add_action_or_reset(scm->dev, qcom_scm_qseecom_free, qseecom_dev);
> +}
> +
> +#else /* CONFIG_QCOM_QSEECOM */
> +
> +static int qcom_scm_qseecom_init(struct qcom_scm *scm)
> +{
> + return 0;
> +}
> +
> +#endif /* CONFIG_QCOM_QSEECOM */
> +
> /**
> * qcom_scm_is_available() - Checks if SCM is available
> */
> @@ -1468,6 +1848,18 @@ static int qcom_scm_probe(struct platform_device *pdev)
> if (download_mode)
> qcom_scm_set_download_mode(true);
>
> + /*
> + * Initialize the QSEECOM interface. Note: QSEECOM is fairly
Nit: I'd add a line break and an empty line before the "Note:".
> + * self-contained and this only adds the interface device (the driver
> + * of which does most of the heavy lifting). So any errors returned
> + * here should be either -ENOMEM or -EINVAL (with the latter only in
> + * case there's a bug in our code). This means that there is no need to
> + * bring down the whole SCM driver. Just log the error instead and let
> + * SCM live.
> + */
> + ret = qcom_scm_qseecom_init(scm);
> + WARN(ret < 0, "failed to initialize qseecom: %d", ret);
Missing '\n'.
> +
> return 0;
> }
>
> +#ifdef CONFIG_QCOM_QSEECOM
> +
> +int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id);
> +int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
> + size_t rsp_size);
> +
> +#else /* CONFIG_QCOM_QSEECOM */
> +
> +int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id)
> +{
> + return -EINVAL;
> +}
> +
> +int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
> + size_t rsp_size)
> +{
> + return -EINVAL;
> +}
These should be static inline as you already noticed.
> +
> +#endif /* CONFIG_QCOM_QSEECOM */
> +
> #endif
With the above fixed you can add my
Reviewed-by: Johan Hovold <johan+linaro@...nel.org>
Johan
Powered by blists - more mailing lists