[PATCH] mmc: boot partition ro lock support

[PATCH] mmc: boot partition ro lock support

[Ulf Hansson]

From: Johan Rudholm <johan.rudholm@stericsson.com>

Enable boot partitions to be power and permanently read-only locked via
a sysfs entry. There will be one sysfs entry for the main mmc device:

/sys/block/mmcblkX/boot_partition_ro_lock

and one for each boot partition:

/sys/block/mmcblkXbootY/ro_lock

The boot partitions are power or permanently locked by writing "pwr_ro"
or "perm_ro" to one of the files.

Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
Signed-off-by: John Beckett <john.beckett@stericsson.com>
Signed-off-by: Johan Rudholm <johan.rudholm@stericsson.com>
---
 drivers/mmc/card/block.c |  124 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/mmc/core/mmc.c   |   15 +++++-
 include/linux/mmc/card.h |    9 +++-
 include/linux/mmc/mmc.h  |    6 ++
 4 files changed, 143 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index b2c76bb..f6084b7 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -107,6 +107,8 @@ struct mmc_blk_data {
 	 */
 	unsigned int	part_curr;
 	struct device_attribute force_ro;
+	struct device_attribute boot_partition_ro_lock;
+	int	area_type;
 };
 
 static DEFINE_MUTEX(open_lock);
@@ -165,6 +167,80 @@ static void mmc_blk_put(struct mmc_blk_data *md)
 	mutex_unlock(&open_lock);
 }
 
+#define EXT_CSD_BOOT_WP_PWR_WP_TEXT "pwr_ro"
+#define EXT_CSD_BOOT_WP_PERM_WP_TEXT "perm_ro"
+#define EXT_CSD_BOOT_WP_WP_DISABLED_TEXT "rw"
+static ssize_t boot_partition_ro_lock_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	int ret;
+	struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+	struct mmc_card *card = md->queue.card;
+	const char *out_text;
+
+	if (card->ext_csd.boot_locked
+			& EXT_CSD_BOOT_WP_B_PERM_WP_EN)
+		out_text = EXT_CSD_BOOT_WP_PERM_WP_TEXT;
+	else if (card->ext_csd.boot_locked
+			& EXT_CSD_BOOT_WP_B_PWR_WP_EN)
+		out_text = EXT_CSD_BOOT_WP_PWR_WP_TEXT;
+	else
+		out_text = EXT_CSD_BOOT_WP_WP_DISABLED_TEXT;
+
+	ret = snprintf(buf, PAGE_SIZE, "%s\n", out_text);
+
+	return ret;
+}
+
+static ssize_t boot_partition_ro_lock_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	int ret;
+	struct mmc_blk_data *md, *part_md;
+	struct mmc_card *card;
+	u8 set = 0;
+
+	md = mmc_blk_get(dev_to_disk(dev));
+	card = md->queue.card;
+
+	if (!strncmp(buf, EXT_CSD_BOOT_WP_PWR_WP_TEXT,
+				strlen(EXT_CSD_BOOT_WP_PWR_WP_TEXT)))
+		set = EXT_CSD_BOOT_WP_B_PWR_WP_EN;
+	else if (!strncmp(buf, EXT_CSD_BOOT_WP_PERM_WP_TEXT,
+				strlen(EXT_CSD_BOOT_WP_PERM_WP_TEXT)))
+		set = EXT_CSD_BOOT_WP_B_PERM_WP_EN;
+
+	if (set) {
+		mmc_claim_host(card->host);
+
+		ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+				EXT_CSD_BOOT_WP,
+				set,
+				card->ext_csd.part_time);
+		if (ret)
+			pr_err("Boot Partition Lock failed: %d", ret);
+		else
+			card->ext_csd.boot_locked = set;
+
+		mmc_release_host(card->host);
+
+		if (!ret)
+			list_for_each_entry(part_md, &md->part, part)
+				if (part_md->area_type ==
+						MMC_BLK_DATA_AREA_BOOT) {
+					pr_info("%s: Locking boot partition "
+						"%s",
+						part_md->disk->disk_name,
+						buf);
+					set_disk_ro(part_md->disk, 1);
+				}
+	}
+	ret = count;
+
+	mmc_blk_put(md);
+	return ret;
+}
+
 static ssize_t force_ro_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
