On Fri, Mar 19, 2021 at 05:52:58PM +0100, Boris Brezillon wrote:
> On Fri, 19 Mar 2021 20:30:10 +0530
> Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> wrote:
>
> > 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@xxxxxxxxxx>
> > ---
> > drivers/mtd/nand/raw/nand_base.c | 111 +++++++++++++++++++++++++++++++
> > include/linux/mtd/rawnand.h | 4 ++
> > 2 files changed, 115 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index c33fa1b1847f..479a79e682cd 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -278,11 +278,47 @@ 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, j;
> > +
> > + /* Skip touching the secure regions if present */
> > + for (i = 0, j = 0; i < chip->nr_secure_regions; i++, j += 2) {
> > + /* First compare the start offset */
> > + if (offset >= chip->secure_regions[j] &&
> > + (offset < chip->secure_regions[j] + chip->secure_regions[j + 1]))
> > + return -EIO;
> > + /* ...then offset + size */
> > + else if (offset < chip->secure_regions[i] &&
> > + (offset + size) >= chip->secure_regions[i])
> > + return -EIO;
>
> How about:
>
> const struct nand_secure_region *region = &chip->secure_regions[i];
>
> if (offset + size <= region->offset ||
> offset >= region->offset + region->size)
> continue;
>
> return -EIO;
>
I guess you mean this:
/* 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;
> > + }
> > +
> > + return 0;
> > +}
> > +
[...]
> > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > index 6b3240e44310..d385c4fe8b0f 100644
> > --- a/include/linux/mtd/rawnand.h
> > +++ b/include/linux/mtd/rawnand.h
> > @@ -1086,6 +1086,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: Array representing the secure regions
> > + * @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 +1137,8 @@ struct nand_chip {
> > unsigned int suspended : 1;
> > int cur_cs;
> > int read_retries;
> > + u64 *secure_regions;
>
>
> Can you please define the following struct:
>
> struct nand_secure_region {
> u64 offset;
> u64 size;
> };
>
> instead of having an array of u64 where even entries encode the offset
> and odd ones the size.
>
Hmm, I think you implicitly said this in your previous review as well and I
somehow lost it. Will incorporate. So we'll have something like this in
of_get_nand_secure_regions():
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);
}
Thanks,
Mani
