Re: [PATCH 2/4] mtd: spi-nor: add advanced protection and security features support

[Paul Barker <paul.barker@sancloud.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 9258 bytes --]

On 27/10/2021 11:33, shiva.linuxworks@gmail.com wrote:
> From: Shivamurthy Shastri <sshivamurthy@micron.com>
> 
> Added functionalities to support advanced securtiy and protection
> features in new SPI NOR flashes.
> 
> Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> ---
>   drivers/mtd/spi-nor/Makefile     |   2 +-
>   drivers/mtd/spi-nor/advprotsec.c | 209 +++++++++++++++++++++++++++++++
>   drivers/mtd/spi-nor/core.c       |   2 +
>   include/linux/mtd/mtd.h          |  19 +++
>   4 files changed, 231 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/mtd/spi-nor/advprotsec.c

The changes to drivers/mtd/spi-nor/core.h in patch 1 of this series can 
be merged into this patch, with the series re-ordered so this patch is 
first.

> 
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 6b904e439372..8e96e2c65c7a 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -1,6 +1,6 @@
>   # SPDX-License-Identifier: GPL-2.0
>   
> -spi-nor-objs			:= core.o sfdp.o swp.o otp.o sysfs.o
> +spi-nor-objs			:= core.o sfdp.o swp.o otp.o advprotsec.o sysfs.o
>   spi-nor-objs			+= atmel.o
>   spi-nor-objs			+= catalyst.o
>   spi-nor-objs			+= eon.o
> diff --git a/drivers/mtd/spi-nor/advprotsec.c b/drivers/mtd/spi-nor/advprotsec.c
> new file mode 100644
> index 000000000000..4dc8e67b16ef
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/advprotsec.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SPI NOR Advanced Sector Protection and Security Features
> + *
> + * Copyright (C) 2021 Micron Technology, Inc.
> + */
> +
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/spi-nor.h>
> +
> +#include "core.h"
> +
> +static int spi_nor_secure_read(struct mtd_info *mtd, size_t len, u8 *buf)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	int ret;
> +
> +	ret = spi_nor_lock_and_prep(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = nor->params->sec_ops->secure_read(nor, len, buf);
> +
> +	spi_nor_unlock_and_unprep(nor);
> +	return ret;
> +}
> +
> +static int spi_nor_secure_write(struct mtd_info *mtd, size_t len, u8 *buf)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	int ret;
> +
> +	ret = spi_nor_lock_and_prep(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = nor->params->sec_ops->secure_write(nor, len, buf);
> +
> +	spi_nor_unlock_and_unprep(nor);
> +	return ret;
> +}
> +
> +static int spi_nor_read_vlock_bits(struct mtd_info *mtd, u32 addr, size_t len,
> +				   u8 *buf)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	int ret;
> +
> +	ret = spi_nor_lock_and_prep(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = nor->params->sec_ops->read_vlock_bits(nor, addr, len, buf);
> +
> +	spi_nor_unlock_and_unprep(nor);
> +	return ret;
> +}
> +
> +static int spi_nor_write_vlock_bits(struct mtd_info *mtd, u32 addr, size_t len,
> +				    u8 *buf)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	int ret;
> +
> +	ret = spi_nor_lock_and_prep(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = spi_nor_write_enable(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = nor->params->sec_ops->write_vlock_bits(nor, addr, len, buf);
> +	if (ret)
> +		return ret;
> +
> +	ret = spi_nor_write_disable(nor);
> +
> +	spi_nor_unlock_and_unprep(nor);
> +	return ret;
> +}
> +
> +static int spi_nor_read_nvlock_bits(struct mtd_info *mtd, u32 addr, size_t len,
> +				    u8 *buf)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	int ret;
> +
> +	ret = spi_nor_lock_and_prep(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = nor->params->sec_ops->read_nvlock_bits(nor, addr, len, buf);
> +
> +	spi_nor_unlock_and_unprep(nor);
> +	return ret;
> +}
> +
> +static int spi_nor_write_nvlock_bits(struct mtd_info *mtd, u32 addr)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	int ret;
> +
> +	ret = spi_nor_lock_and_prep(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = spi_nor_write_enable(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = nor->params->sec_ops->write_nvlock_bits(nor, addr);
> +	if (ret)
> +		return ret;
> +
> +	ret = spi_nor_write_disable(nor);
> +
> +	spi_nor_unlock_and_unprep(nor);
> +	return ret;
> +}
> +
> +static int spi_nor_erase_nvlock_bits(struct mtd_info *mtd)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	int ret;
> +
> +	ret = spi_nor_lock_and_prep(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = spi_nor_write_enable(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = nor->params->sec_ops->erase_nvlock_bits(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = spi_nor_write_disable(nor);
> +
> +	spi_nor_unlock_and_unprep(nor);
> +	return ret;
> +}
> +
> +static int spi_nor_read_global_freeze_bits(struct mtd_info *mtd, size_t len,
> +					   u8 *buf)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	int ret;
> +
> +	ret = spi_nor_lock_and_prep(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = nor->params->sec_ops->read_global_freeze_bits(nor, len, buf);
> +
> +	spi_nor_unlock_and_unprep(nor);
> +	return ret;
> +}
> +
> +static int spi_nor_write_global_freeze_bits(struct mtd_info *mtd, size_t len,
> +					    u8 *buf)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	int ret;
> +
> +	ret = spi_nor_lock_and_prep(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = nor->params->sec_ops->write_global_freeze_bits(nor, len, buf);
> +
> +	spi_nor_unlock_and_unprep(nor);
> +	return ret;
> +}
> +
> +static int spi_nor_read_password(struct mtd_info *mtd, size_t len, u8 *buf)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	int ret;
> +
> +	ret = spi_nor_lock_and_prep(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = nor->params->sec_ops->read_password(nor, len, buf);
> +
> +	spi_nor_unlock_and_unprep(nor);
> +	return ret;
> +}
> +
> +void spi_nor_register_security_ops(struct spi_nor *nor)
> +{
> +	struct mtd_info *mtd = &nor->mtd;
> +
> +	if (!nor->params->sec_ops)
> +		return;
> +
> +	mtd->_secure_packet_read = spi_nor_secure_read;
> +	mtd->_secure_packet_write = spi_nor_secure_write;
> +	mtd->_read_vlock_bits = spi_nor_read_vlock_bits;
> +	mtd->_write_vlock_bits = spi_nor_write_vlock_bits;
> +	mtd->_read_nvlock_bits = spi_nor_read_nvlock_bits;
> +	mtd->_write_nvlock_bits = spi_nor_write_nvlock_bits;
> +	mtd->_erase_nvlock_bits = spi_nor_erase_nvlock_bits;
> +	mtd->_read_global_freeze_bits = spi_nor_read_global_freeze_bits;
> +	mtd->_write_global_freeze_bits = spi_nor_write_global_freeze_bits;
> +	mtd->_read_password = spi_nor_read_password;

This approach requires all or none of the sec_ops functions to be 
implemented. It doesn't consider other drivers which may be able to 
implement a subset of the sec_ops functions.

I also think it would be better not to use extra function pointers here 
and just let other code call the functions defined above directly. The 
caller of these functions will need to check that the pointers aren't 
NULL before calling them anyway, so I think we may as well call the 
functions directly and have each of them check that the corresponding 
sec_ops field is non-NULL before calling it.

> +}
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index cc08bd707378..864f3c7783b3 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3199,6 +3199,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>   
>   	spi_nor_register_locking_ops(nor);
>   
> +	spi_nor_register_security_ops(nor);
> +
>   	/* Send all the required SPI flash commands to initialize device */
>   	ret = spi_nor_init(nor);
>   	if (ret)
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 88227044fc86..bce358c9fb94 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -360,6 +360,25 @@ struct mtd_info {
>   	int (*_get_device) (struct mtd_info *mtd);
>   	void (*_put_device) (struct mtd_info *mtd);
>   
> +	/*
> +	 * Security Operations
> +	 */
> +	int (*_secure_packet_read)(struct mtd_info *mtd, size_t len, u8 *buf);
> +	int (*_secure_packet_write)(struct mtd_info *mtd, size_t len, u8 *buf);
> +	int (*_read_vlock_bits)(struct mtd_info *mtd, u32 addr, size_t len,
> +				u8 *buf);
> +	int (*_write_vlock_bits)(struct mtd_info *mtd, u32 addr, size_t len,
> +				 u8 *buf);
> +	int (*_read_nvlock_bits)(struct mtd_info *mtd, u32 addr, size_t len,
> +				 u8 *buf);
> +	int (*_write_nvlock_bits)(struct mtd_info *mtd, u32 addr);
> +	int (*_erase_nvlock_bits)(struct mtd_info *mtd);
> +	int (*_read_global_freeze_bits)(struct mtd_info *mtd, size_t len,
> +					u8 *buf);
> +	int (*_write_global_freeze_bits)(struct mtd_info *mtd, size_t len,
> +					 u8 *buf);
> +	int (*_read_password)(struct mtd_info *mtd, size_t len, u8 *buf);
> +
>   	/*
>   	 * flag indicates a panic write, low level drivers can take appropriate
>   	 * action if required to ensure writes go through
> 

Thanks,

-- 
Paul Barker
Principal Software Engineer
SanCloud Ltd

e: paul.barker@sancloud.com
w: https://sancloud.co.uk/

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7643 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]