@@ -1329,7 +1405,8 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
 					      struct device *parent,
 					      sector_t size,
 					      bool default_ro,
-					      const char *subname)
+					      const char *subname,
+					      int area_type)
 {
 	struct mmc_blk_data *md;
 	int devidx, ret;
@@ -1354,11 +1431,12 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
 	if (!subname) {
 		md->name_idx = find_first_zero_bit(name_use, max_devices);
 		__set_bit(md->name_idx, name_use);
-	}
-	else
+	} else
 		md->name_idx = ((struct mmc_blk_data *)
 				dev_to_disk(parent)->private_data)->name_idx;
 
+	md->area_type = area_type;
+
 	/*
 	 * Set the read-only status based on the supported commands
 	 * and the write protect switch.
@@ -1452,7 +1530,8 @@ static struct mmc_blk_data *mmc_blk_alloc(struct mmc_card *card)
 		size = card->csd.capacity << (card->csd.read_blkbits - 9);
 	}
 
-	md = mmc_blk_alloc_req(card, &card->dev, size, false, NULL);
+	md = mmc_blk_alloc_req(card, &card->dev, size, false, NULL,
+					MMC_BLK_DATA_AREA_MAIN);
 	return md;
 }
 
@@ -1461,13 +1540,14 @@ static int mmc_blk_alloc_part(struct mmc_card *card,
 			      unsigned int part_type,
 			      sector_t size,
 			      bool default_ro,
-			      const char *subname)
+			      const char *subname,
+			      int area_type)
 {
 	char cap_str[10];
 	struct mmc_blk_data *part_md;
 
 	part_md = mmc_blk_alloc_req(card, disk_to_dev(md->disk), size, default_ro,
-				    subname);
+				    subname, area_type);
 	if (IS_ERR(part_md))
 		return PTR_ERR(part_md);
 	part_md->part_type = part_type;
@@ -1500,7 +1580,8 @@ static int mmc_blk_alloc_parts(struct mmc_card *card, struct mmc_blk_data *md)
 				card->part[idx].part_cfg,
 				card->part[idx].size >> 9,
 				card->part[idx].force_ro,
-				card->part[idx].name);
+				card->part[idx].name,
+				card->part[idx].area_type);
 			if (ret)
 				return ret;
 		}
@@ -1532,6 +1613,10 @@ static void mmc_blk_remove_req(struct mmc_blk_data *md)
 	if (md) {
 		if (md->disk->flags & GENHD_FL_UP) {
 			device_remove_file(disk_to_dev(md->disk), &md->force_ro);
+			if (md->area_type == MMC_BLK_DATA_AREA_MAIN ||
+					md->area_type == MMC_BLK_DATA_AREA_BOOT)
+				device_remove_file(disk_to_dev(md->disk),
+					&md->boot_partition_ro_lock);
 
 			/* Stop new requests from getting into the queue */
 			del_gendisk(md->disk);
@@ -1569,7 +1654,30 @@ static int mmc_add_disk(struct mmc_blk_data *md)
 	md->force_ro.attr.mode = S_IRUGO | S_IWUSR;
 	ret = device_create_file(disk_to_dev(md->disk), &md->force_ro);
 	if (ret)
