Message ID | 20170917095750.14059-1-richard@nod.at |
---|---|
State | Rejected |
Delegated to: | Cyrille Pitchen |
Headers | show |
Series | mtd: spi-nor: Check for spi_nor_hwcaps_read2cmd() return value | expand |
On Sun, 17 Sep 2017 11:57:50 +0200 Richard Weinberger <richard@nod.at> 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 <richard@nod.at> > --- > 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 = ¶ms->reads[cmd]; > half = bfpt.dwords[rd->settings_dword] >> rd->settings_shift; > spi_nor_set_read_settings_from_bfpt(read, half, rd->proto);
Am Montag, 18. September 2017, 11:39:45 CEST schrieb Boris Brezillon: > On Sun, 17 Sep 2017 11:57:50 +0200 > > Richard Weinberger <richard@nod.at> 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 <richard@nod.at> > > --- > > > > 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
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 <richard@nod.at> > --- > 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 = ¶ms->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 = ¶ms->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
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 = ¶ms->reads[cmd]; half = bfpt.dwords[rd->settings_dword] >> rd->settings_shift; spi_nor_set_read_settings_from_bfpt(read, half, rd->proto);
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 <richard@nod.at> --- drivers/mtd/spi-nor/spi-nor.c | 3 +++ 1 file changed, 3 insertions(+)