| 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 22:11:18 +0200
From: Maximilian Luz <luzmaximilian@...il.com>
To: Johan Hovold <johan@...nel.org>
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 8/4/23 18:48, Johan Hovold wrote:
> 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.
Right. I'll add both for the next version.
>> +
>> +#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.
Sure, I'll do that.
>> +
>> + 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).
Okay.
>> +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.
Right.
>> +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.
Oh right. As long as the module and device name match this should work
automatically, correct? I forgot about that.
>> +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.
Sure, I can do that.
>> +}
>> +
>> +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"?
That would be more consistent, yes.
>> + 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:".
Sure, I'll do that.
>> + * 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'.
Right.
>> +
>> 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.
Already done :)
>> +
>> +#endif /* CONFIG_QCOM_QSEECOM */
>> +
>> #endif
>
> With the above fixed you can add my
>
> Reviewed-by: Johan Hovold <johan+linaro@...nel.org>
Thanks!
Regards
Max
Powered by blists - more mailing lists