-		del_gendisk(md->disk);
+		goto force_ro_fail;
+
+	if (md->area_type == MMC_BLK_DATA_AREA_MAIN ||
+				md->area_type == MMC_BLK_DATA_AREA_BOOT) {
+		md->boot_partition_ro_lock.show = boot_partition_ro_lock_show;
+		md->boot_partition_ro_lock.store = boot_partition_ro_lock_store;
+		md->boot_partition_ro_lock.attr.mode = S_IRUGO | S_IWUSR;
+		if (md->area_type == MMC_BLK_DATA_AREA_MAIN)
+			md->boot_partition_ro_lock.attr.name =
+					"boot_partition_ro_lock";
+		else
+			md->boot_partition_ro_lock.attr.name =
+					"ro_lock";
+		ret = device_create_file(disk_to_dev(md->disk),
+				&md->boot_partition_ro_lock);
+		if (ret)
+			goto boot_partition_ro_lock_fail;
+	}
+	return ret;
+
+boot_partition_ro_lock_fail:
+	device_remove_file(disk_to_dev(md->disk), &md->force_ro);
+force_ro_fail:
+	del_gendisk(md->disk);
 
 	return ret;
 }
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index fb5bf01..4c3d0df 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -339,6 +339,15 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
 		card->ext_csd.rel_sectors = ext_csd[EXT_CSD_REL_WR_SEC_C];
 
 		/*
+		 * Note that the call to mmc_part_add defaults to read
+		 * only. If this default assumption is changed, the call must
+		 * take into account the value of boot_locked below.
+		 */
+		card->ext_csd.boot_locked = ext_csd[EXT_CSD_BOOT_WP] &
+			(EXT_CSD_BOOT_WP_B_PERM_WP_EN |
+			 EXT_CSD_BOOT_WP_B_PWR_WP_EN);
+
+		/*
 		 * There are two boot regions of equal size, defined in
 		 * multiples of 128K.
 		 */
@@ -347,7 +356,8 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
 				part_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;
 				mmc_part_add(card, part_size,
 					EXT_CSD_PART_CONFIG_ACC_BOOT0 + idx,
-					"boot%d", idx, true);
+					"boot%d", idx, true,
+					MMC_BLK_DATA_AREA_BOOT);
 			}
 		}
 	}
