From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: richard@nod.at, vigneshr@ti.com, robh+dt@kernel.org,
linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
boris.brezillon@collabora.com, Daniele.Palmas@telit.com,
bjorn.andersson@linaro.org
Subject: Re: [PATCH v8 3/3] mtd: rawnand: Add support for secure regions in NAND memory
Date: Thu, 1 Apr 2021 15:48:12 +0530 [thread overview]
Message-ID: <20210401101812.GE14052@work> (raw)
In-Reply-To: <20210323175715.38b4740a@xps13>
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
next prev parent reply other threads:[~2021-04-01 10:19 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-23 7:39 [PATCH v8 0/3] Add support for secure regions in NAND Manivannan Sadhasivam
2021-03-23 7:39 ` [PATCH v8 1/3] dt-bindings: mtd: Convert Qcom NANDc binding to YAML Manivannan Sadhasivam
2021-03-23 7:39 ` [PATCH v8 2/3] dt-bindings: mtd: Add a property to declare secure regions in NAND chips Manivannan Sadhasivam
2021-03-23 22:29 ` Rob Herring
2021-03-23 7:39 ` [PATCH v8 3/3] mtd: rawnand: Add support for secure regions in NAND memory Manivannan Sadhasivam
2021-03-23 16:57 ` Miquel Raynal
2021-04-01 10:18 ` Manivannan Sadhasivam [this message]
2021-04-01 15:50 ` Boris Brezillon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210401101812.GE14052@work \
--to=manivannan.sadhasivam@linaro.org \
--cc=Daniele.Palmas@telit.com \
--cc=bjorn.andersson@linaro.org \
--cc=boris.brezillon@collabora.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=miquel.raynal@bootlin.com \
--cc=richard@nod.at \
--cc=robh+dt@kernel.org \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).