Re: [PATCH 3/4] mtd: add advanced protection and security ioctls

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

On 27/10/2021 11:33, shiva.linuxworks@gmail.com wrote:
> From: Shivamurthy Shastri <sshivamurthy@micron.com>
> 
> Added new ioctls for advanced protection and security features.
> These features are currently supported by new Micron SPI NOR flashes.
> 
> Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> ---
>   drivers/mtd/mtdchar.c      | 145 +++++++++++++++++++++++++++++++++++++
>   include/uapi/mtd/mtd-abi.h |  11 +++
>   2 files changed, 156 insertions(+)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 155e991d9d75..97b97b80276d 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -654,6 +654,16 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>   	case MTDFILEMODE:
>   	case BLKPG:
>   	case BLKRRPART:
> +	case SECURE_PACKET_READ:
> +	case SECURE_PACKET_WRITE:
> +	case RD_VLOCK_BITS:
> +	case WR_VLOCK_BITS:
> +	case RD_NVLOCK_BITS:
> +	case WR_NVLOCK_BITS:
> +	case ER_NVLOCK_BITS:
> +	case RD_GLOBAL_FREEZE_BITS:
> +	case WR_GLOBAL_FREEZE_BITS:
> +	case RD_PASSWORD:
>   		break;

It looks like you've listed all of the ioctls as "safe" commands so the 
write permission bit is not checked. My understanding is that all ioctls 
which may modify the data in flash need moving to the "dangerous" 
commands section below this so that the write permission bit is checked.

