2017-09-17 09:57:41

by Richard Weinberger

Subject: [PATCH] mtd: spi-nor: Check for spi_nor_hwcaps_read2cmd() return value

The function can return a negativ value in case of errors,
don't use it blindly as array index.

Detected by CoverityScan CID#1418067 ("Memory - illegal accesses")
Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable
Parameters (SFDP) tables")
Signed-off-by: Richard Weinberger <[email protected]>
---
drivers/mtd/spi-nor/spi-nor.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index cf1d4a15e10a..d71765739a93 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2145,6 +2145,9 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,

params->hwcaps.mask |= rd->hwcaps;
cmd = spi_nor_hwcaps_read2cmd(rd->hwcaps);
+ if (cmd < 0)
+ return -EINVAL;
+
read = &params->reads[cmd];
half = bfpt.dwords[rd->settings_dword] >> rd->settings_shift;
spi_nor_set_read_settings_from_bfpt(read, half, rd->proto);
--
2.13.5

2017-09-18 09:39:58

by Boris Brezillon

Subject: Re: [PATCH] mtd: spi-nor: Check for spi_nor_hwcaps_read2cmd() return value

On Sun, 17 Sep 2017 11:57:50 +0200
Richard Weinberger <[email protected]> wrote:

> The function can return a negativ value in case of errors,
> don't use it blindly as array index.
>
> Detected by CoverityScan CID#1418067 ("Memory - illegal accesses")
> Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable
> Parameters (SFDP) tables")

Hm, not sure but I think "Fixes:" should not be wrapped.

> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> drivers/mtd/spi-nor/spi-nor.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index cf1d4a15e10a..d71765739a93 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2145,6 +2145,9 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>
> params->hwcaps.mask |= rd->hwcaps;
> cmd = spi_nor_hwcaps_read2cmd(rd->hwcaps);
> + if (cmd < 0)
> + return -EINVAL;

Why not returning cmd directly?

> +
> read = &params->reads[cmd];
> half = bfpt.dwords[rd->settings_dword] >> rd->settings_shift;
> spi_nor_set_read_settings_from_bfpt(read, half, rd->proto);

2017-09-18 13:39:08

by Richard Weinberger

Subject: Re: [PATCH] mtd: spi-nor: Check for spi_nor_hwcaps_read2cmd() return value

Am Montag, 18. September 2017, 11:39:45 CEST schrieb Boris Brezillon:
> On Sun, 17 Sep 2017 11:57:50 +0200
>
> Richard Weinberger <[email protected]> wrote:
> > The function can return a negativ value in case of errors,
> > don't use it blindly as array index.
> >
> > Detected by CoverityScan CID#1418067 ("Memory - illegal accesses")
> > Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable
> > Parameters (SFDP) tables")
>
> Hm, not sure but I think "Fixes:" should not be wrapped.

Hmm, vi tried to be smart. ;-\

>
> > Signed-off-by: Richard Weinberger <[email protected]>
> > ---
> >
> > drivers/mtd/spi-nor/spi-nor.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index cf1d4a15e10a..d71765739a93 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -2145,6 +2145,9 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
> >
> > params->hwcaps.mask |= rd->hwcaps;
> > cmd = spi_nor_hwcaps_read2cmd(rd->hwcaps);
> >
> > + if (cmd < 0)
> > + return -EINVAL;
>
> Why not returning cmd directly?

I thought about that too but the only other user of that function also returns
-EINVAL upon error.
Maybe Cyrille can give more input whether we should propagate
spi_nor_hwcaps_read2cmd()'s return values or not.

Thanks,
//richard

2017-09-19 17:33:21

by Cyrille Pitchen

Subject: Re: [PATCH] mtd: spi-nor: Check for spi_nor_hwcaps_read2cmd() return value

Hi Richard,

Le 17/09/2017 à 11:57, Richard Weinberger a écrit :
> The function can return a negativ value in case of errors,
> don't use it blindly as array index.
>
> Detected by CoverityScan CID#1418067 ("Memory - illegal accesses")
> Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable
> Parameters (SFDP) tables")
> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> drivers/mtd/spi-nor/spi-nor.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index cf1d4a15e10a..d71765739a93 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2145,6 +2145,9 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>
> params->hwcaps.mask |= rd->hwcaps;
> cmd = spi_nor_hwcaps_read2cmd(rd->hwcaps);
> + if (cmd < 0)
> + return -EINVAL;
> +
> read = &params->reads[cmd];
> half = bfpt.dwords[rd->settings_dword] >> rd->settings_shift;
> spi_nor_set_read_settings_from_bfpt(read, half, rd->proto);
>

This one is a false positive:

static const struct sfdp_bfpt_read sfdp_bfpt_reads[] = {
/* Fast Read 1-1-2 */
{
SNOR_HWCAPS_READ_1_1_2,
BFPT_DWORD(1), BIT(16), /* Supported bit */
BFPT_DWORD(4), 0, /* Settings */
SNOR_PROTO_1_1_2,
},
[...]
};

static int spi_nor_parse_bfpt(...)
{
[...]
for (i = 0; i < ARRAY_SIZE(sfdp_bfpt_reads); i++) {
const struct sfdp_bfpt_read *rd = &sfdp_bfpt_reads[i];
[...]
cmd = spi_nor_hwcaps_read2cmd(rd->hwcaps);
read = &params->reads[cmd];
[...]
}
[...]
}

sfdp_bfpt_reads[] is a static const array where each element has a valid
.hwcaps value so spi_nor_hwcaps_read2cmd() can actually never return an
out of range 'cmd' result here.


However in spi_nor_select_read(), spi_nor_hwcaps_read2cmd() is called
too but this time its input argument now comes from the spi_nor_scan()
caller so in this later case the code already checks the output of
spi_nor_hwcaps_read2cmd() as it could be -EINVAL.


Do you want us to take this patch anyway to please the code checker ?
I don't think it is mandatory as a fix for 4.14-rc2.

Best regards,

Cyrille