@@ -434,7 +444,8 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
 					hc_wp_grp_sz);
 				mmc_part_add(card, part_size << 19,
 					EXT_CSD_PART_CONFIG_ACC_GP0 + idx,
-					"gp%d", idx, false);
+					"gp%d", idx, false,
+					MMC_BLK_DATA_AREA_GP);
 			}
 		}
 		card->ext_csd.sec_trim_mult =
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 6e04e10..3ed333f 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -71,6 +71,7 @@ struct mmc_ext_csd {
 	bool			hpi_en;			/* HPI enablebit */
 	bool			hpi;			/* HPI support bit */
 	unsigned int		hpi_cmd;		/* cmd used as HPI */
+	unsigned int		boot_locked;
 	u8			raw_partition_support;	/* 160 */
 	u8			raw_erased_mem_count;	/* 181 */
 	u8			raw_ext_csd_structure;	/* 194 */
@@ -184,6 +185,10 @@ struct mmc_part {
 	unsigned int	part_cfg;	/* partition type */ 
 	char	name[MAX_MMC_PART_NAME_LEN];
 	bool	force_ro;	/* to make boot parts RO by default */
+	int	area_type;
+#define MMC_BLK_DATA_AREA_MAIN	0
+#define MMC_BLK_DATA_AREA_BOOT	1
+#define MMC_BLK_DATA_AREA_GP	2
 };
 
 /*
@@ -260,12 +265,14 @@ struct mmc_card {
  * This function fill contents in mmc_part.
  */
 static inline void mmc_part_add(struct mmc_card *card, unsigned int size,
-			unsigned int part_cfg, char *name, int idx, bool ro)
+			unsigned int part_cfg, char *name, int idx, bool ro,
+			int area_type)
 {
 	card->part[card->nr_parts].size = size;
 	card->part[card->nr_parts].part_cfg = part_cfg;
 	sprintf(card->part[card->nr_parts].name, name, idx);
 	card->part[card->nr_parts].force_ro = ro;
+	card->part[card->nr_parts].area_type = area_type;
 	card->nr_parts++;
 }
 
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index 0e71356..665548e 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -280,6 +280,7 @@ struct _mmc_csd {
 #define EXT_CSD_RST_N_FUNCTION		162	/* R/W */
 #define EXT_CSD_SANITIZE_START		165     /* W */
 #define EXT_CSD_WR_REL_PARAM		166	/* RO */
+#define EXT_CSD_BOOT_WP			173	/* R/W */
 #define EXT_CSD_ERASE_GROUP_DEF		175	/* R/W */
 #define EXT_CSD_PART_CONFIG		179	/* R/W */
 #define EXT_CSD_ERASED_MEM_CONT		181	/* RO */
@@ -321,6 +322,11 @@ struct _mmc_csd {
 
 #define EXT_CSD_WR_REL_PARAM_EN		(1<<2)
 
+#define EXT_CSD_BOOT_WP_B_PWR_WP_DIS	(0x40)
+#define EXT_CSD_BOOT_WP_B_PERM_WP_DIS	(0x10)
+#define EXT_CSD_BOOT_WP_B_PERM_WP_EN	(0x04)
+#define EXT_CSD_BOOT_WP_B_PWR_WP_EN	(0x01)
+
 #define EXT_CSD_PART_CONFIG_ACC_MASK	(0x7)
 #define EXT_CSD_PART_CONFIG_ACC_BOOT0	(0x1)
 #define EXT_CSD_PART_CONFIG_ACC_GP0	(0x4)
-- 
1.7.5.4

Re: [PATCH] mmc: boot partition ro lock support

[Andrei Warkentin]

Hi Ulf, Johan,

----- Original Message -----
> From: "Ulf Hansson" <ulf.hansson@stericsson.com>
> To: linux-mmc@vger.kernel.org, "Chris Ball" <cjb@laptop.org>
> Cc: "Per Forlin" <per.forlin@stericsson.com>, "Ulf Hansson" <ulf.hansson@stericsson.com>, "Lee Jones"
> <lee.jones@linaro.org>, "Johan Rudholm" <johan.rudholm@stericsson.com>, "John Beckett" <john.beckett@stericsson.com>
> Sent: Friday, October 21, 2011 9:17:23 AM
> Subject: [PATCH] mmc: boot partition ro lock support
> 
> From: Johan Rudholm <johan.rudholm@stericsson.com>
> 
> Enable boot partitions to be power and permanently read-only locked
> via
> a sysfs entry. There will be one sysfs entry for the main mmc device:
> 
> /sys/block/mmcblkX/boot_partition_ro_lock
> 
> and one for each boot partition:
> 
> /sys/block/mmcblkXbootY/ro_lock
> 
> The boot partitions are power or permanently locked by writing
> "pwr_ro"
> or "perm_ro" to one of the files.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
> Signed-off-by: John Beckett <john.beckett@stericsson.com>
> Signed-off-by: Johan Rudholm <johan.rudholm@stericsson.com>
> ---

What does power locking do that force_ro currently doesn't achieve?

The permalocking brick-potential (more like paper-weight-potential) is IMO unacceptably
high that something like this is just accessible via a sysfs attribute. This is exactly
why the boot partitions were put under force_ro, so that some poor sap wouldn't end up
nuking the boot partitions (with obvious consequences), and permalocking seems even nastier.

At a bigger picture, I'm uncertain what the point of this is. It adds code complexity for,
a generally rare and possibly self-destructive operation. This is like the GP partitioning 
support - it's a once-a-time (and unrecoverable) operation usually done at manufacturing (or initial
provisioning time) that shouldn't be even exposed to the general purpose user. This would be a
good use of that arbitrary-CMD ioctl interface John Calixto put in.

Thanks,
A

Re: [PATCH] mmc: boot partition ro lock support

[Chris Ball]

Hi,

(Andrei, looks like your mails are being hard line wrapped around 100 cols.)

On Fri, Oct 21 2011, Andrei Warkentin wrote:
> What does power locking do that force_ro currently doesn't achieve?

The power-lock is used to go read only until the next time power is
reset, even if the kernel later asks for r/w.  This is used on some
devices such as the HTC Desire Z/G2 as a security mechanism -- the
bootloader switches to power r/o just before running the kernel, so
the kernel itself can't modify the boot kernel image.

.. except it can, because the G2 hackers worked out how to glitch the
eMMC's power rail using a kernel module that hits a GPIO, making it come
out of r/o, and managed to make the MMC layer cope with the device
needing reinit without crashing userspace.  But you get the idea.

> The permalocking brick-potential (more like paper-weight-potential) is
> IMO unacceptably high that something like this is just accessible via
> a sysfs attribute. This is exactly why the boot partitions were put
> under force_ro, so that some poor sap wouldn't end up nuking the boot
> partitions (with obvious consequences), and permalocking seems even
> nastier.

I agree.  Does anyone have an argument for including either of these?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: boot partition ro lock support

[Linus Walleij]

On Sat, Oct 22, 2011 at 12:32 PM, Chris Ball <cjb@laptop.org> wrote:
> On Fri, Oct 21 2011, Andrei Warkentin wrote:
>>
>> The permalocking brick-potential (more like paper-weight-potential) is
>> IMO unacceptably high that something like this is just accessible via
>> a sysfs attribute. This is exactly why the boot partitions were put
>> under force_ro, so that some poor sap wouldn't end up nuking the boot
>> partitions (with obvious consequences), and permalocking seems even
>> nastier.

I presume the sysfs files are there exactly to avoid bricking,
by locking partitions down from userspace. So what you're saying
is that this should be done by the kernel itself or bootloader?

[Chris]
> I agree.  Does anyone have an argument for including either of these?

I see the problem with having it as sysfs files, so what is our
rationale about using that documented MMC feature? I can
think of two:

1) Use a kernel cmdline param to permalock partitions

