[PATCH] spi: tools: fix input string formatting

[PATCH] spi: tools: fix input string formatting

[Aleksandar Gerasimovski]

The actual unescape implementation has two bugs:
1. quotation marks from the input string are not removed and are sent
  to the spidev, e.g: input string: \"\\xFE\\x01\" will be sent to the
  spidev as 0x22 0xfe 0x01 0x22
2. there is not format check for decimal input strings

First bug makes spidev_test unusable when strict spi sequence is needed,
second bug is not nice to have it in.

This patch improves unescape function and fixes above listed bugs.

Signed-off-by: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
---
 tools/spi/spidev_test.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/spi/spidev_test.c b/tools/spi/spidev_test.c
index 83844f8..7c36677 100644
--- a/tools/spi/spidev_test.c
+++ b/tools/spi/spidev_test.c
@@ -89,7 +89,7 @@ static void hex_dump(const void *src, size_t length, size_t line_size,
 
 /*
  *  Unescape - process hexadecimal escape character
- *      converts shell input "\x23" -> 0x23
+ *      converts shell input "\\x23" -> 0x23
  */
 static int unescape(char *_dst, char *_src, size_t len)
 {
@@ -100,6 +100,10 @@ static int unescape(char *_dst, char *_src, size_t len)
 	unsigned int ch;
 
 	while (*src) {
+		if (*src == '"') {
+			src++;
+			continue;
+		}
 		if (*src == '\\' && *(src+1) == 'x') {
 			match = sscanf(src + 2, "%2x", &ch);
 			if (!match)
@@ -108,6 +112,9 @@ static int unescape(char *_dst, char *_src, size_t len)
 			src += 4;
 			*dst++ = (unsigned char)ch;
 		} else {
+			match = sscanf(src, "%2d", &ch);
+			if (!match)
+				pabort("malformed input string");
 			*dst++ = *src++;
 		}
 		ret++;
-- 
1.8.3.1

Re: [PATCH] spi: tools: fix input string formatting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]

On Fri, Feb 05, 2021 at 08:04:10AM +0000, Aleksandar Gerasimovski wrote:
> The actual unescape implementation has two bugs:

This is two bugs with two separate fixes, it should be two separate
patches.

> 1. quotation marks from the input string are not removed and are sent
>   to the spidev, e.g: input string: \"\\xFE\\x01\" will be sent to the
>   spidev as 0x22 0xfe 0x01 0x22

It is not clear to me what the issue you see here is - could you be more
specific about where you see this input string and why you believe that
the handling is incorrect?  After going through the shell the above will
be

	"\xFE\x01"

which includes quotation marks which then get sent on to the device.

>  /*
>   *  Unescape - process hexadecimal escape character
> - *      converts shell input "\x23" -> 0x23
> + *      converts shell input "\\x23" -> 0x23
>   */

This is changing the documented input format and not mentioned in the
changelog?

> +		if (*src == '"') {
> +			src++;
> +			continue;
> +		}
>  		if (*src == '\\' && *(src+1) == 'x') {
>  			match = sscanf(src + 2, "%2x", &ch);
>  			if (!match)

This just appears to ignore quotes which isn't at all what I'd expect?

> @@ -108,6 +112,9 @@ static int unescape(char *_dst, char *_src, size_t len)
>  			src += 4;
>  			*dst++ = (unsigned char)ch;
>  		} else {
> +			match = sscanf(src, "%2d", &ch);
> +			if (!match)
> +				pabort("malformed input string");
>  			*dst++ = *src++;

This appears to be requiring that anything passed into unescape() be a
number which isn't something we'd obviously want?  I'd expect the
function to unescape things, not to do other random validation which may
or may not be appropriate in context.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] spi: tools: fix input string formatting

[Aleksandar Gerasimovski]

Hi Mark,

Thanks for the feedback, see inline:

-----Original Message-----
From: Mark Brown <broonie@kernel.org> 
Sent: Freitag, 5. Februar 2021 17:21
To: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
Cc: linux-spi@vger.kernel.org
Subject: Re: [PATCH] spi: tools: fix input string formatting

On Fri, Feb 05, 2021 at 08:04:10AM +0000, Aleksandar Gerasimovski wrote:
> The actual unescape implementation has two bugs:

This is two bugs with two separate fixes, it should be two separate patches.
AG: Ok, I can provide two patches if needed.

> 1. quotation marks from the input string are not removed and are sent
>   to the spidev, e.g: input string: \"\\xFE\\x01\" will be sent to the
>   spidev as 0x22 0xfe 0x01 0x22

It is not clear to me what the issue you see here is - could you be more specific about where you see this input string and why you believe that the handling is incorrect?  After going through the shell the above will be

	"\xFE\x01"

which includes quotation marks which then get sent on to the device.

>  /*
>   *  Unescape - process hexadecimal escape character
> - *      converts shell input "\x23" -> 0x23
> + *      converts shell input "\\x23" -> 0x23
>   */

This is changing the documented input format and not mentioned in the changelog?
AG: Ok my bad!

> +		if (*src == '"') {
> +			src++;
> +			continue;
> +		}
>  		if (*src == '\\' && *(src+1) == 'x') {
>  			match = sscanf(src + 2, "%2x", &ch);
>  			if (!match)

This just appears to ignore quotes which isn't at all what I'd expect?
AG: to be sure we understand each other, you expect quotes to be sent to spi as well? That's expected by design behavior?
Is there any possibility to avoid sending them then?

> @@ -108,6 +112,9 @@ static int unescape(char *_dst, char *_src, size_t len)
>  			src += 4;
>  			*dst++ = (unsigned char)ch;
>  		} else {
> +			match = sscanf(src, "%2d", &ch);
> +			if (!match)
> +				pabort("malformed input string");
>  			*dst++ = *src++;

This appears to be requiring that anything passed into unescape() be a number which isn't something we'd obviously want?  I'd expect the function to unescape things, not to do other random validation which may or may not be appropriate in context.
AG: so by design is expected that everything is accepted here, e.g \"1234qwert\\xde\\xad\"? If yes than I understood this tool wrongly.

Re: [PATCH] spi: tools: fix input string formatting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

On Sat, Feb 06, 2021 at 10:57:04AM +0000, Aleksandar Gerasimovski wrote:

> AG: to be sure we understand each other, you expect quotes to be sent to spi as well? That's expected by design behavior?
> Is there any possibility to avoid sending them then?

If you don't want quotes to be sent then don't send them - my
expectation would be that if you're driving this from the shell then the
shell would not be passing unescaped quotes through to the program.  For
example:

	$ echo "Hello, world!"
	Hello, world!

Here echo only saw the unqouted string so that's what it displays.  If
you're not using a shell and starting this from another program then I'd
assume there's some way to generate unquoted arguments in whatever
you're using.

> 
> This appears to be requiring that anything passed into unescape() be a number which isn't something we'd obviously want?  I'd expect the function to unescape things, not to do other random validation which may or may not be appropriate in context.
> AG: so by design is expected that everything is accepted here, e.g \"1234qwert\\xde\\xad\"? If yes than I understood this tool wrongly.

Yes, that's my expecation - it's just processing escape sequences and
passing everything else through.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] spi: tools: fix input string formatting

[Aleksandar Gerasimovski]

Ok, thanks for clarification.
You can forget this patch! 

-----Original Message-----
From: Mark Brown <broonie@kernel.org> 
Sent: Montag, 8. Februar 2021 11:09
To: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
Cc: linux-spi@vger.kernel.org
Subject: Re: [PATCH] spi: tools: fix input string formatting

On Sat, Feb 06, 2021 at 10:57:04AM +0000, Aleksandar Gerasimovski wrote:

> AG: to be sure we understand each other, you expect quotes to be sent to spi as well? That's expected by design behavior?
> Is there any possibility to avoid sending them then?

If you don't want quotes to be sent then don't send them - my expectation would be that if you're driving this from the shell then the shell would not be passing unescaped quotes through to the program.  For
example:

	$ echo "Hello, world!"
	Hello, world!

Here echo only saw the unqouted string so that's what it displays.  If you're not using a shell and starting this from another program then I'd assume there's some way to generate unquoted arguments in whatever you're using.

> 
> This appears to be requiring that anything passed into unescape() be a number which isn't something we'd obviously want?  I'd expect the function to unescape things, not to do other random validation which may or may not be appropriate in context.
> AG: so by design is expected that everything is accepted here, e.g \"1234qwert\\xde\\xad\"? If yes than I understood this tool wrongly.

Yes, that's my expecation - it's just processing escape sequences and passing everything else through.