[PATCH RFC 1/2] dt-bindings: mtd: ubi-volume: add 'volume-is-critical' property

[PATCH RFC 1/2] dt-bindings: mtd: ubi-volume: add 'volume-is-critical' property

[Daniel Golle]

Add the 'volume-is-critical' boolean property which marks a UBI volume
as critical for the device to boot. If set it prevents the user from
all kinds of write access to the volume as well as from renaming it or
detaching the UBI device it is located on.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 .../devicetree/bindings/mtd/partitions/ubi-volume.yaml   | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
index 19736b26056b..2bd751bb7f9e 100644
--- a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
+++ b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
@@ -29,6 +29,15 @@ properties:
     description:
       This container may reference an NVMEM layout parser.
 
+  volume-is-critical:
+    description: This parameter, if present, indicates that the UBI volume
+      contains early-boot firmware images or data which should not be clobbered.
+      If set, it prevents the user from renaming the volume, writing to it or
+      making any changes affecting it, as well as detaching the UBI device it is
+      located on, so direct access to the underlying MTD device is prevented as
+      well.
+    type: boolean
+
 anyOf:
   - required:
       - volid
-- 
2.46.2

[PATCH RFC 2/2] mtd: ubi: add support for protecting critical volumes

[Daniel Golle]

Allow the boot firmware to define volumes which are critical for the
system to boot, such as the bootloader itself if stored inside a UBI
volume. Protect critical volumes by preventing the user from removing,
resizing or writing to them, and also prevent the UBI device from
being detached if a critical volume is present.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/mtd/ubi/build.c | 29 +++++++++++++++++++++++++++++
 drivers/mtd/ubi/cdev.c  | 33 +++++++++++++++++++++++++++++++++
 drivers/mtd/ubi/ubi.h   |  1 +
 drivers/mtd/ubi/vmt.c   | 27 ++++++++++++++++++++++++++-
 4 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 30be4ed68fad..7a96329c5fd9 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -314,6 +314,30 @@ struct ubi_device *ubi_get_by_major(int major)
 	return NULL;
 }
 