2) Only bootloaders should do such stuff

As noticed all over ARM linux' mailing lists boot loader updates are
nasty stuff, usually it's pretty hard to alter stuff there to get what
you want. Not all boot loaders are as sophisticated cmdline parsers
as U-Boot mind you...

So what about a cmdline approach? That makes it possible
for people who are willingly recompiling and hacking their kernels
to tinker with this if they absolutely want to go in on that
partition, make it rw and change stuff.

...org have I just got all this backwards...?

Yours,
Linus Walleij

Re: [PATCH] mmc: boot partition ro lock support

[Chris Ball]

Hi Linus,

On Sat, Oct 22 2011, Linus Walleij wrote:
> So what about a cmdline approach? That makes it possible
> for people who are willingly recompiling and hacking their kernels
> to tinker with this if they absolutely want to go in on that
> partition, make it rw and change stuff.
>
> ...org have I just got all this backwards...?

Regarding this patch, we're not so worried that someone will take a
read-only boot partition and make it read-write and do something wrong;
sysfs is good enough to protect against that happening (at least by
mistake).

What we're worried about is someone issuing the perm read-only command,
and not realizing that it really means that they can never ever write
any more changes to their eMMC -- it's a one-time fuse -- and perhaps
further not realizing that this may make their expensive cell phone
become a brick of some sort, depending on how much the bootloader or
OS depends on having r/w access.

I don't even know that I'd want to make such an option available via a
boot argument, since boot arguments are often misunderstood.  As Andrei
says, it doesn't sound like something a regular user (as opposed to an
OEM) should ever have reason to do.  I'd rather leave it to specialized
manufacturing equipment.

Does that make sense?  Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: boot partition ro lock support

[Sebastian Rasmussen]

Hi!

> What we're worried about is someone issuing the perm read-only command,
> and not realizing that it really means that they can never ever write
> any more changes to their eMMC -- it's a one-time fuse

I can see why you are worried that people may brick their devices.
How about only adding the read-only-until-power-cycled command?

> I'd rather leave it to specialized manufacturing equipment.

Sure, but then again permanent read-only commands seem to be
able to be sent by writing a userspace tool that issues a ioctl(fd, 0xb3, ...)
using the generic command interface by John Calixto mentioned by
Andrei. I assume that what reassures you in this case is
that CAP_SYS_RAWIO is required and perhaps also obscurity?

 / Sebastian

Re: [PATCH] mmc: boot partition ro lock support

[Chris Ball]

Hi Sebastian,

On Sat, Oct 22 2011, Sebastian Rasmussen wrote:
> Hi!
>
>> What we're worried about is someone issuing the perm read-only command,
>> and not realizing that it really means that they can never ever write
>> any more changes to their eMMC -- it's a one-time fuse
>
> I can see why you are worried that people may brick their devices.
> How about only adding the read-only-until-power-cycled command?

I think that makes sense.  I'd be curious to hear if anyone thinks we
shouldn't even add that command, perhaps on grounds that "the kernel
shouldn't be enthusiastic about locking itself out of future access to
a device" or similar.  As you say, the ioctl interface would still work.

