| Message ID | 20210323073930.89754-4-manivannan.sadhasivam@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for secure regions in NAND | expand |
Hi Manivannan, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> 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 <manivannan.sadhasivam@linaro.org> > --- > 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 = &chip->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", &length); > + 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, > + &chip->secure_regions[i].offset); > + of_property_read_u64_index(dn, "secure-regions", j + 1, > + &chip->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 it is done before the BBT parsing logic. Here, besides the fact the memory will not be freed from rawnand_dt_init()'s caller if something goes wrong, we are at a point where nand_cleanup will not be called. nand_cleanup() will only be called if the controller driver encounters an error *after* a successful nand_scan(). We could perhaps move this call to nand_scan() which would simply solve the situation. We don't need it in rawnand_dt_init() as this won't be rawnand specific anyway... > + > /* > * If neither the user nor the NAND controller have > requested a specific > * ECC engine type, we will default to > NAND_ECC_ENGINE_TYPE_ON_HOST. @@ -6068,6 +6170,9 @@ void > nand_cleanup(struct nand_chip *chip) /* Free manufacturer priv data. > */ nand_manufacturer_cleanup(chip); > > + /* Free secure regions data */ > + kfree(chip->secure_regions); > + > /* Free controller specific allocations after chip > identification */ nand_detach(chip); > > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h > index 6b3240e44310..17ddc900a1dc 100644 > --- a/include/linux/mtd/rawnand.h > +++ b/include/linux/mtd/rawnand.h > @@ -1036,6 +1036,16 @@ struct nand_manufacturer { > void *priv; > }; > > +/** > + * struct nand_secure_region - NAND secure region structure > + * @offset: Offset of the start of the secure region > + * @size: Size of the secure region > + */ > +struct nand_secure_region { > + u64 offset; > + u64 size; > +}; > + > /** > * struct nand_chip - NAND Private Flash Chip Data > * @base: Inherit from the generic NAND device > @@ -1086,6 +1096,8 @@ struct nand_manufacturer { > * NAND Controller drivers should not modify this value, > but they're > * allowed to read it. > * @read_retries: The number of read retry modes supported > + * @secure_regions: Structure containing the secure regions info > + * @nr_secure_regions: Number of secure regions > * @controller: The hardware controller structure which is > shared among multiple > * independent devices > * @ecc: The ECC controller structure > @@ -1135,6 +1147,8 @@ struct nand_chip { > unsigned int suspended : 1; > int cur_cs; > int read_retries; > + struct nand_secure_region *secure_regions; > + u8 nr_secure_regions; > > /* Externals */ > struct nand_controller *controller; Thanks, Miquèl
Hi Miquel, On Tue, Mar 23, 2021 at 05:57:15PM +0100, Miquel Raynal wrote: > Hi Manivannan, > > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> 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 <manivannan.sadhasivam@linaro.org> > > --- > > 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 = &chip->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 = &chip->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 device_node *dn = nand_get_flash_node(chip); > > + struct property *prop; > > + int length, nr_elem, i, j; > > + > > + prop = of_find_property(dn, "secure-regions", &length); > > + 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() ? > Okay > > + 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. > Oh yeah, I missed it. > > + 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, > > + &chip->secure_regions[i].offset); > > + of_property_read_u64_index(dn, "secure-regions", j + 1, > > + &chip->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 it is done before the BBT parsing logic. > > Here, besides the fact the memory will not be freed from > rawnand_dt_init()'s caller if something goes wrong, we are at a point > where nand_cleanup will not be called. nand_cleanup() will only be > called if the controller driver encounters an error *after* a > successful nand_scan(). > > We could perhaps move this call to nand_scan() which would simply solve > the situation. We don't need it in rawnand_dt_init() as this won't be > rawnand specific anyway... > Okay, will do. Thanks, Mani > > + > > /* > > * If neither the user nor the NAND controller have > > requested a specific > > * ECC engine type, we will default to > > NAND_ECC_ENGINE_TYPE_ON_HOST. @@ -6068,6 +6170,9 @@ void > > nand_cleanup(struct nand_chip *chip) /* Free manufacturer priv data. > > */ nand_manufacturer_cleanup(chip); > > > > + /* Free secure regions data */ > > + kfree(chip->secure_regions); > > + > > /* Free controller specific allocations after chip > > identification */ nand_detach(chip); > > > > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h > > index 6b3240e44310..17ddc900a1dc 100644 > > --- a/include/linux/mtd/rawnand.h > > +++ b/include/linux/mtd/rawnand.h > > @@ -1036,6 +1036,16 @@ struct nand_manufacturer { > > void *priv; > > }; > > > > +/** > > + * struct nand_secure_region - NAND secure region structure > > + * @offset: Offset of the start of the secure region > > + * @size: Size of the secure region > > + */ > > +struct nand_secure_region { > > + u64 offset; > > + u64 size; > > +}; > > + > > /** > > * struct nand_chip - NAND Private Flash Chip Data > > * @base: Inherit from the generic NAND device > > @@ -1086,6 +1096,8 @@ struct nand_manufacturer { > > * NAND Controller drivers should not modify this value, > > but they're > > * allowed to read it. > > * @read_retries: The number of read retry modes supported > > + * @secure_regions: Structure containing the secure regions info > > + * @nr_secure_regions: Number of secure regions > > * @controller: The hardware controller structure which is > > shared among multiple > > * independent devices > > * @ecc: The ECC controller structure > > @@ -1135,6 +1147,8 @@ struct nand_chip { > > unsigned int suspended : 1; > > int cur_cs; > > int read_retries; > > + struct nand_secure_region *secure_regions; > > + u8 nr_secure_regions; > > > > /* Externals */ > > struct nand_controller *controller; > > Thanks, > Miquèl
On Thu, 1 Apr 2021 15:48:12 +0530 Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> 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; > > > */ > >
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 = &chip->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; + int length, nr_elem, i, j; + + prop = of_find_property(dn, "secure-regions", &length); + if (prop) { + nr_elem = length / sizeof(u64); + chip->nr_secure_regions = nr_elem / 2; + + chip->secure_regions = kcalloc(nr_elem, sizeof(*chip->secure_regions), GFP_KERNEL); + 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, + &chip->secure_regions[i].offset); + of_property_read_u64_index(dn, "secure-regions", j + 1, + &chip->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; + /* * If neither the user nor the NAND controller have requested a specific * ECC engine type, we will default to NAND_ECC_ENGINE_TYPE_ON_HOST. @@ -6068,6 +6170,9 @@ void nand_cleanup(struct nand_chip *chip) /* Free manufacturer priv data. */ nand_manufacturer_cleanup(chip); + /* Free secure regions data */ + kfree(chip->secure_regions); + /* Free controller specific allocations after chip identification */ nand_detach(chip); diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h index 6b3240e44310..17ddc900a1dc 100644 --- a/include/linux/mtd/rawnand.h +++ b/include/linux/mtd/rawnand.h @@ -1036,6 +1036,16 @@ struct nand_manufacturer { void *priv; }; +/** + * struct nand_secure_region - NAND secure region structure + * @offset: Offset of the start of the secure region + * @size: Size of the secure region + */ +struct nand_secure_region { + u64 offset; + u64 size; +}; + /** * struct nand_chip - NAND Private Flash Chip Data * @base: Inherit from the generic NAND device @@ -1086,6 +1096,8 @@ struct nand_manufacturer { * NAND Controller drivers should not modify this value, but they're * allowed to read it. * @read_retries: The number of read retry modes supported + * @secure_regions: Structure containing the secure regions info + * @nr_secure_regions: Number of secure regions * @controller: The hardware controller structure which is shared among multiple * independent devices * @ecc: The ECC controller structure @@ -1135,6 +1147,8 @@ struct nand_chip { unsigned int suspended : 1; int cur_cs; int read_retries; + struct nand_secure_region *secure_regions; + u8 nr_secure_regions; /* Externals */ struct nand_controller *controller;
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 <manivannan.sadhasivam@linaro.org> --- drivers/mtd/nand/raw/nand_base.c | 105 +++++++++++++++++++++++++++++++ include/linux/mtd/rawnand.h | 14 +++++ 2 files changed, 119 insertions(+) -- 2.25.1