| 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
| ||
|
Message-ID: <74df918148be8c9820acc877e39adf3f@walle.cc>
Date: Tue, 02 Mar 2021 17:19:55 +0100
From: Michael Walle <michael@...le.cc>
To: Vignesh Raghavendra <vigneshr@...com>
Cc: linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-api@...r.kernel.org,
Miquel Raynal <miquel.raynal@...tlin.com>,
Richard Weinberger <richard@....at>,
Tudor.Ambarus@...rochip.com
Subject: Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
Am 2021-03-02 16:30, schrieb Vignesh Raghavendra:
> Hi,
>
> On 3/2/21 4:39 PM, Michael Walle wrote:
>> This may sound like a contradiction but some SPI-NOR flashes really
>> support erasing their OTP region until it is finally locked. Having
>> the
>> possibility to erase an OTP region might come in handy during
>> development.
>>
>> The ioctl argument follows the OTPLOCK style.
>>
>> Signed-off-by: Michael Walle <michael@...le.cc>
>> ---
>> OTP support for SPI-NOR flashes may be merged soon:
>> https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/
>>
>> Tudor suggested to add support for the OTP erase operation most
>> SPI-NOR
>> flashes have:
>> https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/
>>
>> Therefore, this is an RFC to get some feedback on the MTD side, once
>> this
>> is finished, I can post a patch for mtd-utils. Then we'll have a
>> foundation
>> to add the support to SPI-NOR.
>>
>> drivers/mtd/mtdchar.c | 7 ++++++-
>> drivers/mtd/mtdcore.c | 12 ++++++++++++
>> include/linux/mtd/mtd.h | 3 +++
>> include/uapi/mtd/mtd-abi.h | 2 ++
>> 4 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
>> index 323035d4f2d0..da423dd031ae 100644
>> --- a/drivers/mtd/mtdchar.c
>> +++ b/drivers/mtd/mtdchar.c
>> @@ -661,6 +661,7 @@ static int mtdchar_ioctl(struct file *file, u_int
>> cmd, u_long arg)
>> case OTPGETREGIONCOUNT:
>> case OTPGETREGIONINFO:
>> case OTPLOCK:
>> + case OTPERASE:
>
> This is not a Safe IOCTL. We are destroying OTP data. Need to check for
> write permission before allowing the ioctl right?
Ah yes, of course. But this makes me wonder why OTPLOCK
is considered a safe command. As well as MEMLOCK and
MEMUNLOCK. And MEMSETBADBLOCK. Shouldn't these also
require write permissions?
>> case ECCGETLAYOUT:
>> case ECCGETSTATS:
>> case MTDFILEMODE:
>> @@ -938,6 +939,7 @@ static int mtdchar_ioctl(struct file *file, u_int
>> cmd, u_long arg)
>> }
>>
>> case OTPLOCK:
>> + case OTPERASE:
>> {
>> struct otp_info oinfo;
>>
>> @@ -945,7 +947,10 @@ static int mtdchar_ioctl(struct file *file, u_int
>> cmd, u_long arg)
>> return -EINVAL;
>> if (copy_from_user(&oinfo, argp, sizeof(oinfo)))
>> return -EFAULT;
>> - ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
>> + if (cmd == OTPLOCK)
>> + ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
>> + else
>> + ret = mtd_erase_user_prot_reg(mtd, oinfo.start, oinfo.length);
>> break;
>> }
>>
>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>> index 2d6423d89a17..d844d718ef77 100644
>> --- a/drivers/mtd/mtdcore.c
>> +++ b/drivers/mtd/mtdcore.c
>> @@ -1918,6 +1918,18 @@ int mtd_lock_user_prot_reg(struct mtd_info
>> *mtd, loff_t from, size_t len)
>> }
>> EXPORT_SYMBOL_GPL(mtd_lock_user_prot_reg);
>>
>> +int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t
>> len)
>> +{
>> + struct mtd_info *master = mtd_get_master(mtd);
>> +
>> + if (!master->_erase_user_prot_reg)
>> + return -EOPNOTSUPP;
>> + if (!len)
>> + return 0;
>> + return master->_erase_user_prot_reg(master, from, len);
>> +}
>> +EXPORT_SYMBOL_GPL(mtd_erase_user_prot_reg);
>> +
>> /* Chip-supported device locking */
>> int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> {
>> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
>> index 157357ec1441..734ad7a8c353 100644
>> --- a/include/linux/mtd/mtd.h
>> +++ b/include/linux/mtd/mtd.h
>> @@ -336,6 +336,8 @@ struct mtd_info {
>> size_t len, size_t *retlen, u_char *buf);
>> int (*_lock_user_prot_reg) (struct mtd_info *mtd, loff_t from,
>> size_t len);
>> + int (*_erase_user_prot_reg) (struct mtd_info *mtd, loff_t from,
>> + size_t len);
>> int (*_writev) (struct mtd_info *mtd, const struct kvec *vecs,
>> unsigned long count, loff_t to, size_t *retlen);
>> void (*_sync) (struct mtd_info *mtd);
>> @@ -517,6 +519,7 @@ int mtd_read_user_prot_reg(struct mtd_info *mtd,
>> loff_t from, size_t len,
>> int mtd_write_user_prot_reg(struct mtd_info *mtd, loff_t to, size_t
>> len,
>> size_t *retlen, u_char *buf);
>> int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t
>> len);
>> +int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t
>> len);
>>
>> int mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
>> unsigned long count, loff_t to, size_t *retlen);
>> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
>> index 65b9db936557..242015f60d10 100644
>> --- a/include/uapi/mtd/mtd-abi.h
>> +++ b/include/uapi/mtd/mtd-abi.h
>> @@ -205,6 +205,8 @@ struct otp_info {
>> * without OOB, e.g., NOR flash.
>> */
>> #define MEMWRITE _IOWR('M', 24, struct mtd_write_req)
>> +/* Erase a given range of user data (must be in mode
>> %MTD_FILE_MODE_OTP_USER) */
>> +#define OTPERASE _IOR('M', 25, struct otp_info)
>>
>
> Hmm, shouldn't this be:
>
> #define OTPERASE _IOW('M', 25, struct otp_info)
>
> Userspace is writing struct otp_info to the driver. OTPLOCK should
> probably be _IOW() as well.
You're right.
NB. most OTP commands have a wrong direction flag.
>> /*
>> * Obsolete legacy interface. Keep it in order not to break userspace
>>
-michael
Powered by blists - more mailing lists