>> I'd rather leave it to specialized manufacturing equipment.
>
> Sure, but then again permanent read-only commands seem to be
> able to be sent by writing a userspace tool that issues a ioctl(fd, 0xb3, ...)
> using the generic command interface by John Calixto mentioned by
> Andrei. I assume that what reassures you in this case is
> that CAP_SYS_RAWIO is required and perhaps also obscurity?

Yes, that's right -- running a userspace program that you explicitly
downloaded from somewhere and compiled is more intentional than
wondering what a kernel argument or sysfs node does and trying it.
(Maybe I'm special, but I often use kernel arguments and sysfs nodes
without reading their code or finding the best documentation for them
first, when trying to get something to work.)

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: boot partition ro lock support

[Ulf Hansson]

Chris Ball wrote:
> Hi Sebastian,
> 
> On Sat, Oct 22 2011, Sebastian Rasmussen wrote:
>> Hi!
>>
>>> What we're worried about is someone issuing the perm read-only command,
>>> and not realizing that it really means that they can never ever write
>>> any more changes to their eMMC -- it's a one-time fuse
>> I can see why you are worried that people may brick their devices.
>> How about only adding the read-only-until-power-cycled command?
> 
> I think that makes sense.  I'd be curious to hear if anyone thinks we
> shouldn't even add that command, perhaps on grounds that "the kernel
> shouldn't be enthusiastic about locking itself out of future access to
> a device" or similar.  As you say, the ioctl interface would still work.
> 
>>> I'd rather leave it to specialized manufacturing equipment.
>> Sure, but then again permanent read-only commands seem to be
>> able to be sent by writing a userspace tool that issues a ioctl(fd, 0xb3, ...)
>> using the generic command interface by John Calixto mentioned by
>> Andrei. I assume that what reassures you in this case is
>> that CAP_SYS_RAWIO is required and perhaps also obscurity?
> 
> Yes, that's right -- running a userspace program that you explicitly
> downloaded from somewhere and compiled is more intentional than
> wondering what a kernel argument or sysfs node does and trying it.
> (Maybe I'm special, but I often use kernel arguments and sysfs nodes
> without reading their code or finding the best documentation for them
> first, when trying to get something to work.)

The kernel could very much be used in manufacturing environment as well. 
Don't know if and how we should consider this.

Do you think a change to an ioctl is a better alternative than sysfs? 
Then let's change to that!

BR
Ulf Hansson

Re: [PATCH] mmc: boot partition ro lock support

[Chris Ball]

Hi Ulf,

On Mon, Oct 24 2011, Ulf Hansson wrote:
> The kernel could very much be used in manufacturing environment as
> well. Don't know if and how we should consider this.
>
> Do you think a change to an ioctl is a better alternative than sysfs?
> Then let's change to that!

Ah, we're not proposing a new ioctl for this -- we're proposing that the
perm-r/o command could be sent from a userspace binary that *uses* the
arbitrary command ioctl that we already have.

The arbitrary command ioctl is drivers/mmc/card/block.c:mmc_blk_ioctl_cmd().

Please could you send a version of the patch that supports power-on r/o
but not perm r/o, and perhaps also post userspace code for submitting
the perm r/o command through the ioctl interface?  (Perhaps this code
should eventually be submitted to a disk management tool..)

Also, it looks like it's possible to set read-only for the whole device,
as well as for boot partitions.  Could you update the patch to allow
setting power-on r/o for the entire card, not just the boot partitions?

> Enable boot partitions to be power and permanently read-only locked via
> a sysfs entry. There will be one sysfs entry for the main mmc device:
> 
> /sys/block/mmcblkX/boot_partition_ro_lock

I don't follow why this node is necessary, instead of just having the
nodes in the mmcblkXbootY/ directories.  Could you explain?

> and one for each boot partition:
> 
> /sys/block/mmcblkXbootY/ro_lock
>
> The boot partitions are power or permanently locked by writing "pwr_ro"
> or "perm_ro" to one of the files.

Now that we're only having one type of r/o lock, let's do:

  echo 1 > /sys/block/mmcblkXbootY/ro_lock_until_next_power_on

to lock just a boot partition, and:

  echo 1 > /sys/block/mmcblkX/ro_lock_until_next_power_on

