On Thu, Dec 05, 2024 at 05:56:35AM +0100, Vabhav Sharma wrote:
> +
> +int imx_secvio_sc_debugfs(struct device *dev)
> +{
> + struct imx_secvio_sc_data *data = dev_get_drvdata(dev);
> + struct dentry *dir;
> + int ret = 0;
> +
> + /* Create a folder */
> + dir = debugfs_create_dir(dev_name(dev), NULL);
> + if (IS_ERR(dir)) {
> + dev_err(dev, "Failed to create dfs dir\n");
Drop
> + ret = PTR_ERR(dir);
> + goto exit;
Drop all this.
> + }
> + data->dfs = dir;
> +
> + ret = devm_add_action(dev, if_debugfs_remove_recursive, data->dfs);
> + if (ret) {
> + dev_err(dev, "Failed to add managed action to disable IRQ\n");
Drop
> + goto remove_fs;
> + }
> +
> + /* Create the file to read info and write to reg */
> + dir = debugfs_create_file("info", 0x666, data->dfs, dev,
> + &imx_secvio_sc_info_ops);
> + if (IS_ERR(dir)) {
> + dev_err(dev, "Failed to add info to debugfs\n");
There is *never such code*. You never print error messages and fail the
probe on debugfs. Drop.
> + ret = PTR_ERR(dir);
> + goto exit;
Drop
This has several trivial issues. I suggest doing extensive
internal review before asking the community to look at this.
> + }
> +
> +exit:
> + return ret;
> +
> +remove_fs:
> + debugfs_remove_recursive(data->dfs);
> + goto exit;
> +}
> diff --git a/drivers/firmware/imx/imx-scu-secvio.c b/drivers/firmware/imx/imx-scu-secvio.c
> new file mode 100644
> index 000000000000..30eb9e4bf7b8
> --- /dev/null
> +++ b/drivers/firmware/imx/imx-scu-secvio.c
> @@ -0,0 +1,619 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019, 2024 NXP
> + *
> + */
> +
> +/*
> + * The i.MX8QXP SoC contains the Secure Non-Volatile Storage (SNVS) block. This
> + * block can detect specific hardware attacks.This block can only be accessible
> + * using the SCFW API.
> + *
> + * This module interact with the SCU which relay request to/from the SNVS block
> + * to detect if security violation occurred.
> + *
> + * The module exports an API to add processing when a SV is detected:
> + * - register_imx_secvio_sc_notifier
> + * - unregister_imx_secvio_sc_notifier
> + * - imx_secvio_sc_check_state
> + * - imx_secvio_sc_clear_state
> + * - imx_secvio_sc_enable_irq
> + * - imx_secvio_sc_disable_irq
> + */
> +
> +#include <dt-bindings/firmware/imx/rsrc.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/kernel.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +
> +#include <linux/uaccess.h>
> +
> +#include <linux/firmware/imx/ipc.h>
> +#include <linux/firmware/imx/sci.h>
> +#include <linux/firmware/imx/svc/seco.h>
> +#include <linux/firmware/imx/svc/rm.h>
> +#include <linux/firmware/imx/svc/imx-secvio-sc.h>
> +
> +/* Reference on the driver_device */
> +static struct device *imx_secvio_sc_dev;
> +
> +/* Register IDs for sc_seco_secvio_config API */
> +#define HPSVS_ID 0x18
> +#define LPS_ID 0x4c
> +#define LPTDS_ID 0xa4
> +#define HPVIDR_ID 0xf8
> +
> +#define SECO_MINOR_VERSION_SUPPORT_SECVIO_TAMPER 0x53
> +#define SECO_VERSION_MINOR_MASK GENMASK(15, 0)
> +
> +static struct platform_driver imx_secvio_sc_driver;
Drop, not used... or you messed up cleaning processes.
> +static struct platform_device *pdev;
Singletons are really poor coding approach. Fix your driver architecture
so this will be properly instantiated only once, not multiple times.
> +static struct device *scdev;
Drop, not needed. Fix your driver.
> +
> +/* Notifier list for new CB */
> +static BLOCKING_NOTIFIER_HEAD(imx_secvio_sc_notifier_chain);
> +
> +int register_imx_secvio_sc_notifier(struct notifier_block *nb)
> +{
> + return blocking_notifier_chain_register(&imx_secvio_sc_notifier_chain,
> + nb);
> +}
> +EXPORT_SYMBOL(register_imx_secvio_sc_notifier);
NAK
> +
> +int unregister_imx_secvio_sc_notifier(struct notifier_block *nb)
> +{
> + return blocking_notifier_chain_unregister(&imx_secvio_sc_notifier_chain,
> + nb);
> +}
> +EXPORT_SYMBOL(unregister_imx_secvio_sc_notifier);
1. You mix up exports with statics. This driver is poorly organized.
Really it is a mess here.
2. Why do you need to export these? These are not used, so you cannot
have them.
That's a strong NAK. Stop bringing here some stuff for yourodownstream.
> +
> +static void if_imx_scu_irq_register_notifier(void *nb)
> +{
> + imx_scu_irq_register_notifier(nb);
> +}
> +
> +static void if_unregister_imx_secvio_sc_notifier(void *nb)
> +{
> + unregister_imx_secvio_sc_notifier(nb);
> +}
> +
> +static
> +int imx_secvio_sc_notifier_call_chain(struct secvio_sc_notifier_info *info)
> +{
> + return blocking_notifier_call_chain(&imx_secvio_sc_notifier_chain, 0,
> + (void *)info);
> +}
> +
> +int imx_secvio_sc_get_state(struct device *dev,
> + struct secvio_sc_notifier_info *info)
> +{
> + int ret, err = 0;
> + struct imx_secvio_sc_data *data;
> +
> + dev = imx_secvio_sc_dev;
> + if (!dev)
> + return -EINVAL;
> +
> + data = dev_get_drvdata(dev);
> +
> + /* Read secvio status */
> + ret = imx_sc_seco_secvio_config(data->ipc_handle, HPSVS_ID, SECVIO_CONFIG_READ,
> + &info->hpsvs, NULL, NULL, NULL, NULL, 1);
> + if (ret) {
> + err = ret;
> + dev_err(dev, "Fail read secvio config status %d\n", ret);
> + }
> + info->hpsvs &= HPSVS_ALL_SV_MASK;
> +
> + /* Read tampers status */
> + ret = imx_sc_seco_secvio_config(data->ipc_handle, LPS_ID, SECVIO_CONFIG_READ,
> + &info->lps, NULL, NULL, NULL, NULL, 1);
> + if (ret) {
> + err = ret;
> + dev_err(dev, "Fail read tamper 1 status: %d\n", ret);
> + }
> + info->lps &= LPS_ALL_TP_MASK;
> +
> + ret = imx_sc_seco_secvio_config(data->ipc_handle, LPTDS_ID, SECVIO_CONFIG_READ,
> + &info->lptds, NULL, NULL, NULL, NULL, 1);
> + if (ret) {
> + err = ret;
> + dev_err(dev, "Fail read tamper 2 status: %d\n", ret);
> + }
> + info->lptds &= LPTDS_ALL_TP_MASK;
> +
> + dev_dbg(dev, "Status: %.8x, %.8x, %.8x\n", info->hpsvs,
> + info->lps, info->lptds);
No, don't print internal values.
> +
> + return err;
> +}
> +EXPORT_SYMBOL(imx_secvio_sc_get_state);
NAK, no users.
> +
> +int imx_secvio_sc_check_state(struct device *dev)
> +{
> + struct secvio_sc_notifier_info info;
> + int ret;
> +
> + dev = imx_secvio_sc_dev;
> +
> + ret = imx_secvio_sc_get_state(dev, &info);
> + if (ret) {
> + dev_err(dev, "Failed to get secvio state\n");
> + return ret;
> + }
> +
> + /* Call chain of CB registered to this module if status detected */
> + if (info.hpsvs || info.lps || info.lptds)
> + if (imx_secvio_sc_notifier_call_chain(&info))
> + dev_warn(dev,
> + "Issues when calling the notifier chain\n");
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(imx_secvio_sc_check_state);
> +
NAK, no users.
> +static int imx_secvio_sc_disable_irq(struct device *dev)
> +{
> + int ret;
> +
> + if (!dev)
> + return -EINVAL;
> +
> + /* Disable the IRQ */
> + ret = imx_scu_irq_group_enable(IMX_SC_IRQ_GROUP_WAKE, IMX_SC_IRQ_SECVIO,
> + false);
> + if (ret) {
> + dev_err(dev, "Cannot disable SCU IRQ: %d\n", ret);
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +static int imx_secvio_sc_enable_irq(struct device *dev)
> +{
> + int ret = 0, err;
> + u32 irq_status;
> + struct imx_secvio_sc_data *data;
> +
> + if (!dev)
> + return -EINVAL;
> +
> + data = dev_get_drvdata(dev);
> +
> + /* Enable the IRQ */
> + ret = imx_scu_irq_group_enable(IMX_SC_IRQ_GROUP_WAKE, IMX_SC_IRQ_SECVIO,
> + true);
> + if (ret) {
> + dev_err(dev, "Cannot enable SCU IRQ: %d\n", ret);
> + goto exit;
> + }
> +
> + /* Enable interrupt */
> + ret = imx_sc_seco_secvio_enable(data->ipc_handle);
> + if (ret) {
> + dev_err(dev, "Cannot enable SNVS irq: %d\n", ret);
> + goto exit;
> + }
> +
> + /* Unmask interrupt */
> + ret = imx_scu_irq_get_status(IMX_SC_IRQ_GROUP_WAKE, &irq_status);
> + if (ret) {
> + dev_err(dev, "Cannot unmask irq: %d\n", ret);
> + goto exit;
> + }
> +
> +exit:
> + if (ret) {
> + err = imx_secvio_sc_disable_irq(dev);
> + if (err)
> + dev_warn(dev, "Failed to disable the IRQ\n");
> + }
> +
> + return ret;
> +}
> +
> +static int imx_secvio_sc_notify(struct notifier_block *nb,
> + unsigned long event, void *group)
> +{
> + struct imx_secvio_sc_data *data =
> + container_of(nb, struct imx_secvio_sc_data,
> + irq_nb);
> + struct device *dev = data->dev;
> + int ret;
> +
> + /* Filter event for us */
> + if (!((event & IMX_SC_IRQ_SECVIO) &&
> + (*(u8 *)group == IMX_SC_IRQ_GROUP_WAKE)))
> + return 0;
> +
> + dev_warn(dev, "secvio security violation detected\n");
> +
> + ret = imx_secvio_sc_check_state(dev);
> +
> + /* Re-enable interrupt */
> + ret = imx_secvio_sc_enable_irq(dev);
> + if (ret)
> + dev_err(dev, "Failed to enable IRQ\n");
> +
> + return ret;
> +}
> +
> +int imx_secvio_sc_clear_state(struct device *dev, u32 hpsvs, u32 lps, u32 lptds)
> +{
> + int ret;
> + struct imx_secvio_sc_data *data;
> +
> + dev = imx_secvio_sc_dev;
> + if (!dev)
> + return -EINVAL;
> +
> + data = dev_get_drvdata(dev);
> +
> + ret = imx_sc_seco_secvio_config(data->ipc_handle, HPSVS_ID, SECVIO_CONFIG_WRITE,
> + &hpsvs, NULL, NULL, NULL, NULL, 1);
> + if (ret) {
> + dev_err(dev, "Fail to clear secvio status: %d\n", ret);
> + return ret;
> + }
> +
> + ret = imx_sc_seco_secvio_config(data->ipc_handle, LPS_ID, SECVIO_CONFIG_WRITE,
> + &lps, NULL, NULL, NULL, NULL, 1);
> + if (ret) {
> + dev_err(dev, "Fail to clear tamper 1 status: %d\n", ret);
> + return ret;
> + }
> +
> + ret = imx_sc_seco_secvio_config(data->ipc_handle, LPTDS_ID, SECVIO_CONFIG_WRITE,
> + &lptds, NULL, NULL, NULL, NULL, 1);
> + if (ret) {
> + dev_err(dev, "Fail to clear tamper 2 status: %d\n", ret);
> + return ret;
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(imx_secvio_sc_clear_state);
So again export appears after several static functions? Organize your
driver in readable style.
However that's anyway a NAK, no users.
> +
> +static int report_to_user_notify(struct notifier_block *nb,
> + unsigned long status, void *notif_info)
> +{
> + struct secvio_sc_notifier_info *info = notif_info;
> + struct imx_secvio_sc_data *data =
> + container_of(nb, struct imx_secvio_sc_data,
> + report_nb);
> + struct device *dev = data->dev;
> +
> + /* Information about the security violation */
> + if (info->hpsvs & HPSVS_LP_SEC_VIO_MASK)
> + dev_info(dev, "SNVS secvio: LPSV\n");
> + if (info->hpsvs & HPSVS_SW_LPSV_MASK)
> + dev_info(dev, "SNVS secvio: SW LPSV\n");
> + if (info->hpsvs & HPSVS_SW_FSV_MASK)
> + dev_info(dev, "SNVS secvio: SW FSV\n");
> + if (info->hpsvs & HPSVS_SW_SV_MASK)
> + dev_info(dev, "SNVS secvio: SW SV\n");
> + if (info->hpsvs & HPSVS_SV5_MASK)
> + dev_info(dev, "SNVS secvio: SV 5\n");
> + if (info->hpsvs & HPSVS_SV4_MASK)
> + dev_info(dev, "SNVS secvio: SV 4\n");
> + if (info->hpsvs & HPSVS_SV3_MASK)
> + dev_info(dev, "SNVS secvio: SV 3\n");
> + if (info->hpsvs & HPSVS_SV2_MASK)
> + dev_info(dev, "SNVS secvio: SV 2\n");
> + if (info->hpsvs & HPSVS_SV1_MASK)
> + dev_info(dev, "SNVS secvio: SV 1\n");
> + if (info->hpsvs & HPSVS_SV0_MASK)
> + dev_info(dev, "SNVS secvio: SV 0\n");
> +
> + /* Information about the tampers */
> + if (info->lps & LPS_ESVD_MASK)
> + dev_info(dev, "SNVS tamper: External SV\n");
> + if (info->lps & LPS_ET2D_MASK)
> + dev_info(dev, "SNVS tamper: Tamper 2\n");
> + if (info->lps & LPS_ET1D_MASK)
> + dev_info(dev, "SNVS tamper: Tamper 1\n");
> + if (info->lps & LPS_WMT2D_MASK)
> + dev_info(dev, "SNVS tamper: Wire Mesh 2\n");
> + if (info->lps & LPS_WMT1D_MASK)
> + dev_info(dev, "SNVS tamper: Wire Mesh 1\n");
> + if (info->lps & LPS_VTD_MASK)
> + dev_info(dev, "SNVS tamper: Voltage\n");
> + if (info->lps & LPS_TTD_MASK)
> + dev_info(dev, "SNVS tamper: Temperature\n");
> + if (info->lps & LPS_CTD_MASK)
> + dev_info(dev, "SNVS tamper: Clock\n");
> + if (info->lps & LPS_PGD_MASK)
> + dev_info(dev, "SNVS tamper: Power Glitch\n");
> + if (info->lps & LPS_MCR_MASK)
> + dev_info(dev, "SNVS tamper: Monotonic Counter rollover\n");
> + if (info->lps & LPS_SRTCR_MASK)
> + dev_info(dev, "SNVS tamper: Secure RTC rollover\n");
> + if (info->lps & LPS_LPTA_MASK)
> + dev_info(dev, "SNVS tamper: Time alarm\n");
> +
> + if (info->lptds & LPTDS_ET10D_MASK)
> + dev_info(dev, "SNVS tamper: Tamper 10\n");
> + if (info->lptds & LPTDS_ET9D_MASK)
> + dev_info(dev, "SNVS tamper: Tamper 9\n");
> + if (info->lptds & LPTDS_ET8D_MASK)
> + dev_info(dev, "SNVS tamper: Tamper 8\n");
> + if (info->lptds & LPTDS_ET7D_MASK)
> + dev_info(dev, "SNVS tamper: Tamper 7\n");
> + if (info->lptds & LPTDS_ET6D_MASK)
> + dev_info(dev, "SNVS tamper: Tamper 6\n");
> + if (info->lptds & LPTDS_ET5D_MASK)
> + dev_info(dev, "SNVS tamper: Tamper 5\n");
> + if (info->lptds & LPTDS_ET4D_MASK)
> + dev_info(dev, "SNVS tamper: Tamper 4\n");
> + if (info->lptds & LPTDS_ET3D_MASK)
> + dev_info(dev, "SNVS tamper: Tamper 3\n");
> +
> + return 0;
> +}
> +
> +static void if_imx_secvio_sc_disable_irq(void *dev)
> +{
> + imx_secvio_sc_disable_irq(dev);
> +}
> +
> +static int imx_secvio_sc_open(struct inode *node, struct file *filp)
> +{
> + filp->private_data = node->i_private;
> +
> + return 0;
> +}
> +
> +static long imx_secvio_sc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + struct device *dev = file->private_data;
> + struct secvio_sc_notifier_info info;
> + int ret;
> +
> + switch (cmd) {
> + case IMX_SECVIO_SC_GET_STATE:
> + ret = imx_secvio_sc_get_state(dev, &info);
> + if (ret)
> + return ret;
> +
> + ret = copy_to_user((void __user *)arg, &info, sizeof(info));
> + if (ret) {
> + dev_err(dev, "Fail to copy info to user\n");
> + return -EFAULT;
> + }
> + break;
> + case IMX_SECVIO_SC_CHECK_STATE:
> + ret = imx_secvio_sc_check_state(dev);
> + if (ret)
> + return ret;
> + break;
> + case IMX_SECVIO_SC_CLEAR_STATE:
> + ret = copy_from_user(&info, (void __user *)arg, sizeof(info));
> + if (ret) {
> + dev_err(dev, "Fail to copy info from user\n");
> + return -EFAULT;
> + }
> +
> + ret = imx_secvio_sc_clear_state(dev, info.hpsvs, info.lps,
> + info.lptds);
> + if (ret)
> + return ret;
> + break;
> + default:
> + ret = -ENOIOCTLCMD;
> + }
> +
> + return ret;
> +}
> +
> +static const struct file_operations imx_secvio_sc_fops = {
> + .owner = THIS_MODULE,
> + .open = imx_secvio_sc_open,
> + .unlocked_ioctl = imx_secvio_sc_ioctl,
> +};
> +
> +static void if_misc_deregister(void *miscdevice)
> +{
> + misc_deregister(miscdevice);
> +}
> +
> +int imx_scu_secvio_init(struct device *np)
> +{
> + int ret;
> +
> + scdev = np;
> +
> + pdev = platform_device_alloc("imx-secvio-sc", -1);
> + if (!pdev) {
> + pr_err("%s: secvio: Failed to allocate secvio device\n", __func__);
> + return -ENOMEM;
> + }
> +
> + ret = platform_device_add(pdev);
> + if (ret != 0) {
> + pr_err("%s: secvio: Failed to add secvio device\n", __func__);
> + platform_device_put(pdev);
> + return ret;
> + }
> +
> + ret = platform_driver_register(&imx_secvio_sc_driver);
> + if (ret != 0) {
> + pr_err("%s: secvio: Failed to register secvio driver\n", __func__);
> + platform_device_unregister(pdev);
> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(imx_scu_secvio_init);
1. GPL
2. Init is always at the end which avoids your forward declarations
> +
> +static int imx_secvio_sc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct imx_secvio_sc_data *data;
> + u32 seco_version = 0;
> + bool own_secvio;
> + u32 irq_status;
> + int ret;
> +
> + /* Allocate private data */
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + if (!devres_open_group(dev, NULL, GFP_KERNEL))
> + return -ENOMEM;
> +
> + data->dev = dev;
> +
> + dev_set_drvdata(dev, data);
> +
> + data->nvmem = devm_nvmem_device_get(scdev, NULL);
> + if (IS_ERR(data->nvmem)) {
> + ret = PTR_ERR(data->nvmem);
> +
> + if (ret != -EPROBE_DEFER)
> + dev_err_probe(dev, ret, "Failed to retrieve nvmem\n");
Do not open-code dev_err_probe. Syntax is:
ret = dev_err_probe()
> +
> + goto clean;
> + }
> +
> + /* Get a handle */
> + ret = imx_scu_get_handle(&data->ipc_handle);
> + if (ret) {
> + dev_err(dev, "cannot get handle to scu: %d\n", ret);
dev_err_probe. Open the function and analyze whether it can defer
(although one would argue that even if it cannot defer you are supposed
to use dev_err_probe()).
> + goto clean;
> + }
> +
> + /* Check the version of the SECO */
> + ret = imx_sc_seco_build_info(data->ipc_handle, &seco_version, NULL);
> + if (ret) {
> + dev_err(dev, "Failed to get seco version\n");
> + goto clean;
> + }
> +
> + if ((seco_version & SECO_VERSION_MINOR_MASK) <
> + SECO_MINOR_VERSION_SUPPORT_SECVIO_TAMPER) {
> + dev_err(dev, "SECO version %.8x doesn't support all secvio\n",
> + seco_version);
> + ret = -EOPNOTSUPP;
> + goto clean;
> + }
> +
> + /* Init debug FS */
> + ret = imx_secvio_sc_debugfs(dev);
> + if (ret) {
> + dev_err(dev, "Failed to set debugfs\n");
debugfs errors are never a probe failure.
> + goto clean;
> + }
> +
> + /* Check we own the SECVIO */
> + ret = imx_sc_rm_is_resource_owned(data->ipc_handle, IMX_SC_R_SECVIO);
> + if (ret < 0) {
This is not posible. Read the doc: 0 or 1.
> + dev_err(dev, "Failed to retrieve secvio ownership\n");
> + goto clean;
> + }
> +
> + own_secvio = ret > 0;
> + if (!own_secvio) {
> + dev_err(dev, "Secvio resource is not owned\n");
> + ret = -EPERM;
> + goto clean;
> + }
Best regards,
Krzysztof