+/**
+ * ubi_device_got_critical_volume - check if device contains critical volume
+ * @ubi: UBI device description object
+ *
+ * This function checks if any volume on a given UBI device is critical.
+ *
+ * Context: Expects ubi_devices_lock to be held by caller.
+ * Returns: True if there is a critical volume, false otherwise.
+ */
+static bool ubi_device_got_critical_volume(struct ubi_device *ubi)
+{
+	int i;
+
+	for (i = 0; i < ubi->vtbl_slots; i++) {
+		if (!ubi->volumes[i])
+			continue;
+
+		if (ubi->volumes[i]->critical)
+			return true;
+	}
+
+	return false;
+}
+
 /**
  * ubi_major2num - get UBI device number by character device major number.
  * @major: major number
@@ -1102,6 +1126,11 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
 		return -EINVAL;
 
 	spin_lock(&ubi_devices_lock);
+	if (ubi_device_got_critical_volume(ubi)) {
+		spin_unlock(&ubi_devices_lock);
+		return -EPERM;
+	}
+
 	ubi->ref_count -= 1;
 	if (ubi->ref_count) {
 		if (!anyway) {
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index 0d8f04cf03c5..897791ebb71c 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -328,6 +328,9 @@ static ssize_t vol_cdev_write(struct file *file, const char __user *buf,
 	struct ubi_volume *vol = desc->vol;
 	struct ubi_device *ubi = vol->ubi;
 
+	if (vol->critical)
+		return -EPERM;
+
 	if (!vol->updating && !vol->changing_leb)
 		return vol_cdev_direct_write(file, buf, count, offp);
 
@@ -390,6 +393,11 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,
 	{
 		int64_t bytes, rsvd_bytes;
 
+		if (vol->critical) {
+			err = -EPERM;
+			break;
+		}
+
 		if (!capable(CAP_SYS_RESOURCE)) {
 			err = -EPERM;
 			break;
@@ -430,6 +438,11 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,
 	{
 		struct ubi_leb_change_req req;
 
+		if (vol->critical) {
+			err = -EPERM;
+			break;
+		}
+
 		err = copy_from_user(&req, argp,
 				     sizeof(struct ubi_leb_change_req));
 		if (err) {
@@ -464,6 +477,11 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,
 	{
 		int32_t lnum;
 
+		if (vol->critical) {
+			err = -EPERM;
+			break;
+		}
+
 		err = get_user(lnum, (__user int32_t *)argp);
 		if (err) {
 			err = -EFAULT;
@@ -495,6 +513,11 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,
 	{
 		struct ubi_map_req req;
 
+		if (vol->critical) {
+			err = -EPERM;
+			break;
+		}
+
 		err = copy_from_user(&req, argp, sizeof(struct ubi_map_req));
 		if (err) {
 			err = -EFAULT;
@@ -509,6 +532,11 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,
 	{
 		int32_t lnum;
 
+		if (vol->critical) {
+			err = -EPERM;
+			break;
+		}
+
 		err = get_user(lnum, (__user int32_t *)argp);
 		if (err) {
 			err = -EFAULT;
@@ -537,6 +565,11 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,
 	{
 		struct ubi_set_vol_prop_req req;
 
+		if (vol->critical) {
+			err = -EPERM;
+			break;
+		}
+
 		err = copy_from_user(&req, argp,
 				     sizeof(struct ubi_set_vol_prop_req));
 		if (err) {
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 1c9e874e8ede..21b8ce77426b 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -364,6 +364,7 @@ struct ubi_volume {
 	unsigned int updating:1;
 	unsigned int changing_leb:1;
 	unsigned int direct_writes:1;
+	unsigned int critical:1;
 
 #ifdef CONFIG_MTD_UBI_FASTMAP
 	unsigned long *checkmap;
diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
index 5a3558bbb903..a16b84e009a1 100644
--- a/drivers/mtd/ubi/vmt.c
+++ b/drivers/mtd/ubi/vmt.c
@@ -370,6 +370,11 @@ int ubi_remove_volume(struct ubi_volume_desc *desc, int no_vtbl)
 		return -EROFS;
 
 	spin_lock(&ubi->volumes_lock);
+	if (vol->critical) {
+		err = -EPERM;
+		goto out_unlock;
+	}
+
 	if (vol->ref_count > 1) {
 		/*
 		 * The volume is busy, probably someone is reading one of its
@@ -472,6 +477,12 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
 		return PTR_ERR(new_eba_tbl);
 
 	spin_lock(&ubi->volumes_lock);
+	if (vol->critical) {
+		spin_unlock(&ubi->volumes_lock);
+		err = -EPERM;
+		goto out_free;
+	}
+
 	if (vol->ref_count > 1) {
 		spin_unlock(&ubi->volumes_lock);
 		err = -EBUSY;
@@ -578,9 +589,22 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
  */
 int ubi_rename_volumes(struct ubi_device *ubi, struct list_head *rename_list)
 {
-	int err;
+	int err = 0;
 	struct ubi_rename_entry *re;
 
+	spin_lock(&ubi->volumes_lock);
+	list_for_each_entry(re, rename_list, list) {
+		struct ubi_volume *vol = re->desc->vol;
+
+		if (vol->critical) {
+			err = -EPERM;
+			break;
+		}
+	}
+	spin_unlock(&ubi->volumes_lock);
+	if (err)
+		return err;
+
 	err = ubi_vtbl_rename_volumes(ubi, rename_list);
 	if (err)
 		return err;
@@ -641,6 +665,7 @@ int ubi_add_volume(struct ubi_device *ubi, struct ubi_volume *vol)
 	vol->dev.groups = volume_dev_groups;
 	dev_set_name(&vol->dev, "%s_%d", ubi->ubi_name, vol->vol_id);
 	device_set_node(&vol->dev, find_volume_fwnode(vol));
+	vol->critical = device_property_read_bool(&vol->dev, "volume-is-critical");
 	err = device_register(&vol->dev);
 	if (err) {
 		cdev_del(&vol->cdev);
-- 
2.46.2

Re: [PATCH RFC 1/2] dt-bindings: mtd: ubi-volume: add 'volume-is-critical' property

[Krzysztof Kozlowski]

On 28/09/2024 14:47, Daniel Golle wrote:
> Add the 'volume-is-critical' boolean property which marks a UBI volume
> as critical for the device to boot. If set it prevents the user from
> all kinds of write access to the volume as well as from renaming it or
> detaching the UBI device it is located on.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  .../devicetree/bindings/mtd/partitions/ubi-volume.yaml   | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
> index 19736b26056b..2bd751bb7f9e 100644
> --- a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
> +++ b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
> @@ -29,6 +29,15 @@ properties:
>      description:
>        This container may reference an NVMEM layout parser.
>  
> +  volume-is-critical:
> +    description: This parameter, if present, indicates that the UBI volume
> +      contains early-boot firmware images or data which should not be clobbered.
> +      If set, it prevents the user from renaming the volume, writing to it or
> +      making any changes affecting it, as well as detaching the UBI device it is
> +      located on, so direct access to the underlying MTD device is prevented as
> +      well.
> +    type: boolean

UBI volumes are mapping to partitions 1-to-1, right? So rather I would
propose to use partition.yaml - we already have read-only there with
very similar description.

Best regards,
Krzysztof

Re: [PATCH RFC 1/2] dt-bindings: mtd: ubi-volume: add 'volume-is-critical' property

[Daniel Golle]

On Sat, Sep 28, 2024 at 03:02:47PM +0200, Krzysztof Kozlowski wrote:
> On 28/09/2024 14:47, Daniel Golle wrote:
> > Add the 'volume-is-critical' boolean property which marks a UBI volume
> > as critical for the device to boot. If set it prevents the user from
> > all kinds of write access to the volume as well as from renaming it or
> > detaching the UBI device it is located on.
> > 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >  .../devicetree/bindings/mtd/partitions/ubi-volume.yaml   | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
> > index 19736b26056b..2bd751bb7f9e 100644
> > --- a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
> > +++ b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
> > @@ -29,6 +29,15 @@ properties:
> >      description:
> >        This container may reference an NVMEM layout parser.
> >  
> > +  volume-is-critical:
> > +    description: This parameter, if present, indicates that the UBI volume
> > +      contains early-boot firmware images or data which should not be clobbered.
> > +      If set, it prevents the user from renaming the volume, writing to it or
> > +      making any changes affecting it, as well as detaching the UBI device it is
> > +      located on, so direct access to the underlying MTD device is prevented as
> > +      well.
> > +    type: boolean
> 
> UBI volumes are mapping to partitions 1-to-1, right? So rather I would
> propose to use partition.yaml - we already have read-only there with
> very similar description.

No, that's not the case.

An MTD partition can be used as UBI device. A UBI device (and hence MTD
partition) can host *several* UBI volumes.

Marking the MTD partition as 'read-only' won't work, as UBI needs
read-write access to perform bad block relocation, scrubbing, ...

Also, typically not all UBI volumes on a UBI device are
read-only/critical but only a subset of them.

But you are right that the description is inspired by the description
of the 'read-only' property in partition.yaml ;)

I initially thought to also name the property 'read-only', just like
for MTD partitions. However, as the desired effect goes beyond
preventing write access to the volume itself, I thought it'd be
better to use a new name.

Re: [PATCH RFC 1/2] dt-bindings: mtd: ubi-volume: add 'volume-is-critical' property

[Krzysztof Kozlowski]

On 28/09/2024 15:09, Daniel Golle wrote:
> On Sat, Sep 28, 2024 at 03:02:47PM +0200, Krzysztof Kozlowski wrote:
>> On 28/09/2024 14:47, Daniel Golle wrote:
>>> Add the 'volume-is-critical' boolean property which marks a UBI volume
>>> as critical for the device to boot. If set it prevents the user from
>>> all kinds of write access to the volume as well as from renaming it or
>>> detaching the UBI device it is located on.
>>>
>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>> ---
>>>  .../devicetree/bindings/mtd/partitions/ubi-volume.yaml   | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
>>> index 19736b26056b..2bd751bb7f9e 100644
>>> --- a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
>>> +++ b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
>>> @@ -29,6 +29,15 @@ properties:
>>>      description:
>>>        This container may reference an NVMEM layout parser.
>>>  
>>> +  volume-is-critical:
>>> +    description: This parameter, if present, indicates that the UBI volume
>>> +      contains early-boot firmware images or data which should not be clobbered.
>>> +      If set, it prevents the user from renaming the volume, writing to it or
>>> +      making any changes affecting it, as well as detaching the UBI device it is
>>> +      located on, so direct access to the underlying MTD device is prevented as
>>> +      well.
>>> +    type: boolean
>>
>> UBI volumes are mapping to partitions 1-to-1, right? So rather I would
>> propose to use partition.yaml - we already have read-only there with
>> very similar description.
> 
> No, that's not the case.
> 
> An MTD partition can be used as UBI device. A UBI device (and hence MTD
> partition) can host *several* UBI volumes.
> 
> Marking the MTD partition as 'read-only' won't work, as UBI needs
> read-write access to perform bad block relocation, scrubbing, ...

OK, so not partition but read-only volume.

> 
> Also, typically not all UBI volumes on a UBI device are
> read-only/critical but only a subset of them.
> 
> But you are right that the description is inspired by the description
> of the 'read-only' property in partition.yaml ;)
> 
> I initially thought to also name the property 'read-only', just like
> for MTD partitions. However, as the desired effect goes beyond
> preventing write access to the volume itself, I thought it'd be
> better to use a new name.

