[PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()

[PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()

[Dan Carpenter]

Check for whether of_property_count_elems_of_size() returns a negative
error code.

Fixes: 13b89768275d ("mtd: rawnand: Add support for secure regions in NAND memory")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/mtd/nand/raw/nand_base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 57a583149cc0..cbba46432e39 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5231,8 +5231,8 @@ static int of_get_nand_secure_regions(struct nand_chip *chip)
 	int nr_elem, i, j;
 
 	nr_elem = of_property_count_elems_of_size(dn, "secure-regions", sizeof(u64));
-	if (!nr_elem)
-		return 0;
+	if (nr_elem <= 0)
+		return nr_elem;
 
 	chip->nr_secure_regions = nr_elem / 2;
 	chip->secure_regions = kcalloc(chip->nr_secure_regions, sizeof(*chip->secure_regions),
-- 
2.30.2

Re: [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()

[Manivannan Sadhasivam]

On Thu, Jun 17, 2021 at 04:37:25PM +0300, Dan Carpenter wrote:
> Check for whether of_property_count_elems_of_size() returns a negative
> error code.
> 
> Fixes: 13b89768275d ("mtd: rawnand: Add support for secure regions in NAND memory")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/mtd/nand/raw/nand_base.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 57a583149cc0..cbba46432e39 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -5231,8 +5231,8 @@ static int of_get_nand_secure_regions(struct nand_chip *chip)
>  	int nr_elem, i, j;
>  
>  	nr_elem = of_property_count_elems_of_size(dn, "secure-regions", sizeof(u64));
> -	if (!nr_elem)
> -		return 0;
> +	if (nr_elem <= 0)
> +		return nr_elem;
>  
>  	chip->nr_secure_regions = nr_elem / 2;
>  	chip->secure_regions = kcalloc(chip->nr_secure_regions, sizeof(*chip->secure_regions),
> -- 
> 2.30.2
> 

Re: [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()

[Miquel Raynal]

On Thu, 2021-06-17 at 13:37:25 UTC, Dan Carpenter wrote:
> Check for whether of_property_count_elems_of_size() returns a negative
> error code.
> 
> Fixes: 13b89768275d ("mtd: rawnand: Add support for secure regions in NAND memory")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks.

Miquel

Re: [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()

[Martin Kaiser]

Hi all,

Thus wrote Miquel Raynal (miquel.raynal@bootlin.com):

> On Thu, 2021-06-17 at 13:37:25 UTC, Dan Carpenter wrote:
> > Check for whether of_property_count_elems_of_size() returns a negative
> > error code.

> > Fixes: 13b89768275d ("mtd: rawnand: Add support for secure regions in NAND memory")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks.

I'm running linux-next on an imx25 system with the following flash chip

[    1.997539] nand: device found, Manufacturer ID: 0x98, Chip ID: 0xaa
[    2.004134] nand: Toshiba NAND 256MiB 1,8V 8-bit
[    2.008917] nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 128

The system is using the drivers/mtd/nand/raw/mxc_nand.c driver.

Since this commit appeared in linux-next, mxc_nand's probe function fails
with -EINVAL, taking this path

mxcnd_probe
   nand_scan
      nand_scan_with_ids
         nand_scan_tail
            of_get_nand_secure_regions

nr_elem = of_property_count_elems_of_size(dn, "secure-regions", sizeof(u64));
returns -EINVAL as there's no secure-regions property in my device tree.

We should certainly handle negative error codes before we calculate
chip->nr_secure_regions = nr_elem / 2
but a missing secure-regions property is a valid case and should not make
the probe fail.

If the property exists, but the device-tree entry is incorrect
and of_property_count_elems_of_size returns -ENODATA, we might print a
warning and ignore the entry.

What do you think?

Thanks,

   Martin

Re: [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()

[Manivannan Sadhasivam]

On Sat, Jul 24, 2021 at 04:27:30PM +0200, Martin Kaiser wrote:
> Hi all,
> 
> Thus wrote Miquel Raynal (miquel.raynal@bootlin.com):
> 
> > On Thu, 2021-06-17 at 13:37:25 UTC, Dan Carpenter wrote:
> > > Check for whether of_property_count_elems_of_size() returns a negative
> > > error code.
> 
> > > Fixes: 13b89768275d ("mtd: rawnand: Add support for secure regions in NAND memory")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> 
> > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks.
> 
> I'm running linux-next on an imx25 system with the following flash chip
> 
> [    1.997539] nand: device found, Manufacturer ID: 0x98, Chip ID: 0xaa
> [    2.004134] nand: Toshiba NAND 256MiB 1,8V 8-bit
> [    2.008917] nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 128
> 
> The system is using the drivers/mtd/nand/raw/mxc_nand.c driver.
> 
> Since this commit appeared in linux-next, mxc_nand's probe function fails
> with -EINVAL, taking this path
> 
> mxcnd_probe
>    nand_scan
>       nand_scan_with_ids
>          nand_scan_tail
>             of_get_nand_secure_regions
> 
> nr_elem = of_property_count_elems_of_size(dn, "secure-regions", sizeof(u64));
> returns -EINVAL as there's no secure-regions property in my device tree.
> 

Doh! Sorry for missing this.

> We should certainly handle negative error codes before we calculate
> chip->nr_secure_regions = nr_elem / 2
> but a missing secure-regions property is a valid case and should not make
> the probe fail.
> 

Absolutely!

> If the property exists, but the device-tree entry is incorrect
> and of_property_count_elems_of_size returns -ENODATA, we might print a
> warning and ignore the entry.
> 

Hmm, I think it is best to error out in this case as the user has got DT wrong.

> What do you think?
> 

Since of_property_count_elems_of_size() returns -EINVAL if the length is not
a multiple of sizeof(u64), we can't just ignore -EINVAL.

So I think we can just check for the existence of the property before invoking
of_get_nand_secure_regions(). Miquel, what do you think?

Thanks,
Mani

> Thanks,
> 
>    Martin

Re: [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()

[Miquel Raynal]

Hi Mani,

Manivannan Sadhasivam <mani@kernel.org> wrote on Sat, 24 Jul 2021
21:33:08 +0530:

> On Sat, Jul 24, 2021 at 04:27:30PM +0200, Martin Kaiser wrote:
> > Hi all,
> > 
> > Thus wrote Miquel Raynal (miquel.raynal@bootlin.com):
> >   
> > > On Thu, 2021-06-17 at 13:37:25 UTC, Dan Carpenter wrote:  
> > > > Check for whether of_property_count_elems_of_size() returns a negative
> > > > error code.  
> >   
> > > > Fixes: 13b89768275d ("mtd: rawnand: Add support for secure regions in NAND memory")
> > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>  
> >   
> > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks.  
> > 
> > I'm running linux-next on an imx25 system with the following flash chip
> > 
> > [    1.997539] nand: device found, Manufacturer ID: 0x98, Chip ID: 0xaa
> > [    2.004134] nand: Toshiba NAND 256MiB 1,8V 8-bit
> > [    2.008917] nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 128
> > 
> > The system is using the drivers/mtd/nand/raw/mxc_nand.c driver.
> > 
> > Since this commit appeared in linux-next, mxc_nand's probe function fails
> > with -EINVAL, taking this path
> > 
> > mxcnd_probe
> >    nand_scan
> >       nand_scan_with_ids
> >          nand_scan_tail
> >             of_get_nand_secure_regions
> > 
> > nr_elem = of_property_count_elems_of_size(dn, "secure-regions", sizeof(u64));
> > returns -EINVAL as there's no secure-regions property in my device tree.
> >   
> 
> Doh! Sorry for missing this.
> 
> > We should certainly handle negative error codes before we calculate
> > chip->nr_secure_regions = nr_elem / 2
> > but a missing secure-regions property is a valid case and should not make
> > the probe fail.
> >   
> 
> Absolutely!
> 
> > If the property exists, but the device-tree entry is incorrect
> > and of_property_count_elems_of_size returns -ENODATA, we might print a
> > warning and ignore the entry.
> >   
> 
> Hmm, I think it is best to error out in this case as the user has got DT wrong.
> 
> > What do you think?
> >   
> 
> Since of_property_count_elems_of_size() returns -EINVAL if the length is not
> a multiple of sizeof(u64), we can't just ignore -EINVAL.
> 
> So I think we can just check for the existence of the property before invoking
> of_get_nand_secure_regions(). Miquel, what do you think?

Yes please add this check and we should be good.

Thanks,
Miquèl