>   
>   	/* "dangerous" commands */
> @@ -1017,6 +1027,141 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>   		ret = 0;
>   		break;
>   	}
> +	case SECURE_PACKET_READ:
> +	{
> +		struct mtd_oob_buf buf;
> +		u8 *oobbuf;
> +
> +		if (copy_from_user(&buf, argp, sizeof(buf)))
> +			ret = -EFAULT;
> +
> +		oobbuf = kmalloc(buf.length, GFP_KERNEL);
> +		ret = master->_secure_packet_read(master, buf.length, oobbuf);
> +		if (copy_to_user(buf.ptr, oobbuf, buf.length))
> +			ret = -EFAULT;
> +		break;
> +	}
> +
> +	case SECURE_PACKET_WRITE:
> +	{
> +		struct mtd_oob_buf buf;
> +		u8 *oobbuf;
> +
> +		if (copy_from_user(&buf, argp, sizeof(buf)))
> +			ret = -EFAULT;
> +
> +		oobbuf = memdup_user(buf.ptr, buf.length);
> +		ret = master->_secure_packet_write(master, buf.length, oobbuf);
> +		break;
> +	}
> +
> +	case RD_VLOCK_BITS:
> +	{
> +		struct mtd_oob_buf buf;
> +		u8 *oobbuf;
> +
> +		if (copy_from_user(&buf, argp, sizeof(buf)))
> +			ret = -EFAULT;
> +
> +		oobbuf = kmalloc(buf.length, GFP_KERNEL);
> +		ret = master->_read_vlock_bits(master, buf.start, buf.length,
> +					       oobbuf);
> +		if (copy_to_user(buf.ptr, oobbuf, buf.length))
> +			ret = -EFAULT;
> +		break;
> +	}
> +
> +	case WR_VLOCK_BITS:
> +	{
> +		struct mtd_oob_buf buf;
> +		u8 *oobbuf;
> +
> +		if (copy_from_user(&buf, argp, sizeof(buf)))
> +			ret = -EFAULT;
> +
> +		oobbuf = memdup_user(buf.ptr, buf.length);
> +		ret = master->_write_vlock_bits(master, buf. start, buf.length,
> +						oobbuf);
> +		break;
> +	}
> +
> +	case RD_NVLOCK_BITS:
> +	{
> +		struct mtd_oob_buf buf;
> +		u8 *oobbuf;
> +
> +		if (copy_from_user(&buf, argp, sizeof(buf)))
> +			ret = -EFAULT;
> +
> +		oobbuf = kmalloc(buf.length, GFP_KERNEL);
> +		ret = master->_read_nvlock_bits(master, buf.start, buf.length,
> +						oobbuf);
> +		if (copy_to_user(buf.ptr, oobbuf, buf.length))
> +			ret = -EFAULT;
> +		break;
> +	}
> +
> +	case WR_NVLOCK_BITS:
> +	{
> +		struct mtd_oob_buf buf;
> +
> +		if (copy_from_user(&buf, argp, sizeof(buf)))
> +			ret = -EFAULT;
> +
> +		ret = master->_write_nvlock_bits(master, buf.start);
> +		break;
> +	}
> +
> +	case ER_NVLOCK_BITS:
> +	{
> +		ret = master->_erase_nvlock_bits(master);
> +		break;
> +	}
> +
> +	case RD_GLOBAL_FREEZE_BITS:
> +	{
> +		struct mtd_oob_buf buf;
> +		u8 *oobbuf;
> +
> +		if (copy_from_user(&buf, argp, sizeof(buf)))
> +			ret = -EFAULT;
> +
> +		oobbuf = kmalloc(buf.length, GFP_KERNEL);
> +		ret = master->_read_global_freeze_bits(master, buf.length,
> +						       oobbuf);
> +		if (copy_to_user(buf.ptr, oobbuf, buf.length))
> +			ret = -EFAULT;
> +		break;
> +	}
> +
> +	case WR_GLOBAL_FREEZE_BITS:
> +	{
> +		struct mtd_oob_buf buf;
> +		u8 *oobbuf;
> +
> +		if (copy_from_user(&buf, argp, sizeof(buf)))
> +			ret = -EFAULT;
> +
> +		oobbuf = memdup_user(buf.ptr, buf.length);
> +		ret = master->_write_global_freeze_bits(master, buf.length,
> +							oobbuf);
> +		break;
> +	}
> +
> +	case RD_PASSWORD:
> +	{
> +		struct mtd_oob_buf buf;
> +		u8 *oobbuf;
> +
> +		if (copy_from_user(&buf, argp, sizeof(buf)))
> +			ret = -EFAULT;
> +
> +		oobbuf = kmalloc(buf.length, GFP_KERNEL);
> +		ret = master->_read_password(master, buf.length, oobbuf);
> +		if (copy_to_user(buf.ptr, oobbuf, buf.length))
> +			ret = -EFAULT;
> +		break;
> +	}
>   	}
>   
>   	return ret;
> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
> index b869990c2db2..dbd7bf60d484 100644
> --- a/include/uapi/mtd/mtd-abi.h
> +++ b/include/uapi/mtd/mtd-abi.h
> @@ -208,6 +208,17 @@ struct otp_info {
>   /* Erase a given range of user data (must be in mode %MTD_FILE_MODE_OTP_USER) */
>   #define OTPERASE		_IOW('M', 25, struct otp_info)
>   
> +#define SECURE_PACKET_READ	_IOWR('M', 26, struct mtd_oob_buf)
> +#define SECURE_PACKET_WRITE	_IOWR('M', 27, struct mtd_oob_buf)
> +#define RD_VLOCK_BITS		_IOWR('M', 28, struct mtd_oob_buf)
> +#define WR_VLOCK_BITS		_IOWR('M', 29, struct mtd_oob_buf)
> +#define RD_NVLOCK_BITS		_IOWR('M', 30, struct mtd_oob_buf)
> +#define WR_NVLOCK_BITS		_IOWR('M', 31, struct mtd_oob_buf)
> +#define ER_NVLOCK_BITS		_IO('M', 32)
> +#define RD_GLOBAL_FREEZE_BITS	_IOWR('M', 33, struct mtd_oob_buf)
> +#define WR_GLOBAL_FREEZE_BITS	_IOWR('M', 34, struct mtd_oob_buf)
> +#define RD_PASSWORD		_IOWR('M', 35, struct mtd_oob_buf)
> +

All other ioctls defined in this header are preceeded by a comment which 
briefly explains what they do. I think this is needed for these new 
ioctls as well.

>   /*
>    * Obsolete legacy interface. Keep it in order not to break userspace
>    * interfaces
> 

Thanks,

-- 
Paul Barker
Principal Software Engineer
SanCloud Ltd

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