linux-kernel - Re: [PATCH v5 06/11] firmware: imx: add driver for NXP EdgeLock Enclave

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f7a1252f-d043-b197-6d21-2a603d928da3@gmx.net>
Date:   Fri, 25 Aug 2023 12:22:24 +0200
From:   Stefan Wahren <wahrenst@....net>
To:     Pankaj Gupta <pankaj.gupta@....com>, shawnguo@...nel.org,
        s.hauer@...gutronix.de, kernel@...gutronix.de, clin@...e.com,
        conor+dt@...nel.org, pierre.gondois@....com, ping.bai@....com,
        xiaoning.wang@....com, wei.fang@....com, peng.fan@....com,
        haibo.chen@....com, festevam@...il.com, linux-imx@....com,
        davem@...emloft.net, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org,
        linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, gaurav.jain@....com,
        alexander.stein@...tq-group.com, sahil.malhotra@....com,
        aisheng.dong@....com, V.Sethi@....com
Cc:     kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v5 06/11] firmware: imx: add driver for NXP EdgeLock
 Enclave

Hi Pankaj,

Am 23.08.23 um 09:33 schrieb Pankaj Gupta:
> The Edgelock Enclave , is the secure enclave embedded in the SoC
> to support the features like HSM, SHE & V2X, using message based
> communication channel.
>
> ELE FW communicates on a dedicated MU with application core where
> kernel is running. It exists on specific i.MX processors. e.g.
> i.MX8ULP, i.MX93.
>
> Reported-by: kernel test robot <lkp@...el.com>
> Closes:https://lore.kernel.org/oe-kbuild-all/202304120902.bP52A56z-lkp@intel.com
> Signed-off-by: Pankaj Gupta <pankaj.gupta@....com>
> ---
>   Documentation/ABI/testing/se-cdev         |   29 +
>   drivers/firmware/imx/Kconfig              |   12 +
>   drivers/firmware/imx/Makefile             |    2 +
>   drivers/firmware/imx/ele_base_msg.c       |   62 ++
>   drivers/firmware/imx/ele_common.c         |   34 +
>   drivers/firmware/imx/ele_common.h         |   21 +
>   drivers/firmware/imx/se_fw.c              | 1201 +++++++++++++++++++++
>   drivers/firmware/imx/se_fw.h              |  168 +++
>   include/linux/firmware/imx/ele_base_msg.h |   37 +
>   include/linux/firmware/imx/ele_mu_ioctl.h |   52 +
>   10 files changed, 1618 insertions(+)
>   create mode 100644 Documentation/ABI/testing/se-cdev
>   create mode 100644 drivers/firmware/imx/ele_base_msg.c
>   create mode 100644 drivers/firmware/imx/ele_common.c
>   create mode 100644 drivers/firmware/imx/ele_common.h
>   create mode 100644 drivers/firmware/imx/se_fw.c
>   create mode 100644 drivers/firmware/imx/se_fw.h
>   create mode 100644 include/linux/firmware/imx/ele_base_msg.h
>   create mode 100644 include/linux/firmware/imx/ele_mu_ioctl.h
...
> +
> +int ele_get_info(struct device *dev, phys_addr_t addr, u32 data_size)
> +{
> +	struct ele_mu_priv *priv = dev_get_drvdata(dev);
> +	int ret;
> +	unsigned int tag, command, size, ver, status;
> +
> +	ret = plat_fill_cmd_msg_hdr(priv,
> +				    (struct mu_hdr *)&priv->tx_msg.header,
> +				    ELE_GET_INFO_REQ, 16);
> +	if (ret)
> +		return ret;
> +
> +	priv->tx_msg.data[0] = upper_32_bits(addr);
> +	priv->tx_msg.data[1] = lower_32_bits(addr);
> +	priv->tx_msg.data[2] = data_size;
> +	ret = imx_ele_msg_send_rcv(priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	tag = MSG_TAG(priv->rx_msg.header);
> +	command = MSG_COMMAND(priv->rx_msg.header);
> +	size = MSG_SIZE(priv->rx_msg.header);
> +	ver = MSG_VER(priv->rx_msg.header);
> +	status = RES_STATUS(priv->rx_msg.data[0]);
> +	if (tag == priv->rsp_tag &&
> +	    command == ELE_GET_INFO_REQ &&
> +	    size == ELE_GET_INFO_REQ_MSG_SZ &&
> +	    ver == ELE_BASE_API_VERSION &&
> +	    status == priv->success_tag)
> +		return 0;
except of the coding style, i won't recommend this error handling. In
case a user report a failure of ele_get_info(), we need to figure out
which of these conditions failed. Why not check the conditions step by
step and give a detailed error message.

The same applies to the rest of the series.

Best regards
> +
> +	return -EINVAL;
> +}
>