[PATCH v2 1/5] mtd: spi-nor: don't drop sfdp data if optional parsers fail

[T***@microchip.com]

JESD216C states that just the Basic Flash Parameter Table is mandatory.
Already defined (or future) additional parameter headers and tables are
optional.

Don't drop already collected sfdp data in case an optional table
parser fails. In case of failing, each optional parser is responsible
to roll back to the previously known spi_nor data.

Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
Reported-by: Yogesh Gaur <***@nxp.com>
Suggested-by: Boris Brezillon <***@bootlin.com>
Signed-off-by: Tudor Ambarus <***@microchip.com>
---
v2: update Fixes tag to point to correct commit

drivers/mtd/spi-nor/spi-nor.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 4a96ee719e5a..2cdf96013689 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -3130,7 +3130,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
if (err)
goto exit;

- /* Parse other parameter headers. */
+ /* Parse optional parameter tables. */
for (i = 0; i < header.nph; i++) {
param_header = &param_headers[i];

@@ -3143,8 +3143,17 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
break;
}

- if (err)
- goto exit;
+ if (err) {
+ dev_warn(dev, "Failed to parse optional parameter table: %04x\n",
+ SFDP_PARAM_HEADER_ID(param_header));
+ /*
+ * Let's not drop all information we extracted so far
+ * if optional table parsers fail. In case of failing,
+ * each optional parser is responsible to roll back to
+ * the previously known spi_nor data.
+ */
+ err = 0;
+ }
}

exit:

[T***@microchip.com]

Iterate over smpt array using its starting address and length
instead of the blind iterations that used data found in the array.

This prevents possible memory accesses outside of the smpt array
boundaries in case software, or manufacturers, misrepresent smpt
array fields.

Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
Suggested-by: Boris Brezillon <***@bootlin.com>
Signed-off-by: Tudor Ambarus <***@microchip.com>
---
v2: add Fixes tag, add a blank line

drivers/mtd/spi-nor/spi-nor.c | 40 ++++++++++++++++++++++++++++++----------
1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 2cdf96013689..98e433e8e4c2 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2860,12 +2860,15 @@ static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
* spi_nor_get_map_in_use() - get the configuration map in use
* @nor: pointer to a 'struct spi_nor'
* @smpt: pointer to the sector map parameter table
+ * @smpt_len: sector map parameter table length
*/
-static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt)
+static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
+ u8 smpt_len)
{
const u32 *ret = NULL;
- u32 i, addr;
+ u32 addr;
int err;
+ u8 i;
u8 addr_width, read_opcode, read_dummy;
u8 read_data_mask, data_byte, map_id;

@@ -2874,9 +2877,11 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt)
read_opcode = nor->read_opcode;

map_id = 0;
- i = 0;
/* Determine if there are any optional Detection Command Descriptors */
- while (!(smpt[i] & SMPT_DESC_TYPE_MAP)) {
+ for (i = 0; i < smpt_len; i += 2) {
+ if (smpt[i] & SMPT_DESC_TYPE_MAP)
+ break;
+
read_data_mask = SMPT_CMD_READ_DATA(smpt[i]);
nor->addr_width = spi_nor_smpt_addr_width(nor, smpt[i]);
nor->read_dummy = spi_nor_smpt_read_dummy(nor, smpt[i]);
@@ -2892,18 +2897,33 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt)
* Configuration that is currently in use.
*/
map_id = map_id << 1 | !!(data_byte & read_data_mask);
- i = i + 2;
}

- /* Find the matching configuration map */
- while (SMPT_MAP_ID(smpt[i]) != map_id) {
+ /*
+ * If command descriptors are provided, they always precede map
+ * descriptors in the table. There is no need to start the iteration
+ * over smpt array all over again.
+ *
+ * Find the matching configuration map.
+ */
+ while (i < smpt_len) {
+ if (SMPT_MAP_ID(smpt[i]) == map_id) {
+ ret = smpt + i;
+ break;
+ }
+
+ /*
+ * If there are no more configuration map descriptors and no
+ * configuration ID matched the configuration identifier, the
+ * sector address map is unknown.
+ */
if (smpt[i] & SMPT_DESC_END)
- goto out;
+ break;
+
/* increment the table index to the next map */
i += SMPT_MAP_REGION_COUNT(smpt[i]) + 1;
}

- ret = smpt + i;
/* fall through */
out:
nor->addr_width = addr_width;
@@ -3025,7 +3045,7 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,
for (i = 0; i < smpt_header->length; i++)
smpt[i] = le32_to_cpu(smpt[i]);

- sector_map = spi_nor_get_map_in_use(nor, smpt);
+ sector_map = spi_nor_get_map_in_use(nor, smpt, smpt_header->length);
if (!sector_map) {
ret = -EINVAL;
goto out;

[T***@microchip.com]

spi_nor_read_raw() calls nor->read() which might be implemented
by the m25p80 driver. m25p80 uses the spi-mem layer which requires
DMA-able in/out buffers. Pass kmalloc'ed dma buffer to spi_nor_read_raw().

Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
Signed-off-by: Tudor Ambarus <***@microchip.com>
---
v2: drop GFP_DMA, rename buf, add comment

drivers/mtd/spi-nor/spi-nor.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 04a1c5b825e6..458ca8321999 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2161,7 +2161,7 @@ spi_nor_set_pp_settings(struct spi_nor_pp_command *pp,
* @nor: pointer to a 'struct spi_nor'
* @addr: offset in the serial flash memory
* @len: number of bytes to read
- * @buf: buffer where the data is copied into
+ * @buf: buffer where the data is copied into (dma-safe memory)
*
* Return: 0 on success, -errno otherwise.
*/
@@ -2868,11 +2868,17 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
u8 smpt_len)
{
const u32 *ret;
+ u8 *buf;
u32 addr;
int err;
u8 i;
u8 addr_width, read_opcode, read_dummy;
- u8 read_data_mask, data_byte, map_id;
+ u8 read_data_mask, map_id;
+
+ /* Use a kmalloc'ed bounce buffer to guarantee it is DMA-able. */
+ buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+ if (!buf)
+ return ERR_PTR(-ENOMEM);

addr_width = nor->addr_width;
read_dummy = nor->read_dummy;
@@ -2890,7 +2896,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
addr = smpt[i + 1];

- err = spi_nor_read_raw(nor, addr, 1, &data_byte);
+ err = spi_nor_read_raw(nor, addr, 1, buf);
if (err) {
ret = ERR_PTR(err);
goto out;
@@ -2900,7 +2906,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
* Build an index value that is used to select the Sector Map
* Configuration that is currently in use.
*/
- map_id = map_id << 1 | !!(data_byte & read_data_mask);
+ map_id = map_id << 1 | !!(*buf & read_data_mask);
}

/*
@@ -2931,6 +2937,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,

/* fall through */
out:
+ kfree(buf);
nor->addr_width = addr_width;
nor->read_dummy = read_dummy;
nor->read_opcode = read_opcode;

[T***@microchip.com]

The entire smpt array is initialized with data read from sfdp,
there is no need to init it with zeroes before.

Signed-off-by: Tudor Ambarus <***@microchip.com>
---
drivers/mtd/spi-nor/spi-nor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 458ca8321999..f9c657867e5a 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -3044,7 +3044,7 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,

/* Read the Sector Map Parameter Table. */
len = smpt_header->length * sizeof(*smpt);
- smpt = kzalloc(len, GFP_KERNEL);
+ smpt = kmalloc(len, GFP_KERNEL);
if (!smpt)
return -ENOMEM;

[T***@microchip.com]

Don't overwrite the errno from spi_nor_read_raw().

Signed-off-by: Tudor Ambarus <***@microchip.com>
---
drivers/mtd/spi-nor/spi-nor.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 98e433e8e4c2..04a1c5b825e6 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2861,11 +2861,13 @@ static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
* @nor: pointer to a 'struct spi_nor'
* @smpt: pointer to the sector map parameter table
* @smpt_len: sector map parameter table length
+ *
+ * Return: pointer to the map in use, ERR_PTR(-errno) otherwise.
*/
static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
u8 smpt_len)
{
- const u32 *ret = NULL;
+ const u32 *ret;
u32 addr;
int err;
u8 i;
@@ -2889,8 +2891,10 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
addr = smpt[i + 1];

err = spi_nor_read_raw(nor, addr, 1, &data_byte);
- if (err)
+ if (err) {
+ ret = ERR_PTR(err);
goto out;
+ }

/*
* Build an index value that is used to select the Sector Map
@@ -2906,6 +2910,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
*
* Find the matching configuration map.
*/
+ ret = ERR_PTR(-EINVAL);
while (i < smpt_len) {
if (SMPT_MAP_ID(smpt[i]) == map_id) {
ret = smpt + i;
@@ -3046,8 +3051,8 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,
smpt[i] = le32_to_cpu(smpt[i]);

sector_map = spi_nor_get_map_in_use(nor, smpt, smpt_header->length);
- if (!sector_map) {
- ret = -EINVAL;
+ if (IS_ERR(sector_map)) {
+ ret = PTR_ERR(sector_map);
goto out;
}

[Yogesh Narayan Gaur]

-----Original Message-----
Sent: Friday, November 9, 2018 10:27 PM
Subject: [PATCH v2 1/5] mtd: spi-nor: don't drop sfdp data if optional parsers fail
JESD216C states that just the Basic Flash Parameter Table is mandatory.
Already defined (or future) additional parameter headers and tables are optional.
Don't drop already collected sfdp data in case an optional table parser fails. In
case of failing, each optional parser is responsible to roll back to the previously
known spi_nor data.
Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
---
v2: update Fixes tag to point to correct commit
drivers/mtd/spi-nor/spi-nor.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index
4a96ee719e5a..2cdf96013689 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -3130,7 +3130,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
if (err)
goto exit;
- /* Parse other parameter headers. */
+ /* Parse optional parameter tables. */
for (i = 0; i < header.nph; i++) {
param_header = &param_headers[i];
@@ -3143,8 +3143,17 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
break;
}
- if (err)
- goto exit;
+ if (err) {
+ dev_warn(dev, "Failed to parse optional parameter
table: %04x\n",
+ SFDP_PARAM_HEADER_ID(param_header));
+ /*
+ * Let's not drop all information we extracted so far
+ * if optional table parsers fail. In case of failing,
+ * each optional parser is responsible to roll back to
+ * the previously known spi_nor data.
+ */
+ err = 0;
+ }
}
--
2.9.4

[Boris Brezillon]

Post by T***@microchip.com
Don't overwrite the errno from spi_nor_read_raw().

Applied to http://git.infradead.org/linux-mtd.git mtd/fixes, thanks.

Boris

[Boris Brezillon]

Post by T***@microchip.com
spi_nor_read_raw() calls nor->read() which might be implemented
by the m25p80 driver. m25p80 uses the spi-mem layer which requires
DMA-able in/out buffers. Pass kmalloc'ed dma buffer to spi_nor_read_raw().
Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")

Applied to http://git.infradead.org/linux-mtd.git mtd/fixes, thanks.

Boris

[Boris Brezillon]

Post by T***@microchip.com
Iterate over smpt array using its starting address and length
instead of the blind iterations that used data found in the array.
This prevents possible memory accesses outside of the smpt array
boundaries in case software, or manufacturers, misrepresent smpt
array fields.
Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")

Applied to http://git.infradead.org/linux-mtd.git mtd/fixes, thanks.

Boris

[Boris Brezillon]

Post by T***@microchip.com
JESD216C states that just the Basic Flash Parameter Table is mandatory.
Already defined (or future) additional parameter headers and tables are
optional.
Don't drop already collected sfdp data in case an optional table
parser fails. In case of failing, each optional parser is responsible
to roll back to the previously known spi_nor data.
Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")

Applied to http://git.infradead.org/linux-mtd.git mtd/fixes, thanks.

Boris

[Boris Brezillon]

Post by T***@microchip.com
The entire smpt array is initialized with data read from sfdp,
there is no need to init it with zeroes before.

Applied to http://git.infradead.org/linux-mtd.git spi-nor/next, thanks.

Boris