Yeah, maybe... critical indeed covers multiple cases but is also
subjective. For some bootloader is critical, for other bootloader still
might be fully A/B updateable thus could be modifiable. For others, they
want to use fw_setenv from user-space so not critical at all.

I am in general not so happy in describing flash layout in DT, because
it sounds way too policy or one-use-case specific.

UBI volume is purely SW construct. One OS can partition MTD in multiple
ways, so maybe I just do not understand why we have in the first place.

Best regards,
Krzysztof

Re: [PATCH RFC 1/2] dt-bindings: mtd: ubi-volume: add 'volume-is-critical' property

[Daniel Golle]

On Sat, Sep 28, 2024 at 03:45:49PM +0200, Krzysztof Kozlowski wrote:
> On 28/09/2024 15:09, Daniel Golle wrote:
> > On Sat, Sep 28, 2024 at 03:02:47PM +0200, Krzysztof Kozlowski wrote:
> >> On 28/09/2024 14:47, Daniel Golle wrote:
> >>> Add the 'volume-is-critical' boolean property which marks a UBI volume
> >>> as critical for the device to boot. If set it prevents the user from
> >>> all kinds of write access to the volume as well as from renaming it or
> >>> detaching the UBI device it is located on.
> >>>
> >>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> >>> ---
> >>>  .../devicetree/bindings/mtd/partitions/ubi-volume.yaml   | 9 +++++++++
> >>>  1 file changed, 9 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
> >>> index 19736b26056b..2bd751bb7f9e 100644
> >>> --- a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
> >>> +++ b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
> >>> @@ -29,6 +29,15 @@ properties:
> >>>      description:
> >>>        This container may reference an NVMEM layout parser.
> >>>  
> >>> +  volume-is-critical:
> >>> +    description: This parameter, if present, indicates that the UBI volume
> >>> +      contains early-boot firmware images or data which should not be clobbered.
> >>> +      If set, it prevents the user from renaming the volume, writing to it or
> >>> +      making any changes affecting it, as well as detaching the UBI device it is
> >>> +      located on, so direct access to the underlying MTD device is prevented as
> >>> +      well.
> >>> +    type: boolean
> >>
> >> UBI volumes are mapping to partitions 1-to-1, right? So rather I would
> >> propose to use partition.yaml - we already have read-only there with
> >> very similar description.
> > 
> > No, that's not the case.
> > 
> > An MTD partition can be used as UBI device. A UBI device (and hence MTD
> > partition) can host *several* UBI volumes.
> > 
> > Marking the MTD partition as 'read-only' won't work, as UBI needs
> > read-write access to perform bad block relocation, scrubbing, ...
> 
> OK, so not partition but read-only volume.

