[PATCH 1/2] mmc-utils: introduce optional --verify argument for 'extcsd write'

[PATCH 1/2] mmc-utils: introduce optional --verify argument for 'extcsd write'

[Enrico Jorns]

Registers can be write-once but ioctl does not necessarily return with
an error. Thus it is a good idea to allow verifying the data written.

Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
---
 mmc.c      |  7 ++++---
 mmc_cmds.c | 28 ++++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/mmc.c b/mmc.c
index 795b4e3..3f813b4 100644
--- a/mmc.c
+++ b/mmc.c
@@ -56,9 +56,10 @@ static struct Command commands[] = {
 		"Print extcsd data from <device>.",
 	  NULL
 	},
-	{ do_write_extcsd, 3,
-	  "extcsd write", "<offset> <value> <device>\n"
-		  "Write <value> at offset <offset> to <device>'s extcsd.",
+	{ do_write_extcsd, -3,
+	  "extcsd write", "<offset> <value> <device> [--verify]\n"
+		  "Write <value> at offset <offset> to <device>'s extcsd.\n"
+		  "  --verify  Verify data written",
 	  NULL
 	},
 	{ do_writeprotect_boot_get, -1,
diff --git a/mmc_cmds.c b/mmc_cmds.c
index df66986..154020e 100644
--- a/mmc_cmds.c
+++ b/mmc_cmds.c
@@ -1986,9 +1986,10 @@ int do_write_extcsd(int nargs, char **argv)
 	int fd, ret;
 	int offset, value;
 	char *device;
+	int verify = 0;
 
-	if (nargs != 4) {
-		fprintf(stderr, "Usage: mmc extcsd write <offset> <value> </path/to/mmcblkX>\n");
+	if (nargs != 4 && nargs != 5) {
+		fprintf(stderr, "Usage: mmc extcsd write <offset> <value> </path/to/mmcblkX> [--verify]\n");
 		exit(1);
 	}
 
@@ -1996,6 +1997,14 @@ int do_write_extcsd(int nargs, char **argv)
 	value  = strtol(argv[2], NULL, 0);
 	device = argv[3];
 
+	if (nargs == 5) {
+		if (strcmp(argv[4], "--verify") == 0) {
+			verify = 1;
+		} else {
+			fprintf(stderr, "Unknown argument: '%s'\n", argv[4]);
+		}
+	}
+
 	fd = open(device, O_RDWR);
 	if (fd < 0) {
 		perror("open");
@@ -2010,6 +2019,21 @@ int do_write_extcsd(int nargs, char **argv)
 		exit(1);
 	}
 
+	if (verify) {
+		__u8 ext_csd[512];
+
+		ret = read_extcsd(fd, ext_csd);
+		if (ret) {
+			fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
+			exit(1);
+		}
+
+		if (ext_csd[offset] != value) {
+			fprintf(stderr, "Verification failed: expected 0x%x, got 0x%x\n", value, ext_csd[offset]);
+			exit(1);
+		}
+	}
+
 	return ret;
 }
 
-- 
2.39.2

[PATCH 2/2] mmc-utils: add error handling to 'extcsd write' input value parsing

[Enrico Jorns]

So far, any value was silently accepted, even a

  mmc extcsd write 0x100 yeah /dev/mmcblk1

which resulted in write of value 0x0 at offset 0x0 since the parsing
error of 'yeah' was ignored and the returned value of 0 was taken
unconditionally.
The 0x100 was then implicitly casted down to the expected __u8 input for
the offset and also ended up as 0.

This is not the behavior one would expect when dealing with eMMC
registers that might, depending on which we write to, be writable only
once in the eMMC's lifetime.

This introduces the str_to_u8() helper function that checks if input
values are parsable and have a proper size. Internally it also uses
strtoul() instead of strtol() since input is always expected to be
non-negative. Also, use proper input variables (unsigned long instead
of int) to remove another layer of implicit down casts.

Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
---
 mmc_cmds.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/mmc_cmds.c b/mmc_cmds.c
index 154020e..e348651 100644
--- a/mmc_cmds.c
+++ b/mmc_cmds.c
@@ -1981,10 +1981,28 @@ out_free:
 	return ret;
 }
 
+__u8 str_to_u8(const char* input)
+{
+	unsigned long parsedval;
+	char *endptr;
+
+	parsedval = strtoul(input, &endptr, 0);
+	if (*endptr != '\0') {
+		fprintf(stderr, "Invalid input: %s. Cannot parse to 8 bit integer.\n", input);
+		exit(1);
+	}
+	if (parsedval >= UINT8_MAX) {
+		fprintf(stderr, "Invalid input: Value %s too large 8 bit integer.\n", input);
+		exit(1);
+	}
+
+	return (__u8) parsedval;
+}
+
 int do_write_extcsd(int nargs, char **argv)
 {
 	int fd, ret;
-	int offset, value;
+	__u8 offset, value;
 	char *device;
 	int verify = 0;
 
@@ -1993,8 +2011,8 @@ int do_write_extcsd(int nargs, char **argv)
 		exit(1);
 	}
 
-	offset = strtol(argv[1], NULL, 0);
-	value  = strtol(argv[2], NULL, 0);
+	offset = str_to_u8(argv[1]);
+	value = str_to_u8(argv[2]);
 	device = argv[3];
 
 	if (nargs == 5) {
-- 
2.39.2

RE: [PATCH 1/2] mmc-utils: introduce optional --verify argument for 'extcsd write'

[Christian Loehle]

-----Original Message-----
From: Enrico Jorns <ejo@pengutronix.de> 
Sent: Monday, May 22, 2023 11:53 PM
To: linux-mmc@vger.kernel.org
Cc: Avri Altman <avri.altman@wdc.com>; Ulf Hansson <ulf.hansson@linaro.org>; ejo@pengutronix.de
Subject: [PATCH 1/2] mmc-utils: introduce optional --verify argument for 'extcsd write'

> Registers can be write-once but ioctl does not necessarily return with an error. Thus it is a good idea to allow verifying the data written.

But it will return with an error soon, I'm working on it, anyway I think it's not a bad idea.
User needs to be aware that not all indices can be '--verify'ed, though.

> 
> Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
> ---
>  mmc.c      |  7 ++++---
>  mmc_cmds.c | 28 ++++++++++++++++++++++++++--
>  2 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/mmc.c b/mmc.c
> index 795b4e3..3f813b4 100644
> --- a/mmc.c
> +++ b/mmc.c
> @@ -56,9 +56,10 @@ static struct Command commands[] = {
>  		"Print extcsd data from <device>.",
>  	  NULL
>  	},
> -	{ do_write_extcsd, 3,
> -	  "extcsd write", "<offset> <value> <device>\n"
> -		  "Write <value> at offset <offset> to <device>'s extcsd.",
> +	{ do_write_extcsd, -3,
> +	  "extcsd write", "<offset> <value> <device> [--verify]\n"
> +		  "Write <value> at offset <offset> to <device>'s extcsd.\n"
> +		  "  --verify  Verify data written",
>  	  NULL
>  	},
>  	{ do_writeprotect_boot_get, -1,
> diff --git a/mmc_cmds.c b/mmc_cmds.c
> index df66986..154020e 100644
> --- a/mmc_cmds.c
> +++ b/mmc_cmds.c
> @@ -1986,9 +1986,10 @@ int do_write_extcsd(int nargs, char **argv)
>  	int fd, ret;
>  	int offset, value;
>  	char *device;
> +	int verify = 0;
>  
> -	if (nargs != 4) {
> -		fprintf(stderr, "Usage: mmc extcsd write <offset> <value> </path/to/mmcblkX>\n");
> +	if (nargs != 4 && nargs != 5) {
> +		fprintf(stderr, "Usage: mmc extcsd write <offset> <value> 
> +</path/to/mmcblkX> [--verify]\n");
>  		exit(1);
>  	}
>  
> @@ -1996,6 +1997,14 @@ int do_write_extcsd(int nargs, char **argv)
>  	value  = strtol(argv[2], NULL, 0);
>  	device = argv[3];
>  
> +	if (nargs == 5) {
> +		if (strcmp(argv[4], "--verify") == 0) {
> +			verify = 1;
> +		} else {
> +			fprintf(stderr, "Unknown argument: '%s'\n", argv[4]);
> +		}
> +	}
> +
>  	fd = open(device, O_RDWR);
>  	if (fd < 0) {
>  		perror("open");
> @@ -2010,6 +2019,21 @@ int do_write_extcsd(int nargs, char **argv)
>  		exit(1);
>  	}
>  
> +	if (verify) {
> +		__u8 ext_csd[512];
> +
> +		ret = read_extcsd(fd, ext_csd);
> +		if (ret) {
> +			fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
I think this error message should indicate that the write already happened.

> +			exit(1);
> +		}
> +
> +		if (ext_csd[offset] != value) {
> +			fprintf(stderr, "Verification failed: expected 0x%x, got 0x%x\n", value, ext_csd[offset]);
> +			exit(1);
> +		}
> +	}
> +
>  	return ret;
>  }
>  
> --
> 2.39.2
>

Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782

RE: [PATCH 1/2] mmc-utils: introduce optional --verify argument for 'extcsd write'

[Avri Altman]

 
> Registers can be write-once but ioctl does not necessarily return with an
> error. Thus it is a good idea to allow verifying the data written.
Yeah - I find this approach more practical, than e.g. analyze the R1b response.

> 
> Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
Aked-by: Avri Altman <avri.altman@wdc.com>

> ---
>  mmc.c      |  7 ++++---
>  mmc_cmds.c | 28 ++++++++++++++++++++++++++--
>  2 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/mmc.c b/mmc.c
> index 795b4e3..3f813b4 100644
> --- a/mmc.c
> +++ b/mmc.c
> @@ -56,9 +56,10 @@ static struct Command commands[] = {
>                 "Print extcsd data from <device>.",
>           NULL
>         },
> -       { do_write_extcsd, 3,
> -         "extcsd write", "<offset> <value> <device>\n"
> -                 "Write <value> at offset <offset> to <device>'s extcsd.",
> +       { do_write_extcsd, -3,
> +         "extcsd write", "<offset> <value> <device> [--verify]\n"
> +                 "Write <value> at offset <offset> to <device>'s extcsd.\n"
> +                 "  --verify  Verify data written",
>           NULL
>         },
>         { do_writeprotect_boot_get, -1,
> diff --git a/mmc_cmds.c b/mmc_cmds.c
> index df66986..154020e 100644
> --- a/mmc_cmds.c
> +++ b/mmc_cmds.c
> @@ -1986,9 +1986,10 @@ int do_write_extcsd(int nargs, char **argv)
>         int fd, ret;
>         int offset, value;
>         char *device;
> +       int verify = 0;
> 
> -       if (nargs != 4) {
> -               fprintf(stderr, "Usage: mmc extcsd write <offset> <value>
> </path/to/mmcblkX>\n");
> +       if (nargs != 4 && nargs != 5) {
> +               fprintf(stderr, "Usage: mmc extcsd write <offset>
> + <value> </path/to/mmcblkX> [--verify]\n");
>                 exit(1);
>         }
> 
> @@ -1996,6 +1997,14 @@ int do_write_extcsd(int nargs, char **argv)
>         value  = strtol(argv[2], NULL, 0);
>         device = argv[3];
> 
> +       if (nargs == 5) {
> +               if (strcmp(argv[4], "--verify") == 0) {
> +                       verify = 1;
> +               } else {
> +                       fprintf(stderr, "Unknown argument: '%s'\n", argv[4]);
> +               }
> +       }
> +
>         fd = open(device, O_RDWR);
>         if (fd < 0) {
>                 perror("open");
> @@ -2010,6 +2019,21 @@ int do_write_extcsd(int nargs, char **argv)
>                 exit(1);
>         }
> 
> +       if (verify) {
> +               __u8 ext_csd[512];
> +
> +               ret = read_extcsd(fd, ext_csd);
> +               if (ret) {
> +                       fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
> +                       exit(1);
> +               }
> +
> +               if (ext_csd[offset] != value) {
> +                       fprintf(stderr, "Verification failed: expected 0x%x, got 0x%x\n",
> value, ext_csd[offset]);
> +                       exit(1);
> +               }
> +       }
> +
>         return ret;
>  }
> 
> --
> 2.39.2

RE: [PATCH 2/2] mmc-utils: add error handling to 'extcsd write' input value parsing

[Avri Altman]

> -----Original Message-----
> From: Enrico Jorns <ejo@pengutronix.de>
> Sent: Tuesday, May 23, 2023 12:53 AM
> To: linux-mmc@vger.kernel.org
> Cc: Avri Altman <Avri.Altman@wdc.com>; Ulf Hansson
> <ulf.hansson@linaro.org>; ejo@pengutronix.de
> Subject: [PATCH 2/2] mmc-utils: add error handling to 'extcsd write' input
> value parsing
> 
> CAUTION: This email originated from outside of Western Digital. Do not click
> on links or open attachments unless you recognize the sender and know that
> the content is safe.
> 
> 
> So far, any value was silently accepted, even a
> 
>   mmc extcsd write 0x100 yeah /dev/mmcblk1
> 
> which resulted in write of value 0x0 at offset 0x0 since the parsing error of
> 'yeah' was ignored and the returned value of 0 was taken unconditionally.
> The 0x100 was then implicitly casted down to the expected __u8 input for the
> offset and also ended up as 0.
The largest writable offset is 191 - maybe use this for valid offset.
There are writable fields that are more than a single byte in size.
Are you still allowing writing those?

Thanks,
Avri


> 
> This is not the behavior one would expect when dealing with eMMC registers
> that might, depending on which we write to, be writable only once in the
> eMMC's lifetime.
> 
> This introduces the str_to_u8() helper function that checks if input values are
> parsable and have a proper size. Internally it also uses
> strtoul() instead of strtol() since input is always expected to be non-negative.
> Also, use proper input variables (unsigned long instead of int) to remove
> another layer of implicit down casts.
> 
> Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
> ---
>  mmc_cmds.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/mmc_cmds.c b/mmc_cmds.c
> index 154020e..e348651 100644
> --- a/mmc_cmds.c
> +++ b/mmc_cmds.c
> @@ -1981,10 +1981,28 @@ out_free:
>         return ret;
>  }
> 
> +__u8 str_to_u8(const char* input)
> +{
> +       unsigned long parsedval;
> +       char *endptr;
> +
> +       parsedval = strtoul(input, &endptr, 0);
> +       if (*endptr != '\0') {
> +               fprintf(stderr, "Invalid input: %s. Cannot parse to 8 bit integer.\n",
> input);
> +               exit(1);
> +       }
> +       if (parsedval >= UINT8_MAX) {
> +               fprintf(stderr, "Invalid input: Value %s too large 8 bit integer.\n",
> input);
> +               exit(1);
> +       }
> +
> +       return (__u8) parsedval;
> +}
> +
>  int do_write_extcsd(int nargs, char **argv)  {
>         int fd, ret;
> -       int offset, value;
> +       __u8 offset, value;
>         char *device;
>         int verify = 0;
> 
> @@ -1993,8 +2011,8 @@ int do_write_extcsd(int nargs, char **argv)
>                 exit(1);
>         }
> 
> -       offset = strtol(argv[1], NULL, 0);
> -       value  = strtol(argv[2], NULL, 0);
> +       offset = str_to_u8(argv[1]);
> +       value = str_to_u8(argv[2]);
>         device = argv[3];
> 
>         if (nargs == 5) {
> --
> 2.39.2