linux-kernel - Re: [PATCH v2 3/3] mtd: rawnand: qcom: Add support for secure regions in NAND memory

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210225131948.GA28614@work>
Date:   Thu, 25 Feb 2021 18:49:48 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To:     Miquel Raynal <miquel.raynal@...tlin.com>
Cc:     richard@....at, vigneshr@...com, robh+dt@...nel.org,
        linux-arm-msm@...r.kernel.org, linux-mtd@...ts.infradead.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        boris.brezillon@...labora.com, Daniele.Palmas@...it.com,
        bjorn.andersson@...aro.org
Subject: Re: [PATCH v2 3/3] mtd: rawnand: qcom: Add support for secure
 regions in NAND memory

Hi Miquel,

On Thu, Feb 25, 2021 at 08:47:02AM +0100, Miquel Raynal wrote:
> Hi Manivannan,
> 
> Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org> wrote on Thu,
> 25 Feb 2021 09:41:29 +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,
> > "nand-secure-regions". So let's make use of this property and skip
> > access to the secure regions present in a system.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > ---
> 
> [...]
> 
> >  	config_nand_page_write(nandc);
> > @@ -2830,7 +2865,8 @@ static int qcom_nand_host_init_and_register(struct qcom_nand_controller *nandc,
> >  	struct nand_chip *chip = &host->chip;
> >  	struct mtd_info *mtd = nand_to_mtd(chip);
> >  	struct device *dev = nandc->dev;
> > -	int ret;
> > +	struct property *prop;
> > +	int ret, length, nr_elem;
> >  
> >  	ret = of_property_read_u32(dn, "reg", &host->cs);
> >  	if (ret) {
> > @@ -2886,6 +2922,24 @@ static int qcom_nand_host_init_and_register(struct qcom_nand_controller *nandc,
> >  		}
> >  	}
> >  
> > +	/*
> > +	 * 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.
> > +	 */
> > +	prop = of_find_property(dn, "nand-secure-regions", &length);
> 
> I'm not sure the nand- prefix on this property is needed here, but
> whatever.
> 

I was not sure either but added it since most of the other properties
had it. But I can remove it.

> > +	if (prop) {
> > +		nr_elem = length / sizeof(u32);
> > +		host->nr_sec_regions = nr_elem / 2;
> > +
> > +		host->sec_regions = devm_kcalloc(dev, nr_elem, sizeof(u32), GFP_KERNEL);
> > +		if (!host->sec_regions)
> > +			return -ENOMEM;
> > +
> > +		of_property_read_u32_array(dn, "nand-secure-regions", host->sec_regions, nr_elem);
> > +	}
> > +
> 
> I would move this before nand_scan().
> 

Okay, I'll do it.

Thanks,
Mani

> If you don't, you should bail out with a nand_cleanup() upon error.
> 
> >  	ret = mtd_device_parse_register(mtd, probes, NULL, NULL, 0);
> >  	if (ret)
> >  		nand_cleanup(chip);
> 
> 
> Otherwise lgtm.
> 
> Thanks,
> Miquèl