Re: [PATCH v8 3/3] mtd: rawnand: Add support for secure regions in NAND memory
On Thu, 1 Apr 2021 15:48:12 +0530
Manivannan Sadhasivam wrote:
> static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs)
> {
> + struct mtd_info *mtd = nand_to_mtd(chip);
> + int last_page = ((mtd->erasesize - mtd->writesize) >>
> +chip->page_shift) & chip->pagemask;
> int ret;
>
> if (chip->options & NAND_NO_BBM_QUIRK)
> return 0;
>
> /* Check if the region is secured */
> - ret = nand_check_secure_region(chip, ofs, 0);
> + ret = nand_check_secure_region(chip, ofs, last_page);
or just:
ret = nand_check_secure_region(chip, ofs, mtd->erasesize);
> if (ret)
> return ret;
>
> > */
> >
Re: [PATCH v8 3/3] mtd: rawnand: Add support for secure regions in NAND memory
Hi Miquel,
On Tue, Mar 23, 2021 at 05:57:15PM +0100, Miquel Raynal wrote:
> Hi Manivannan,
>
> Manivannan Sadhasivam wrote on Tue,
> 23 Mar 2021 13:09:30 +0530:
>
> > On a typical end product, a vendor may choose to secure some regions in
> > the NAND memory which are supposed to stay intact between FW upgrades.
> > The access to those regions will be blocked by a secure element like
> > Trustzone. So the normal world software like Linux kernel should not
> > touch these regions (including reading).
> >
> > The regions are declared using a NAND chip DT property,
> > "secure-regions". So let's make use of this property in the raw NAND
> > core and skip access to the secure regions present in a system.
> >
> > Signed-off-by: Manivannan Sadhasivam
> > ---
> > drivers/mtd/nand/raw/nand_base.c | 105 +++
> > include/linux/mtd/rawnand.h | 14 +
> > 2 files changed, 119 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_base.c
> > b/drivers/mtd/nand/raw/nand_base.c
> > index c33fa1b1847f..2a990219f498 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -278,11 +278,46 @@ static int nand_block_bad(struct nand_chip *chip,
> > loff_t ofs)
> > return 0;
> > }
> >
> > +/**
> > + * nand_check_secure_region() - Check if the region is secured
> > + * @chip: NAND chip object
> > + * @offset: Offset of the region to check
> > + * @size: Size of the region to check
> > + *
> > + * Checks if the region is secured by comparing the offset and size with
> > the
> > + * list of secure regions obtained from DT. Returns -EIO if the region is
> > + * secured else 0.
> > + */
> > +static int nand_check_secure_region(struct nand_chip *chip, loff_t offset,
> > u64 size)
>
> I think I would prefer a boolean return value here, with a rename:
>
> static bool nand_region_is_secured() or
> nand_region_is_accessible/reachable/whatever()
>
> then something lik:
>
> if (nand_region_is_secured())
> return -EIO;
>
Okay
> > +{
> > + int i;
> > +
> > + /* Skip touching the secure regions if present */
> > + for (i = 0; i < chip->nr_secure_regions; i++) {
> > + const struct nand_secure_region *region =
> > >secure_regions[i];
> > +
> > + if (offset + size < region->offset ||
> > + offset >= region->offset + region->size)
>
> I think as-is the condition does not work.
>
> Let's assume we want to check the region { .offset = 1, size = 1 } and
> the region { .offset = 2, size = 1 } is reserved. This is:
>
> if ((1 + 1 < 2) /* false */ ||
> (1 >= 2 + 1) /* false */)
> continue;
> return -EIO; /* EIO is returned while the area is valid
I made a mistake. I should've used "offset + size <= region->offset" as
suggested by Boris.
The reason why I didn't go for it because the SoC was still accessing
the secure region with (>=). So I went with just (>) blindly :/
The actual issue was with the check at nand_isbad_bbm(), where I didn't
pass the size of the region to check, instead just offset as below:
nand_check_secure_region(chip, ofs, 0);
Because of this, the check went fine but since the block_bad() function
reads the blocks starting from the offset, the secure region was
accessed.
For fixing this, I'm going to use below diff:
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 2a990219f498..53589c835f66 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -296,7 +296,7 @@ static int nand_check_secure_region(struct nand_chip *chip,
loff_t offset, u64 s
for (i = 0; i < chip->nr_secure_regions; i++) {
const struct nand_secure_region *region =
>secure_regions[i];
- if (offset + size < region->offset ||
+ if (offset + size <= region->offset ||
offset >= region->offset + region->size)
continue;
@@ -308,13 +308,16 @@ static int nand_check_secure_region(struct nand_chip
*chip, loff_t offset, u64 s
static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs)
{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ int last_page = ((mtd->erasesize - mtd->writesize) >>
+chip->page_shift) & chip->pagemask;
int ret;
if (chip->options & NAND_NO_BBM_QUIRK)
return 0;
/* Check if the region is secured */
- ret = nand_check_secure_region(chip, ofs, 0);
+ ret = nand_check_secure_region(chip, ofs, last_page);
if (ret)
return ret;
> */
>
> > + continue;
> > +
>
> Perhaps a dev_dbg() entry here would make sense.
>
Okay
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
>
> [...]
>
> > +static int of_get_nand_secure_regions(struct nand_chip *chip)
> > +{
> > + struct
Re: [PATCH v8 3/3] mtd: rawnand: Add support for secure regions in NAND memory
Hi Manivannan,
Manivannan Sadhasivam wrote on Tue,
23 Mar 2021 13:09:30 +0530:
> On a typical end product, a vendor may choose to secure some regions in
> the NAND memory which are supposed to stay intact between FW upgrades.
> The access to those regions will be blocked by a secure element like
> Trustzone. So the normal world software like Linux kernel should not
> touch these regions (including reading).
>
> The regions are declared using a NAND chip DT property,
> "secure-regions". So let's make use of this property in the raw NAND
> core and skip access to the secure regions present in a system.
>
> Signed-off-by: Manivannan Sadhasivam
> ---
> drivers/mtd/nand/raw/nand_base.c | 105 +++
> include/linux/mtd/rawnand.h | 14 +
> 2 files changed, 119 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index c33fa1b1847f..2a990219f498 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -278,11 +278,46 @@ static int nand_block_bad(struct nand_chip *chip,
> loff_t ofs)
> return 0;
> }
>
> +/**
> + * nand_check_secure_region() - Check if the region is secured
> + * @chip: NAND chip object
> + * @offset: Offset of the region to check
> + * @size: Size of the region to check
> + *
> + * Checks if the region is secured by comparing the offset and size with the
> + * list of secure regions obtained from DT. Returns -EIO if the region is
> + * secured else 0.
> + */
> +static int nand_check_secure_region(struct nand_chip *chip, loff_t offset,
> u64 size)
I think I would prefer a boolean return value here, with a rename:
static bool nand_region_is_secured() or
nand_region_is_accessible/reachable/whatever()
then something lik:
if (nand_region_is_secured())
return -EIO;
> +{
> + int i;
> +
> + /* Skip touching the secure regions if present */
> + for (i = 0; i < chip->nr_secure_regions; i++) {
> + const struct nand_secure_region *region =
> >secure_regions[i];
> +
> + if (offset + size < region->offset ||
> + offset >= region->offset + region->size)
I think as-is the condition does not work.
Let's assume we want to check the region { .offset = 1, size = 1 } and
the region { .offset = 2, size = 1 } is reserved. This is:
if ((1 + 1 < 2) /* false */ ||
(1 >= 2 + 1) /* false */)
continue;
return -EIO; /* EIO is returned while the area is valid
*/
> + continue;
> +
Perhaps a dev_dbg() entry here would make sense.
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
[...]
> +static int of_get_nand_secure_regions(struct nand_chip *chip)
> +{
> + struct device_node *dn = nand_get_flash_node(chip);
> + struct property *prop;
> + int length, nr_elem, i, j;
> +
> + prop = of_find_property(dn, "secure-regions", );
> + if (prop) {
I generally prefer the below logic:
if (!prop)
return 0;
Then you earn an indentation level.
> + nr_elem = length / sizeof(u64);
of_property_count_elems_of_size() ?
> + chip->nr_secure_regions = nr_elem / 2;
> +
> + chip->secure_regions = kcalloc(nr_elem,
> sizeof(*chip->secure_regions), GFP_KERNEL);
IIRC ->secure_regions is a structure with lengths and offset, so you
don't want to allocate nr_elem but nr_secure_regions number of
items here.
> + if (!chip->secure_regions)
> + return -ENOMEM;
> +
> + for (i = 0, j = 0; i < chip->nr_secure_regions; i++, j += 2) {
> + of_property_read_u64_index(dn, "secure-regions", j,
> +
> >secure_regions[i].offset);
> + of_property_read_u64_index(dn, "secure-regions", j + 1,
> +
> >secure_regions[i].size);
> + }
> + }
> +
> + return 0;
> +}
> +
> static int rawnand_dt_init(struct nand_chip *chip)
> {
> struct nand_device *nand = mtd_to_nanddev(nand_to_mtd(chip));
> struct device_node *dn = nand_get_flash_node(chip);
> + int ret;
>
> if (!dn)
> return 0;
> @@ -5015,6 +5107,16 @@ static int rawnand_dt_init(struct nand_chip *chip)
> of_get_nand_ecc_user_config(nand);
> of_get_nand_ecc_legacy_user_config(chip);
>
> + /*
> + * Look for secure regions in the NAND chip. These regions are supposed
> + * to be protected by a secure element like Trustzone. So the read/write
> + * accesses to these regions will be blocked in the runtime by this
> + * driver.
> + */
> + ret = of_get_nand_secure_regions(chip);
> + if (!ret)
> + return ret;
I think we can do this initialization pretty much when we want in the
init process as long as
[PATCH v8 3/3] mtd: rawnand: Add support for secure regions in NAND memory
On a typical end product, a vendor may choose to secure some regions in
the NAND memory which are supposed to stay intact between FW upgrades.
The access to those regions will be blocked by a secure element like
Trustzone. So the normal world software like Linux kernel should not
touch these regions (including reading).
The regions are declared using a NAND chip DT property,
"secure-regions". So let's make use of this property in the raw NAND
core and skip access to the secure regions present in a system.
Signed-off-by: Manivannan Sadhasivam
---
drivers/mtd/nand/raw/nand_base.c | 105 +++
include/linux/mtd/rawnand.h | 14 +
2 files changed, 119 insertions(+)
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index c33fa1b1847f..2a990219f498 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -278,11 +278,46 @@ static int nand_block_bad(struct nand_chip *chip, loff_t
ofs)
return 0;
}
+/**
+ * nand_check_secure_region() - Check if the region is secured
+ * @chip: NAND chip object
+ * @offset: Offset of the region to check
+ * @size: Size of the region to check
+ *
+ * Checks if the region is secured by comparing the offset and size with the
+ * list of secure regions obtained from DT. Returns -EIO if the region is
+ * secured else 0.
+ */
+static int nand_check_secure_region(struct nand_chip *chip, loff_t offset, u64
size)
+{
+ int i;
+
+ /* Skip touching the secure regions if present */
+ for (i = 0; i < chip->nr_secure_regions; i++) {
+ const struct nand_secure_region *region =
>secure_regions[i];
+
+ if (offset + size < region->offset ||
+ offset >= region->offset + region->size)
+ continue;
+
+ return -EIO;
+ }
+
+ return 0;
+}
+
static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs)
{
+ int ret;
+
if (chip->options & NAND_NO_BBM_QUIRK)
return 0;
+ /* Check if the region is secured */
+ ret = nand_check_secure_region(chip, ofs, 0);
+ if (ret)
+ return ret;
+
if (chip->legacy.block_bad)
return chip->legacy.block_bad(chip, ofs);
@@ -397,6 +432,11 @@ static int nand_do_write_oob(struct nand_chip *chip,
loff_t to,
return -EINVAL;
}
+ /* Check if the region is secured */
+ ret = nand_check_secure_region(chip, to, ops->ooblen);
+ if (ret)
+ return ret;
+
chipnr = (int)(to >> chip->chip_shift);
/*
@@ -565,6 +605,11 @@ static int nand_block_isreserved(struct mtd_info *mtd,
loff_t ofs)
if (!chip->bbt)
return 0;
+
+ /* Check if the region is secured */
+ if (nand_check_secure_region(chip, ofs, 0))
+ return -EIO;
+
/* Return info from the table */
return nand_isreserved_bbt(chip, ofs);
}
@@ -3127,6 +3172,11 @@ static int nand_do_read_ops(struct nand_chip *chip,
loff_t from,
int retry_mode = 0;
bool ecc_fail = false;
+ /* Check if the region is secured */
+ ret = nand_check_secure_region(chip, from, readlen);
+ if (ret)
+ return ret;
+
chipnr = (int)(from >> chip->chip_shift);
nand_select_target(chip, chipnr);
@@ -3458,6 +3508,11 @@ static int nand_do_read_oob(struct nand_chip *chip,
loff_t from,
pr_debug("%s: from = 0x%08Lx, len = %i\n",
__func__, (unsigned long long)from, readlen);
+ /* Check if the region is secured */
+ ret = nand_check_secure_region(chip, from, readlen);
+ if (ret)
+ return ret;
+
stats = mtd->ecc_stats;
len = mtd_oobavail(mtd, ops);
@@ -3979,6 +4034,11 @@ static int nand_do_write_ops(struct nand_chip *chip,
loff_t to,
return -EINVAL;
}
+ /* Check if the region is secured */
+ ret = nand_check_secure_region(chip, to, writelen);
+ if (ret)
+ return ret;
+
column = to & (mtd->writesize - 1);
chipnr = (int)(to >> chip->chip_shift);
@@ -4180,6 +4240,11 @@ int nand_erase_nand(struct nand_chip *chip, struct
erase_info *instr,
if (check_offs_len(chip, instr->addr, instr->len))
return -EINVAL;
+ /* Check if the region is secured */
+ ret = nand_check_secure_region(chip, instr->addr, instr->len);
+ if (ret)
+ return ret;
+
/* Grab the lock and see if the device is available */
ret = nand_get_device(chip);
if (ret)
@@ -4995,10 +5060,37 @@ static bool of_get_nand_on_flash_bbt(struct device_node
*np)
return of_property_read_bool(np, "nand-on-flash-bbt");
}
+static int of_get_nand_secure_regions(struct nand_chip *chip)
+{
+ struct device_node *dn = nand_get_flash_node(chip);
+ struct property *prop;
+
