[v2,2/4] block/ssh.c: Don't double-check that characters are hex digits

Commit Message

Peter Maydell Oct. 8, 2024, 4:47 p.m. UTC

In compare_fingerprint() we effectively check whether the characters
in the fingerprint are valid hex digits twice: first we do so with
qemu_isxdigit(), but then the hex2decimal() function also has a code
path where it effectively detects an invalid digit and returns -1.
This causes Coverity to complain because it thinks that we might use
that -1 value in an expression where it would be an integer overflow.

Avoid the double-check of hex digit validity by testing the return
values from hex2decimal() rather than doing separate calls to
qemu_isxdigit().

Since this means we now use the illegal-character return value
from hex2decimal(), rewrite it from "-1" to "UINT_MAX", which
has the same effect since the return type is "unsigned" but
looks less confusing at the callsites when we detect it with
"c0 > 0xf".

Resolves: Coverity CID 1547813
Signed-off-by: Peter Maydell <[e-mail]>
Reviewed-by: Kevin Wolf <[e-mail]>
---
v1->v2: make hex2decimal() return UINT_MAX, not -1
---
 block/ssh.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Richard Henderson Oct. 8, 2024, 6:46 p.m. UTC | #1

On 10/8/24 09:47, Peter Maydell wrote:
> In compare_fingerprint() we effectively check whether the characters
> in the fingerprint are valid hex digits twice: first we do so with
> qemu_isxdigit(), but then the hex2decimal() function also has a code
> path where it effectively detects an invalid digit and returns -1.
> This causes Coverity to complain because it thinks that we might use
> that -1 value in an expression where it would be an integer overflow.
> 
> Avoid the double-check of hex digit validity by testing the return
> values from hex2decimal() rather than doing separate calls to
> qemu_isxdigit().
> 
> Since this means we now use the illegal-character return value
> from hex2decimal(), rewrite it from "-1" to "UINT_MAX", which
> has the same effect since the return type is "unsigned" but
> looks less confusing at the callsites when we detect it with
> "c0 > 0xf".
> 
> Resolves: Coverity CID 1547813
> Signed-off-by: Peter Maydell<[e-mail]>
> Reviewed-by: Kevin Wolf<[e-mail]>
> ---
> v1->v2: make hex2decimal() return UINT_MAX, not -1
> ---
>   block/ssh.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <[e-mail]>

r~

Patch

diff --git a/block/ssh.c b/block/ssh.c
index 871e1d47534..9f8140bcb68 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -364,7 +364,7 @@  static unsigned hex2decimal(char ch)
         return 10 + (ch - 'A');
     }
 
-    return -1;
+    return UINT_MAX;
 }
 
 /* Compare the binary fingerprint (hash of host key) with the
@@ -376,13 +376,15 @@  static int compare_fingerprint(const unsigned char *fingerprint, size_t len,
     unsigned c;
 
     while (len > 0) {
+        unsigned c0, c1;
         while (*host_key_check == ':')
             host_key_check++;
-        if (!qemu_isxdigit(host_key_check[0]) ||
-            !qemu_isxdigit(host_key_check[1]))
+        c0 = hex2decimal(host_key_check[0]);
+        c1 = hex2decimal(host_key_check[1]);
+        if (c0 > 0xf || c1 > 0xf) {
             return 1;
-        c = hex2decimal(host_key_check[0]) * 16 +
-            hex2decimal(host_key_check[1]);
+        }
+        c = c0 * 16 + c1;
         if (c - *fingerprint != 0)
             return c - *fingerprint;
         fingerprint++;