+1

> 
> > 
> > Also, typically not all UBI volumes on a UBI device are
> > read-only/critical but only a subset of them.
> > 
> > But you are right that the description is inspired by the description
> > of the 'read-only' property in partition.yaml ;)
> > 
> > I initially thought to also name the property 'read-only', just like
> > for MTD partitions. However, as the desired effect goes beyond
> > preventing write access to the volume itself, I thought it'd be
> > better to use a new name.
> 
> Yeah, maybe... critical indeed covers multiple cases but is also
> subjective. For some bootloader is critical, for other bootloader still
> might be fully A/B updateable thus could be modifiable. For others, they
> want to use fw_setenv from user-space so not critical at all.

The case I want to cover here is the bootloader itself being stored
inside a UBI volume. MediaTek's fork of ARM TrustedFirmware-A bl2 comes
with support for UBI and loads BL3 (which is TF-A BL31 and U-Boot, and
maybe OP-TEE as well) from a static UBI volume. Removing, renaming or
altering that volume results in the device not being able to boot any
more and requiring a complicated intervention (at attaching debugging
UART and using low-level recovery tool) in order to recover.

It is true that also in this case a fully A/B updateable strategy could
be implemented, however, typically that would not be done in Linux, but
rather the code carrying out such update would be part of the bootloader
itself.
UEFI update capsules are a good example for that, but in most cases
things are much less complex on simple embedded Linux networking
appliances such as routers, access points, switches, ...

> 
> I am in general not so happy in describing flash layout in DT, because
> it sounds way too policy or one-use-case specific.
> 
> UBI volume is purely SW construct. One OS can partition MTD in multiple
> ways, so maybe I just do not understand why we have in the first place.

Support for attaching UBI from DT and referencing UBI volumes has been
added because they serve as NVMEM providers, ie. UBI volumes are used to
store things such as Ethernet MAC addresses and Wi-Fi calibration data,
similar to how it also works on devices with NOR flash and simple MTD
partitions holding that data.
And we needed the kernel drivers to be able to access that information
directly, also in case eg. nfsroot is being used.

So while it's true that both, MTD partitioning as well as use of UBI
are pure software constructs, their use can go beyond and below the OS
kernel, meaning that early boot firmware usually requires a specific MTD
partition layout, and some require also certain UBI volumes to be present
for the device to be able to even start the bootloader.

It is also true that (just like MTD partitioning as well) it is
questionable whether this should be communicated as a property of the
flash hardware in DT, or rather end up as information filed under the
/chosen node which to me seems more appropriate. Moving all MTD
partitioning away from the node representing the flash chip and into the
/chosen node would have to happen in a way which doesn't break
compatibility with existing DT, which all describe MTD partitions as a
subnode of the storage hardware itself. Obviously that is beyond the
scope (and independent) of being able to mark some UBI volumes as
critical.

Re: [PATCH RFC 1/2] dt-bindings: mtd: ubi-volume: add 'volume-is-critical' property

[Zhihao Cheng]