to lock the entire card (which will cover the boot partition too), with
echo 0 to unlock.

Other review comments:  I noticed that your pr_*(); statements are
missing "\n"s on the end, and you should add documentation on the new
sysfs nodes to:
   Documentation/mmc/mmc-dev-parts.txt

I think the overlap between your patch and Andrei's mmcblkXbootY/force_ro
node is going to be confusing -- one operates purely in the kernel and
the other is sent to the card.  Do you have any proposal for making the
difference clearer?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

RE: [PATCH] mmc: boot partition ro lock support

[Johan RUDHOLM]

Hi,

Chris Ball wrote:
> 
> Please could you send a version of the patch that supports power-on r/o
> but not perm r/o, and perhaps also post userspace code for submitting
> the perm r/o command through the ioctl interface?  (Perhaps this code
> should eventually be submitted to a disk management tool..)

Ok and ok, we will lose the permanent ro-locking and supply sample code for future reference.

> Also, it looks like it's possible to set read-only for the whole
> device,
> as well as for boot partitions.  Could you update the patch to allow
> setting power-on r/o for the entire card, not just the boot partitions?

Will give it a shot!

> > Enable boot partitions to be power and permanently read-only locked
> via
> > a sysfs entry. There will be one sysfs entry for the main mmc device:
> >
> > /sys/block/mmcblkX/boot_partition_ro_lock
> 
> I don't follow why this node is necessary, instead of just having the
> nodes in the mmcblkXbootY/ directories.  Could you explain?

This was meant to reflect that ro-locking the boot partitions is actually done per card and not per boot partition (so if you lock one boot partition, the other one is locked as well). Is there another way of making this connection clear? Maybe it will be enough to mention it in the Documentation?

>   echo 1 > /sys/block/mmcblkXbootY/ro_lock_until_next_power_on
> 
> to lock just a boot partition, and:
> 
>   echo 1 > /sys/block/mmcblkX/ro_lock_until_next_power_on
> 
> to lock the entire card (which will cover the boot partition too), with
> echo 0 to unlock.

Ok, this looks good. However, writing 0 will not unlock since the card needs to be power cycled.

> Other review comments:  I noticed that your pr_*(); statements are
> missing "\n"s on the end, and you should add documentation on the new
> sysfs nodes to:
>    Documentation/mmc/mmc-dev-parts.txt

Ok!

> I think the overlap between your patch and Andrei's
> mmcblkXbootY/force_ro
> node is going to be confusing -- one operates purely in the kernel and
> the other is sent to the card.  Do you have any proposal for making the
> difference clearer?

I concur, the same can be said about general purpose partitions as well? Partitions that are configured in hardware rather than software. The current layout in sysfs does not reflect this difference either, below

mmcblk0 

we find

mmcblk0p1
mmcblk0boot0
mmcblk0gp0

which are a mix of different partition types. Maybe separate sysfs trees in some way? I am afraid I'll have to pass this question back to you.

Thank you all for your comments.

Kind regards, Johan

Re: [PATCH] mmc: boot partition ro lock support

[J Freyensee]

On 10/22/2011 11:38 PM, Chris Ball wrote:
> Hi Sebastian,
>
> On Sat, Oct 22 2011, Sebastian Rasmussen wrote:
>> Hi!
>>
>>> What we're worried about is someone issuing the perm read-only command,
>>> and not realizing that it really means that they can never ever write
>>> any more changes to their eMMC -- it's a one-time fuse
>>
>> I can see why you are worried that people may brick their devices.
>> How about only adding the read-only-until-power-cycled command?
>
> I think that makes sense.  I'd be curious to hear if anyone thinks we
> shouldn't even add that command, perhaps on grounds that "the kernel
> shouldn't be enthusiastic about locking itself out of future access to
> a device" or similar.  As you say, the ioctl interface would still work.

The only reason why I could think you would want a command like that is 
if the kernel goes viral on a normal user on say, a cell phone.  Then 
this would be more of defensive mechanism that would be tripped.

I think if a hacker/kernel-modifier bricks their device, then that was 
their risk, but for a normal user (which makes up the majority of 
Android phones in which Android is the majority smart phones out there), 
it should be hard to brick a device.

