linux-kernel - Re: [PATCH v5 09/11] firmware: imx: enclave-fw: add handling for save/restore IMEM region

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: <41581c90-3ee0-c969-2b54-ee512d637788@linaro.org>
Date:   Thu, 24 Aug 2023 20:37:01 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
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
Subject: Re: [PATCH v5 09/11] firmware: imx: enclave-fw: add handling for
 save/restore IMEM region

On 23/08/2023 09:33, Pankaj Gupta wrote:
> Some IMEM region is lost during kernel power down. Due to this,
> firmware's functionaity cannot work correctly.
> 
> Saving encrypted IMEM region in kernel memory during power down,
> and restore IMEM region on resume.
> 
> Signed-off-by: Gaurav Jain <gaurav.jain@....com>
> Signed-off-by: Pankaj Gupta <pankaj.gupta@....com>


> @@ -959,6 +962,17 @@ static int se_probe_cleanup(struct platform_device *pdev)
>  		priv->flags &= (~RESERVED_DMA_POOL);
>  	}
>  
> +	/* free the buffer in ele-mu remove, previously allocated
> +	 * in ele-mu probe to store encrypted IMEM
> +	 */

Use Linux coding style comments.

> +	if (priv->imem.buf) {
> +		dmam_free_coherent(&pdev->dev,
> +				   ELE_IMEM_SIZE,
> +				   priv->imem.buf,
> +				   priv->imem.phyaddr);
> +		priv->imem.buf = NULL;
> +	}
> +
>  	if (priv->ctxs) {
>  		for (i = 0; i < priv->max_dev_ctx; i++) {
>  			if (priv->ctxs[i])
> @@ -1160,6 +1174,19 @@ static int se_fw_probe(struct platform_device *pdev)
>  			dev_err(dev, "Failed to init ele-trng\n");
>  	}
>  
> +	if (info->imem_mgmt) {
> +		/* allocate buffer where ELE store encrypted IMEM */
> +		priv->imem.buf = dmam_alloc_coherent(dev, ELE_IMEM_SIZE,
> +						     &priv->imem.phyaddr,
> +						     GFP_KERNEL);
> +		if (!priv->imem.buf) {
> +			dev_err(dev,
> +				"dmam-alloc-failed: To store encr-IMEM.\n");
> +			ret = -ENOMEM;
> +			goto exit;
> +		}
> +	}
> +
>  	pr_info("i.MX secure-enclave: %s's mu#%d interface to firmware, configured.\n",
>  		info->se_name,
>  		priv->ele_mu_id);
> @@ -1196,17 +1223,31 @@ static int se_fw_remove(struct platform_device *pdev)
>  #ifdef CONFIG_PM_SLEEP
>  static int se_fw_suspend(struct device *dev)
>  {
> +	struct ele_mu_priv *priv = dev_get_drvdata(dev);
> +	const struct of_device_id *of_id = of_match_device(se_fw_match, dev);

No.

> +	struct imx_info *info = (of_id != NULL) ? (struct imx_info *)of_id->data
> +						: NULL;
> +
> +	if (info && info->imem_mgmt)
> +		priv->imem.size = save_imem(dev);
> +
>  	return 0;
>  }
>  
>  static int se_fw_resume(struct device *dev)
>  {
>  	struct ele_mu_priv *priv = dev_get_drvdata(dev);
> +	const struct of_device_id *of_id = of_match_device(se_fw_match, dev);

Why do you keep matching device every time? Don't.

> +	struct imx_info *info = (of_id != NULL) ? (struct imx_info *)of_id->data
> +						: NULL;
>  	int i;
>  
>  	for (i = 0; i < priv->max_dev_ctx; i++)
>  		wake_up_interruptible(&priv->ctxs[i]->wq);
>  
> +	if (info && info->imem_mgmt)
> +		restore_imem(dev, info->pool_name);
> +
>  	return 0;
>  }
>  #endif
> diff --git a/drivers/firmware/imx/se_fw.h b/drivers/firmware/imx/se_fw.h
> index b3502affbc85..acb967f2357c 100644
> --- a/drivers/firmware/imx/se_fw.h
> +++ b/drivers/firmware/imx/se_fw.h
> @@ -165,4 +165,12 @@ struct ele_mu_priv {
>  	struct ele_imem_buf imem;
>  };
>  
> +phys_addr_t get_phy_buf_mem_pool(struct device *dev,
> +				 char *mem_pool_name,
> +				 u32 **buf,
> +				 uint32_t size);
> +void free_phybuf_mem_pool(struct device *dev,
> +			  char *mem_pool_name,
> +			  u32 *buf,
> +			  uint32_t size);
>  #endif
> diff --git a/include/linux/firmware/imx/ele_base_msg.h b/include/linux/firmware/imx/ele_base_msg.h
> index 8a5c385210fc..6fbea7a8d7c9 100644
> --- a/include/linux/firmware/imx/ele_base_msg.h
> +++ b/include/linux/firmware/imx/ele_base_msg.h
> @@ -37,12 +37,23 @@
>  #define ELE_GET_TRNG_STATE_RETRY_COUNT	0x5
>  #define CSAL_TRNG_STATE_MASK		0x0000ffff
>  
> +#define ELE_SERVICE_SWAP_REQ		0xDF
> +#define ELE_SERVICE_SWAP_REQ_MSG_SZ	0x03
> +#define ELE_IMEM_SIZE			0x10000
> +#define ELE_IMEM_STATE_OK		0xCA
> +#define ELE_IMEM_STATE_BAD		0xFE
> +#define ELE_IMEM_STATE_WORD		0x27
> +#define ELE_IMEM_STATE_MASK		0x00ff0000
> +#define ELE_IMEM_EXPORT			0x1
> +#define ELE_IMEM_IMPORT			0x2
> +
>  #define ELE_BASE_API_VERSION		0x6
> -#define ELE_SUCCESS_IND			0xD6
> -#define ELE_FAILURE_IND			0x29

No, you just added them. Don't add wrong code to remove it in next patch.

Best regards,
Krzysztof