在 2024/9/28 22:38, Daniel Golle 写道:
> On Sat, Sep 28, 2024 at 03:45:49PM +0200, Krzysztof Kozlowski wrote:
>> On 28/09/2024 15:09, Daniel Golle wrote:
>>> On Sat, Sep 28, 2024 at 03:02:47PM +0200, Krzysztof Kozlowski wrote:
>>>> On 28/09/2024 14:47, Daniel Golle wrote:
>>>>> Add the 'volume-is-critical' boolean property which marks a UBI volume
>>>>> as critical for the device to boot. If set it prevents the user from
>>>>> all kinds of write access to the volume as well as from renaming it or
>>>>> detaching the UBI device it is located on.
>>>>>
>>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>>>> ---
>>>>>   .../devicetree/bindings/mtd/partitions/ubi-volume.yaml   | 9 +++++++++
>>>>>   1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
>>>>> index 19736b26056b..2bd751bb7f9e 100644
>>>>> --- a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
>>>>> +++ b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
>>>>> @@ -29,6 +29,15 @@ properties:
>>>>>       description:
>>>>>         This container may reference an NVMEM layout parser.
>>>>>   
>>>>> +  volume-is-critical:
>>>>> +    description: This parameter, if present, indicates that the UBI volume
>>>>> +      contains early-boot firmware images or data which should not be clobbered.
>>>>> +      If set, it prevents the user from renaming the volume, writing to it or
>>>>> +      making any changes affecting it, as well as detaching the UBI device it is
>>>>> +      located on, so direct access to the underlying MTD device is prevented as
>>>>> +      well.
>>>>> +    type: boolean
>>>>
>>>> UBI volumes are mapping to partitions 1-to-1, right? So rather I would
>>>> propose to use partition.yaml - we already have read-only there with
>>>> very similar description.
>>>
>>> No, that's not the case.
>>>
>>> An MTD partition can be used as UBI device. A UBI device (and hence MTD
>>> partition) can host *several* UBI volumes.
>>>
>>> Marking the MTD partition as 'read-only' won't work, as UBI needs
>>> read-write access to perform bad block relocation, scrubbing, ...
>>
>> OK, so not partition but read-only volume.
> 
> +1
> 
>>
>>>
>>> Also, typically not all UBI volumes on a UBI device are
>>> read-only/critical but only a subset of them.
>>>
>>> But you are right that the description is inspired by the description
>>> of the 'read-only' property in partition.yaml ;)
>>>
>>> I initially thought to also name the property 'read-only', just like
>>> for MTD partitions. However, as the desired effect goes beyond
>>> preventing write access to the volume itself, I thought it'd be
>>> better to use a new name.
>>
>> Yeah, maybe... critical indeed covers multiple cases but is also
>> subjective. For some bootloader is critical, for other bootloader still
>> might be fully A/B updateable thus could be modifiable. For others, they
>> want to use fw_setenv from user-space so not critical at all.
> 
> The case I want to cover here is the bootloader itself being stored
> inside a UBI volume. MediaTek's fork of ARM TrustedFirmware-A bl2 comes
> with support for UBI and loads BL3 (which is TF-A BL31 and U-Boot, and
> maybe OP-TEE as well) from a static UBI volume. Removing, renaming or
> altering that volume results in the device not being able to boot any
> more and requiring a complicated intervention (at attaching debugging
> UART and using low-level recovery tool) in order to recover.

Who removes/renames the 'critical' volume? I suggest to fix it in the 
upper layer(not in kernel). After looking through the patch 2, it seems 
a hack solution.

Re: [PATCH RFC 1/2] dt-bindings: mtd: ubi-volume: add 'volume-is-critical' property

[Daniel Golle]

On Sun, Sep 29, 2024 at 12:03:11PM +0800, Zhihao Cheng wrote:
> 在 2024/9/28 22:38, Daniel Golle 写道:
> > On Sat, Sep 28, 2024 at 03:45:49PM +0200, Krzysztof Kozlowski wrote:
> > > On 28/09/2024 15:09, Daniel Golle wrote:
> > > > On Sat, Sep 28, 2024 at 03:02:47PM +0200, Krzysztof Kozlowski wrote:
> > > > > On 28/09/2024 14:47, Daniel Golle wrote:
> > > > > > Add the 'volume-is-critical' boolean property which marks a UBI volume
> > > > > > as critical for the device to boot. If set it prevents the user from
> > > > > > all kinds of write access to the volume as well as from renaming it or
> > > > > > detaching the UBI device it is located on.
> > > > > > 
> > > > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > > > > > ---
> > > > > >   .../devicetree/bindings/mtd/partitions/ubi-volume.yaml   | 9 +++++++++
> > > > > >   1 file changed, 9 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
> > > > > > index 19736b26056b..2bd751bb7f9e 100644
> > > > > > --- a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
> > > > > > @@ -29,6 +29,15 @@ properties:
> > > > > >       description:
> > > > > >         This container may reference an NVMEM layout parser.
> > > > > > +  volume-is-critical:
> > > > > > +    description: This parameter, if present, indicates that the UBI volume
> > > > > > +      contains early-boot firmware images or data which should not be clobbered.
> > > > > > +      If set, it prevents the user from renaming the volume, writing to it or
> > > > > > +      making any changes affecting it, as well as detaching the UBI device it is
> > > > > > +      located on, so direct access to the underlying MTD device is prevented as
> > > > > > +      well.
> > > > > > +    type: boolean
> > > > > 
> > > > > UBI volumes are mapping to partitions 1-to-1, right? So rather I would
> > > > > propose to use partition.yaml - we already have read-only there with
> > > > > very similar description.
> > > > 
> > > > No, that's not the case.
> > > > 
> > > > An MTD partition can be used as UBI device. A UBI device (and hence MTD
> > > > partition) can host *several* UBI volumes.
> > > > 
> > > > Marking the MTD partition as 'read-only' won't work, as UBI needs
> > > > read-write access to perform bad block relocation, scrubbing, ...
> > > 
> > > OK, so not partition but read-only volume.
> > 
> > +1
> > 
> > > 
> > > > 
> > > > Also, typically not all UBI volumes on a UBI device are
> > > > read-only/critical but only a subset of them.
> > > > 
> > > > But you are right that the description is inspired by the description
> > > > of the 'read-only' property in partition.yaml ;)
> > > > 
> > > > I initially thought to also name the property 'read-only', just like
> > > > for MTD partitions. However, as the desired effect goes beyond
> > > > preventing write access to the volume itself, I thought it'd be
> > > > better to use a new name.
> > > 
> > > Yeah, maybe... critical indeed covers multiple cases but is also
> > > subjective. For some bootloader is critical, for other bootloader still
> > > might be fully A/B updateable thus could be modifiable. For others, they
> > > want to use fw_setenv from user-space so not critical at all.
> > 
> > The case I want to cover here is the bootloader itself being stored
> > inside a UBI volume. MediaTek's fork of ARM TrustedFirmware-A bl2 comes
> > with support for UBI and loads BL3 (which is TF-A BL31 and U-Boot, and
> > maybe OP-TEE as well) from a static UBI volume. Removing, renaming or
> > altering that volume results in the device not being able to boot any
> > more and requiring a complicated intervention (at attaching debugging
> > UART and using low-level recovery tool) in order to recover.
> 
> Who removes/renames the 'critical' volume? I suggest to fix it in the upper
> layer(not in kernel). After looking through the patch 2, it seems a hack
> solution.