Jay

>
>>> I'd rather leave it to specialized manufacturing equipment.
>>
>> Sure, but then again permanent read-only commands seem to be
>> able to be sent by writing a userspace tool that issues a ioctl(fd, 0xb3, ...)
>> using the generic command interface by John Calixto mentioned by
>> Andrei. I assume that what reassures you in this case is
>> that CAP_SYS_RAWIO is required and perhaps also obscurity?
>
> Yes, that's right -- running a userspace program that you explicitly
> downloaded from somewhere and compiled is more intentional than
> wondering what a kernel argument or sysfs node does and trying it.
> (Maybe I'm special, but I often use kernel arguments and sysfs nodes
> without reading their code or finding the best documentation for them
> first, when trying to get something to work.)
>
> Thanks,
>
> - Chris.


-- 
J (James/Jay) Freyensee
Storage Technology Group
Intel Corporation

Re: [PATCH] mmc: boot partition ro lock support

[Andrei E. Warkentin]

2011/10/24 Johan RUDHOLM <johan.rudholm@stericsson.com>:

>> I think the overlap between your patch and Andrei's
>> mmcblkXbootY/force_ro
>> node is going to be confusing -- one operates purely in the kernel and
>> the other is sent to the card.  Do you have any proposal for making the
>> difference clearer?
>
> I concur, the same can be said about general purpose partitions as well? Partitions that are configured in hardware rather than software. The current layout in sysfs does not reflect this difference either, below
>

Well, they're not really partitions, though, but separate logical
devices that can be
configred only once,  but I generally agree with you.

My personal opinion is that since the kernel is not used solely in a
controlled, embedded
environment, effort should be done to reduce the number of physically damaging
actions a curious user could do :-). I don't mean "formatting disk", I
mean "raising Vcc
to the point of smoking device", bricking, exploding, etc. In line
with this thought,
I think it should be more difficult to permanently lock a card - an
ioctl (whether
specific or generic, involving knowing what command to send) I think
is the better solution.

As far as the temporary locking, I'm still not really convinced, but
I'll say I don't have a
strong opinion about it. Certainly, as an implementer, I would do so
in the bootloader
rather than the kernel (why? for Android - because you can fastboot
boot kernels via USB,
or maybe some bootloader somewhere supports booting from SD, so your
security policy
should be enforced as soon as possible, instead of relying on booting
your trusted image).

A

RE: [PATCH] mmc: boot partition ro lock support

[Johan RUDHOLM]

Hi all,

Chris Ball wrote:
> Ah, we're not proposing a new ioctl for this -- we're proposing that
> the
> perm-r/o command could be sent from a userspace binary that *uses* the
> arbitrary command ioctl that we already have.
> 
> The arbitrary command ioctl is
> drivers/mmc/card/block.c:mmc_blk_ioctl_cmd().
> 
> Please could you send a version of the patch that supports power-on r/o
> but not perm r/o, and perhaps also post userspace code for submitting
> the perm r/o command through the ioctl interface?  (Perhaps this code
> should eventually be submitted to a disk management tool..)

I've reworked the patch now, and will send it in a moment. I've also written some sample userspace code using the ioctl, but I'm having a hard time getting it to issue the MMC_SWITCH opcode -- the ioctl call never returns.

I believe this problem is similar to the issue discussed in a separate thread (Extension of MMC Block IOCTL ...), since I'm sending no data with the data_ptr. I have a patch for this as well, which does not initiate the struct mmc_data if blksz and blocks is zero, but I'm not sure this is the proper way to do it for all commands and uses. I can send it if there is interest.

> Also, it looks like it's possible to set read-only for the whole
> device,
> as well as for boot partitions.  Could you update the patch to allow
> setting power-on r/o for the entire card, not just the boot partitions?

Hmm, I'm not able to find this in the spec -- could you point a little closer please?

> I think the overlap between your patch and Andrei's
> mmcblkXbootY/force_ro
> node is going to be confusing -- one operates purely in the kernel and
> the other is sent to the card.  Do you have any proposal for making the
> difference clearer?

At least it is mentioned in the documentation now.

Kind regards, Johan