[PATCH] mtdcore: add writeable attribute in sysfs

[PATCH] mtdcore: add writeable attribute in sysfs

[Charles-Antoine Couret]

MTD devices read-write or read-only property could be set by the bootloader with
the mtdparts argument but there is no way to change this definition at runtime
on Linux. The flags can be read but can not be changed.

This patch adds the possibility to force a MTD device to be read-only or read-write
even if the bootloader requested something else. The purpose is to offer to
the user of a device to eventually change something that is not allowed
like updating the bootloader or change its environement.

Or to protect the device without using mtdro virtual device to read the content.
---
 drivers/mtd/mtdcore.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 807d17d863b3..3a9a7055ee53 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -158,6 +158,39 @@ static ssize_t mtd_flags_show(struct device *dev,
 }
 static DEVICE_ATTR(flags, S_IRUGO, mtd_flags_show, NULL);
 
+static ssize_t mtd_writeable_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+	unsigned int writeable = mtd->flags & MTD_WRITEABLE ? 1 : 0;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", writeable);
+
+}
+
+static ssize_t mtd_writeable_store(struct device *dev,
+					   struct device_attribute *attr,
+					   const char *buf, size_t count)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+	unsigned int writeable;
+	int retval;
+
+
+	retval = kstrtouint(buf, 0, &writeable);
+	if (retval)
+		return retval;
+
+	if (writeable)
+		mtd->flags |= MTD_WRITEABLE;
+	else
+		mtd->flags &= ~MTD_WRITEABLE;
+
+	return count;
+}
+static DEVICE_ATTR(writeable, S_IRUGO | S_IWUSR, mtd_writeable_show,
+	mtd_writeable_store);
+
 static ssize_t mtd_size_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
@@ -322,6 +355,7 @@ static DEVICE_ATTR(bbt_blocks, S_IRUGO, mtd_bbtblocks_show, NULL);
 static struct attribute *mtd_attrs[] = {
 	&dev_attr_type.attr,
 	&dev_attr_flags.attr,
+	&dev_attr_writeable.attr,
 	&dev_attr_size.attr,
 	&dev_attr_erasesize.attr,
 	&dev_attr_writesize.attr,
-- 
2.17.0

Re: [PATCH] mtdcore: add writeable attribute in sysfs

[Boris Brezillon]

Hi Charles-Antoine,

On Thu, 12 Apr 2018 15:42:32 +0200
Charles-Antoine Couret <charles-antoine.couret@essensium.com> wrote:

> MTD devices read-write or read-only property could be set by the bootloader with
> the mtdparts argument but there is no way to change this definition at runtime
> on Linux. The flags can be read but can not be changed.
> 
> This patch adds the possibility to force a MTD device to be read-only or read-write
> even if the bootloader requested something else. The purpose is to offer to
> the user of a device to eventually change something that is not allowed
> like updating the bootloader or change its environement.

Hm, why don't you remove the RO constraint on such partitions then? I
mean, if the device can be written, even rarely, it's no longer a
read-only device.

> 
> Or to protect the device without using mtdro virtual device to read the content.

What's the point? Only root can access /dev/mtdX. Looks like you want
to protect against inadvertent write to the wrong MTD dev, but I'm not
sure this is the kernel responsibility to enforce that.

Missing Signed-off-by tag.

> ---
>  drivers/mtd/mtdcore.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 807d17d863b3..3a9a7055ee53 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -158,6 +158,39 @@ static ssize_t mtd_flags_show(struct device *dev,
>  }
>  static DEVICE_ATTR(flags, S_IRUGO, mtd_flags_show, NULL);
>  
> +static ssize_t mtd_writeable_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)

Please align parameters on the open-parenthesis.

> +{
> +	struct mtd_info *mtd = dev_get_drvdata(dev);
> +	unsigned int writeable = mtd->flags & MTD_WRITEABLE ? 1 : 0;
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", writeable);
> +
> +}
> +
> +static ssize_t mtd_writeable_store(struct device *dev,
> +					   struct device_attribute *attr,
> +					   const char *buf, size_t count)
> +{
> +	struct mtd_info *mtd = dev_get_drvdata(dev);
> +	unsigned int writeable;
> +	int retval;
> +
> +
> +	retval = kstrtouint(buf, 0, &writeable);
> +	if (retval)
> +		return retval;
> +
> +	if (writeable)
> +		mtd->flags |= MTD_WRITEABLE;
> +	else
> +		mtd->flags &= ~MTD_WRITEABLE;

What happens if you turn a RW device into a RO while it's still being
used by someone? Not sure at all this is safe.

Regards,

Boris

> +
> +	return count;
> +}
> +static DEVICE_ATTR(writeable, S_IRUGO | S_IWUSR, mtd_writeable_show,
> +	mtd_writeable_store);
> +
>  static ssize_t mtd_size_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> @@ -322,6 +355,7 @@ static DEVICE_ATTR(bbt_blocks, S_IRUGO, mtd_bbtblocks_show, NULL);
>  static struct attribute *mtd_attrs[] = {
>  	&dev_attr_type.attr,
>  	&dev_attr_flags.attr,
> +	&dev_attr_writeable.attr,
>  	&dev_attr_size.attr,
>  	&dev_attr_erasesize.attr,
>  	&dev_attr_writesize.attr,

Re: [PATCH] mtdcore: add writeable attribute in sysfs

[Couret Charles-Antoine]

Le 20/04/2018 à 21:43, Boris Brezillon a écrit :
> Hi Charles-Antoine,
Hi,
> Hm, why don't you remove the RO constraint on such partitions then? I
> mean, if the device can be written, even rarely, it's no longer a
> read-only device.
Sometimes the partition is set as read-only but it is not the case in 
reality, it is only a software flag.
 From my experience, it is often to protect the bootloader to be changed 
from Linux system. To avoid corruption (or to get full control)
>> Or to protect the device without using mtdro virtual device to read the content.
> What's the point? Only root can access /dev/mtdX. Looks like you want
> to protect against inadvertent write to the wrong MTD dev, but I'm not
> sure this is the kernel responsibility to enforce that.
Yes the idea is to keep (or to set) this additional protection and 
having a way to by-pass only when it is relevant (to make an update or 
to do something special).
I am not sure that is a big deal to add this change on the kernel, the 
code is very light and simple.
> Missing Signed-off-by tag.
Thanks, I will add that.
> Please align parameters on the open-parenthesis.
Thanks, sorry for this issue.
>
> What happens if you turn a RW device into a RO while it's still being
> used by someone? Not sure at all this is safe.
>
> Regards,
>
> Boris
The writeable flag is checked before the write operation. If this flag 
is cleared while a process is writing something, the write operation can 
be completed. But after this completion, no other write operations are 
allowed.

I will make some tests tomorrow to check this part. But normally that 
should not be an issue.
Regards,
Charles-Antoine Couret