The enemy is the user, the upper layer is between the keyboard and the
screen. Just like for 'read-only' MTD partitions I'm looking
for a similar solution for UBI which prevents the user from accidentally
deleting or destroying the bootloader, lets say, when logged in via SSH.

Re: [PATCH RFC 1/2] dt-bindings: mtd: ubi-volume: add 'volume-is-critical' property

[Zhihao Cheng]

在 2024/9/29 18:52, Daniel Golle 写道:
> On Sun, Sep 29, 2024 at 12:03:11PM +0800, Zhihao Cheng wrote:
>> 在 2024/9/28 22:38, Daniel Golle 写道:
>>> On Sat, Sep 28, 2024 at 03:45:49PM +0200, Krzysztof Kozlowski wrote:
>>>> On 28/09/2024 15:09, Daniel Golle wrote:
>>>>> On Sat, Sep 28, 2024 at 03:02:47PM +0200, Krzysztof Kozlowski wrote:
>>>>>> On 28/09/2024 14:47, Daniel Golle wrote:
>>>>>>> Add the 'volume-is-critical' boolean property which marks a UBI volume
>>>>>>> as critical for the device to boot. If set it prevents the user from
>>>>>>> all kinds of write access to the volume as well as from renaming it or
>>>>>>> detaching the UBI device it is located on.
>>>>>>>
>>>>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>>>>>> ---
>>>>>>>    .../devicetree/bindings/mtd/partitions/ubi-volume.yaml   | 9 +++++++++
>>>>>>>    1 file changed, 9 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
>>>>>>> index 19736b26056b..2bd751bb7f9e 100644
>>>>>>> --- a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
>>>>>>> @@ -29,6 +29,15 @@ properties:
>>>>>>>        description:
>>>>>>>          This container may reference an NVMEM layout parser.
>>>>>>> +  volume-is-critical:
>>>>>>> +    description: This parameter, if present, indicates that the UBI volume
>>>>>>> +      contains early-boot firmware images or data which should not be clobbered.
>>>>>>> +      If set, it prevents the user from renaming the volume, writing to it or
>>>>>>> +      making any changes affecting it, as well as detaching the UBI device it is
>>>>>>> +      located on, so direct access to the underlying MTD device is prevented as
>>>>>>> +      well.
>>>>>>> +    type: boolean
>>>>>>
>>>>>> UBI volumes are mapping to partitions 1-to-1, right? So rather I would
>>>>>> propose to use partition.yaml - we already have read-only there with
>>>>>> very similar description.
>>>>>
>>>>> No, that's not the case.
>>>>>
>>>>> An MTD partition can be used as UBI device. A UBI device (and hence MTD
>>>>> partition) can host *several* UBI volumes.
>>>>>
>>>>> Marking the MTD partition as 'read-only' won't work, as UBI needs
>>>>> read-write access to perform bad block relocation, scrubbing, ...
>>>>
>>>> OK, so not partition but read-only volume.
>>>
>>> +1
>>>
>>>>
>>>>>
>>>>> Also, typically not all UBI volumes on a UBI device are
>>>>> read-only/critical but only a subset of them.
>>>>>
>>>>> But you are right that the description is inspired by the description
>>>>> of the 'read-only' property in partition.yaml ;)
>>>>>
>>>>> I initially thought to also name the property 'read-only', just like
>>>>> for MTD partitions. However, as the desired effect goes beyond
>>>>> preventing write access to the volume itself, I thought it'd be
>>>>> better to use a new name.
>>>>
>>>> Yeah, maybe... critical indeed covers multiple cases but is also
>>>> subjective. For some bootloader is critical, for other bootloader still
>>>> might be fully A/B updateable thus could be modifiable. For others, they
>>>> want to use fw_setenv from user-space so not critical at all.
>>>
>>> The case I want to cover here is the bootloader itself being stored
>>> inside a UBI volume. MediaTek's fork of ARM TrustedFirmware-A bl2 comes
>>> with support for UBI and loads BL3 (which is TF-A BL31 and U-Boot, and
>>> maybe OP-TEE as well) from a static UBI volume. Removing, renaming or
>>> altering that volume results in the device not being able to boot any
>>> more and requiring a complicated intervention (at attaching debugging
>>> UART and using low-level recovery tool) in order to recover.
>>
>> Who removes/renames the 'critical' volume? I suggest to fix it in the upper
>> layer(not in kernel). After looking through the patch 2, it seems a hack
>> solution.
> 
> The enemy is the user, the upper layer is between the keyboard and the
> screen. Just like for 'read-only' MTD partitions I'm looking
> for a similar solution for UBI which prevents the user from accidentally
> deleting or destroying the bootloader, lets say, when logged in via SSH.
> .
> 

