Messages in this thread | | | Date | Mon, 6 Dec 2021 11:13:02 +0000 | Subject | Re: [PATCH 3/4] mtd: add advanced protection and security ioctls | From | Paul Barker <> |
| |
Apologies for the double-reply, spotted another thing I'd like to comment on...
On 06/12/2021 10:42, Paul Barker wrote: > 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);
After reviewing patch 2 in this series it's clear that these function pointers are NULL if a driver does not implement the sec_ops functions. So unless the implementation there is changed, each of these calls needs to be wrapped in a test to see if the corresponding pointer is valid and to return -EOPNOTSUPP if it is NULL.
>> + 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/ [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |