[PATCH] eeprom: at25: Rework buggy read splitting

[PATCH] eeprom: at25: Rework buggy read splitting

[Geert Uytterhoeven]

The recent change to split reads into chunks has several problems:
  1. If an SPI controller has no transfer size limit, max_chunk is
     SIZE_MAX, and num_msgs becomes zero, causing no data to be read
     into the buffer, and exposing the original contents of the buffer
     to userspace,
  2. If the requested read size is not a multiple of the maximum
     transfer size, the last transfer reads too much data, overflowing
     the buffer,
  3. The loop logic differs from the write case.

Fix the above by:
  1. Keeping track of the number of bytes that are still to be
     transferred, instead of precalculating the number of messages and
     keeping track of the number of bytes tranfered,
  2. Calculating the transfer size of each individual message, taking
     into account the number of bytes left,
  3. Switching from a "while"-loop to a "do-while"-loop, and renaming
     "msg_count" to "segment".

While at it, drop the superfluous cast from "unsigned int" to "unsigned
int", also from at25_ee_write(), where it was probably copied from.

Fixes: 0a35780c755ccec0 ("eeprom: at25: Split reads into chunks and cap write size")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Tested on Ebisu-4D with 25LC040 EEPROM.
---
 drivers/misc/eeprom/at25.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index c9c56fd194c1301f..bdffc6543f6f8b7f 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -80,10 +80,9 @@ static int at25_ee_read(void *priv, unsigned int offset,
 	struct at25_data *at25 = priv;
 	char *buf = val;
 	size_t max_chunk = spi_max_transfer_size(at25->spi);
-	size_t num_msgs = DIV_ROUND_UP(count, max_chunk);
-	size_t nr_bytes = 0;
-	unsigned int msg_offset;
-	size_t msg_count;
+	unsigned int msg_offset = offset;
+	size_t bytes_left = count;
+	size_t segment;
 	u8			*cp;
 	ssize_t			status;
 	struct spi_transfer	t[2];
@@ -97,9 +96,8 @@ static int at25_ee_read(void *priv, unsigned int offset,
 	if (unlikely(!count))
 		return -EINVAL;
 
-	msg_offset = (unsigned int)offset;
-	msg_count = min(count, max_chunk);
-	while (num_msgs) {
+	do {
+		segment = min(bytes_left, max_chunk);
 		cp = at25->command;
 
 		instr = AT25_READ;
@@ -131,8 +129,8 @@ static int at25_ee_read(void *priv, unsigned int offset,
 		t[0].len = at25->addrlen + 1;
 		spi_message_add_tail(&t[0], &m);
 
-		t[1].rx_buf = buf + nr_bytes;
-		t[1].len = msg_count;
+		t[1].rx_buf = buf;
+		t[1].len = segment;
 		spi_message_add_tail(&t[1], &m);
 
 		status = spi_sync(at25->spi, &m);
@@ -142,10 +140,10 @@ static int at25_ee_read(void *priv, unsigned int offset,
 		if (status)
 			return status;
 
-		--num_msgs;
-		msg_offset += msg_count;
-		nr_bytes += msg_count;
-	}
+		msg_offset += segment;
+		buf += segment;
+		bytes_left -= segment;
+	} while (bytes_left > 0);
 
 	dev_dbg(&at25->spi->dev, "read %zu bytes at %d\n",
 		count, offset);
@@ -229,7 +227,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
 	do {
 		unsigned long	timeout, retries;
 		unsigned	segment;
-		unsigned	offset = (unsigned) off;
+		unsigned	offset = off;
 		u8		*cp = bounce;
 		int		sr;
 		u8		instr;
-- 
2.25.1

Re: [PATCH] eeprom: at25: Rework buggy read splitting

[Joel Stanley]

On Tue, 21 Jun 2022 at 13:22, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> The recent change to split reads into chunks has several problems:
>   1. If an SPI controller has no transfer size limit, max_chunk is
>      SIZE_MAX, and num_msgs becomes zero, causing no data to be read
>      into the buffer, and exposing the original contents of the buffer
>      to userspace,
>   2. If the requested read size is not a multiple of the maximum
>      transfer size, the last transfer reads too much data, overflowing
>      the buffer,
>   3. The loop logic differs from the write case.
>
> Fix the above by:
>   1. Keeping track of the number of bytes that are still to be
>      transferred, instead of precalculating the number of messages and
>      keeping track of the number of bytes tranfered,

sp: transferred

>   2. Calculating the transfer size of each individual message, taking
>      into account the number of bytes left,
>   3. Switching from a "while"-loop to a "do-while"-loop, and renaming
>      "msg_count" to "segment".
>
> While at it, drop the superfluous cast from "unsigned int" to "unsigned
> int", also from at25_ee_write(), where it was probably copied from.
>
> Fixes: 0a35780c755ccec0 ("eeprom: at25: Split reads into chunks and cap write size")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks Geert for the patch. This is particularly important as I
noticed the "fix" has been backported to stable trees.

I was surprised that patch went in as-is; I thought it needed some
discussion before merging. I wasn't sure if it was the correct fix at
all; I wondered if it should have been fixed at the spi layer. Do you
have an opinion there?

Eddie, can you jump in for some testing and a review of this one?

Cheers,

Joel

> ---
> Tested on Ebisu-4D with 25LC040 EEPROM.
> ---
>  drivers/misc/eeprom/at25.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
> index c9c56fd194c1301f..bdffc6543f6f8b7f 100644
> --- a/drivers/misc/eeprom/at25.c
> +++ b/drivers/misc/eeprom/at25.c
> @@ -80,10 +80,9 @@ static int at25_ee_read(void *priv, unsigned int offset,
>         struct at25_data *at25 = priv;
>         char *buf = val;
>         size_t max_chunk = spi_max_transfer_size(at25->spi);
> -       size_t num_msgs = DIV_ROUND_UP(count, max_chunk);
> -       size_t nr_bytes = 0;
> -       unsigned int msg_offset;
> -       size_t msg_count;
> +       unsigned int msg_offset = offset;
> +       size_t bytes_left = count;
> +       size_t segment;
>         u8                      *cp;
>         ssize_t                 status;
>         struct spi_transfer     t[2];
> @@ -97,9 +96,8 @@ static int at25_ee_read(void *priv, unsigned int offset,
>         if (unlikely(!count))
>                 return -EINVAL;
>
> -       msg_offset = (unsigned int)offset;
> -       msg_count = min(count, max_chunk);
> -       while (num_msgs) {
> +       do {
> +               segment = min(bytes_left, max_chunk);
>                 cp = at25->command;
>
>                 instr = AT25_READ;
> @@ -131,8 +129,8 @@ static int at25_ee_read(void *priv, unsigned int offset,
>                 t[0].len = at25->addrlen + 1;
>                 spi_message_add_tail(&t[0], &m);
>
> -               t[1].rx_buf = buf + nr_bytes;
> -               t[1].len = msg_count;
> +               t[1].rx_buf = buf;
> +               t[1].len = segment;
>                 spi_message_add_tail(&t[1], &m);
>
>                 status = spi_sync(at25->spi, &m);
> @@ -142,10 +140,10 @@ static int at25_ee_read(void *priv, unsigned int offset,
>                 if (status)
>                         return status;
>
> -               --num_msgs;
> -               msg_offset += msg_count;
> -               nr_bytes += msg_count;
> -       }
> +               msg_offset += segment;
> +               buf += segment;
> +               bytes_left -= segment;
> +       } while (bytes_left > 0);
>
>         dev_dbg(&at25->spi->dev, "read %zu bytes at %d\n",
>                 count, offset);
> @@ -229,7 +227,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
>         do {
>                 unsigned long   timeout, retries;
>                 unsigned        segment;
> -               unsigned        offset = (unsigned) off;
> +               unsigned        offset = off;
>                 u8              *cp = bounce;
>                 int             sr;
>                 u8              instr;
> --
> 2.25.1
>