I guess that other partitions(excepts mtd) have the similar situations, 
users could delete a rootfs(ext4) partition by operation the raw block 
device. The kernel has no way to stop user doing this, what if the user 
just want to rebuild partions?
Marking volume as critical(by a stopper in kernel) could prevent user 
mistakenly operating, but I think it is more important that user need to 
know what he/she is doing.

Re: [PATCH RFC 1/2] dt-bindings: mtd: ubi-volume: add 'volume-is-critical' property

[Daniel Golle]

On 29 September 2024 11:23:12 UTC, Zhihao Cheng <chengzhihao1@huawei.com> wrote:
>在 2024/9/29 18:52, Daniel Golle 写道:
>> On Sun, Sep 29, 2024 at 12:03:11PM +0800, Zhihao Cheng wrote:
>>> 在 2024/9/28 22:38, Daniel Golle 写道:
>>>> On Sat, Sep 28, 2024 at 03:45:49PM +0200, Krzysztof Kozlowski wrote:
>>>> [...]
>>>> The case I want to cover here is the bootloader itself being stored
>>>> inside a UBI volume. MediaTek's fork of ARM TrustedFirmware-A bl2 comes
>>>> with support for UBI and loads BL3 (which is TF-A BL31 and U-Boot, and
>>>> maybe OP-TEE as well) from a static UBI volume. Removing, renaming or
>>>> altering that volume results in the device not being able to boot any
>>>> more and requiring a complicated intervention (at attaching debugging
>>>> UART and using low-level recovery tool) in order to recover.
>>> 
>>> Who removes/renames the 'critical' volume? I suggest to fix it in the upper
>>> layer(not in kernel). After looking through the patch 2, it seems a hack
>>> solution.
>> 
>> The enemy is the user, the upper layer is between the keyboard and the
>> screen. Just like for 'read-only' MTD partitions I'm looking
>> for a similar solution for UBI which prevents the user from accidentally
>> deleting or destroying the bootloader, lets say, when logged in via SSH.
>> .
>> 
>
>I guess that other partitions(excepts mtd) have the similar situations, users could delete a rootfs(ext4) partition by operation the raw block device. The kernel has no way to stop user doing this, what if the user just want to rebuild partions?

True, but as you correctly pointed out the worst-case scenario would be Linux no longer booting. You would fix that by using a bootable USB pendrive.
However, the user wont easily manage to delete the bootloader or BIOS by accident, such as a simple typo of the partition number or accidentally destroying GPT. If the storage of low-level boot firmware is accissible from within Linux then there are always additional safeguards, such as the write-protection of mmcblkXbootY hw-partition, or MTD partitons holding bootloader components being marked as read-only.

>Marking volume as critical(by a stopper in kernel) could prevent user mistakenly operating, but I think it is more important that user need to know what he/she is doing.

I agree that education of users is crucial, yet I think the risk of needing complicated hardware interventions (direct access to NAND flash chip, attaching JTAG, ...) which are required in order to recover from a mistake should also be taken into account.

Re: [PATCH RFC 2/2] mtd: ubi: add support for protecting critical volumes

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Daniel Golle" <daniel@makrotopia.org>
> Allow the boot firmware to define volumes which are critical for the
> system to boot, such as the bootloader itself if stored inside a UBI
> volume. Protect critical volumes by preventing the user from removing,
> resizing or writing to them, and also prevent the UBI device from
> being detached if a critical volume is present.

I agree with the doubts raised in patch 1/2, if userspace is so hostile
to delete system partitions, there is little hope.
But I'm still open for discussion.

What I veto is preventing detach.
This makes a clean tear down of the system impossible.
It becomes more and more common that advanced userspace shuts down
everything it setup during boot. e.g. while reboot switching back
to an initramfs, umounting root, shutting down all storage, etc...

Thanks,
//richard

Re: [PATCH RFC 2/2] mtd: ubi: add support for protecting critical volumes

[Zhihao Cheng]

在 2024/9/29 20:26, Richard Weinberger 写道:
> ----- Ursprüngliche Mail -----
>> Von: "Daniel Golle" <daniel@makrotopia.org>
>> Allow the boot firmware to define volumes which are critical for the
>> system to boot, such as the bootloader itself if stored inside a UBI
>> volume. Protect critical volumes by preventing the user from removing,
>> resizing or writing to them, and also prevent the UBI device from
>> being detached if a critical volume is present.
> 
> I agree with the doubts raised in patch 1/2, if userspace is so hostile
> to delete system partitions, there is little hope.
> But I'm still open for discussion.

Yes, I agree that it is meaningful to prevent user from operating 
volumes accidently. How about doing that by some existing methods? Eg. 
selinux(Design sepolicy for ioctl cmd).
> 
> What I veto is preventing detach.
> This makes a clean tear down of the system impossible.
> It becomes more and more common that advanced userspace shuts down
> everything it setup during boot. e.g. while reboot switching back
> to an initramfs, umounting root, shutting down all storage, etc...
> 
> Thanks,
> //richard
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

Re: [PATCH RFC 2/2] mtd: ubi: add support for protecting critical volumes

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "chengzhihao1" <chengzhihao1@huawei.com>
>>> Von: "Daniel Golle" <daniel@makrotopia.org>
>>> Allow the boot firmware to define volumes which are critical for the
>>> system to boot, such as the bootloader itself if stored inside a UBI
>>> volume. Protect critical volumes by preventing the user from removing,
>>> resizing or writing to them, and also prevent the UBI device from
>>> being detached if a critical volume is present.
>> 
>> I agree with the doubts raised in patch 1/2, if userspace is so hostile
>> to delete system partitions, there is little hope.
>> But I'm still open for discussion.
> 
> Yes, I agree that it is meaningful to prevent user from operating
> volumes accidently. How about doing that by some existing methods? Eg.
> selinux(Design sepolicy for ioctl cmd).

Another thought, do we really need to enforce this in kernel space?
Teaching ubi-tools to be super careful with some volumes is also an option.

like a ubirmvol ... --i-know-what-im-doing.

Thanks,
//richard

Re: [PATCH RFC 2/2] mtd: ubi: add support for protecting critical volumes

[Daniel Golle]

On Mon, Sep 30, 2024 at 08:43:40PM +0200, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
> > Von: "chengzhihao1" <chengzhihao1@huawei.com>
> >>> Von: "Daniel Golle" <daniel@makrotopia.org>
> >>> Allow the boot firmware to define volumes which are critical for the
> >>> system to boot, such as the bootloader itself if stored inside a UBI
> >>> volume. Protect critical volumes by preventing the user from removing,
> >>> resizing or writing to them, and also prevent the UBI device from
> >>> being detached if a critical volume is present.
> >> 
> >> I agree with the doubts raised in patch 1/2, if userspace is so hostile
> >> to delete system partitions, there is little hope.
> >> But I'm still open for discussion.
> > 
> > Yes, I agree that it is meaningful to prevent user from operating
> > volumes accidently. How about doing that by some existing methods? Eg.
> > selinux(Design sepolicy for ioctl cmd).
> 
> Another thought, do we really need to enforce this in kernel space?
> Teaching ubi-tools to be super careful with some volumes is also an option.
> 
> like a ubirmvol ... --i-know-what-im-doing.

True, enforcement doesn't need to happen in kernel (though I think it's
nicer, but really just a matter of taste, I guess). ubi-tools would still
need to be able to recognize critical volumes somehow, and that could be
done by checking if the 'volume-is-critical' property is present in
/sys/class/ubi/ubi*_*/of_node/

If you prefer going down that road instead I will work on patches for
git.infradead.org/mtd-utils.git instead.

Re: [PATCH RFC 2/2] mtd: ubi: add support for protecting critical volumes

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Daniel Golle" <daniel@makrotopia.org>
>> like a ubirmvol ... --i-know-what-im-doing.
> 
> True, enforcement doesn't need to happen in kernel (though I think it's
> nicer, but really just a matter of taste, I guess). ubi-tools would still
> need to be able to recognize critical volumes somehow, and that could be
> done by checking if the 'volume-is-critical' property is present in
> /sys/class/ubi/ubi*_*/of_node/

Exactly.
I also don't mind adding a in-memory 'volume-is-critical' property to
UBI directly. I'm just a little hesitated to change the UAPI or the on-disk
data structures for this features.
 
> If you prefer going down that road instead I will work on patches for
> git.infradead.org/mtd-utils.git instead.

Yes. When done in userspace, it's also much easier to offer the --i-know-what-im-doing
flag to still remove a critical volume.
No need to touch UAPI.

Zhihao Cheng, what do you think about this approach?

Thanks,
//richard

Re: [PATCH RFC 2/2] mtd: ubi: add support for protecting critical volumes

[Zhihao Cheng]

在 2024/10/1 3:54, Richard Weinberger 写道:
> ----- Ursprüngliche Mail -----
>> Von: "Daniel Golle" <daniel@makrotopia.org>
>>> like a ubirmvol ... --i-know-what-im-doing.
>>

Sorry for the delay reply, caused by my holiday.
>> True, enforcement doesn't need to happen in kernel (though I think it's
>> nicer, but really just a matter of taste, I guess). ubi-tools would still
>> need to be able to recognize critical volumes somehow, and that could be
>> done by checking if the 'volume-is-critical' property is present in
>> /sys/class/ubi/ubi*_*/of_node/
> 
> Exactly.
> I also don't mind adding a in-memory 'volume-is-critical' property to
> UBI directly. I'm just a little hesitated to change the UAPI or the on-disk
> data structures for this features.

Add a new interface under the sysfs is fine.
>   
>> If you prefer going down that road instead I will work on patches for
>> git.infradead.org/mtd-utils.git instead.
> 
> Yes. When done in userspace, it's also much easier to offer the --i-know-what-im-doing
> flag to still remove a critical volume.

Looks good, I like it.