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().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Could alternatively have put a g_assert_non_reached() in
hex2decimal(), but this seemed better to me.
---
 block/ssh.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 27d582e0e3d..510dd208aba 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -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++;
-- 
2.34.1

Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben:
> 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.

If it's a Coverity issue, I think you want to mention the CID, too.

> Avoid the double-check of hex digit validity by testing the return
> values from hex2decimal() rather than doing separate calls to
> qemu_isxdigit().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Could alternatively have put a g_assert_non_reached() in
> hex2decimal(), but this seemed better to me.

I don't like that hex2decimal() returns -1 when its result is unsigned,
which is why you had to write the check as > 0xf rather than < 0. So in
this sense, g_assert_non_reached() would look better to me. But we could
also just change it to return UINT_MAX instead, which should be the
same, just written in a less confusing way.

>  block/ssh.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index 27d582e0e3d..510dd208aba 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -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++;

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

On Wed, 31 Jul 2024 at 16:21, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben:
> > 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.
>
> If it's a Coverity issue, I think you want to mention the CID, too.

Yes;

Resolves: Coverity CID 1547813

> > Avoid the double-check of hex digit validity by testing the return
> > values from hex2decimal() rather than doing separate calls to
> > qemu_isxdigit().
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Could alternatively have put a g_assert_non_reached() in
> > hex2decimal(), but this seemed better to me.
>
> I don't like that hex2decimal() returns -1 when its result is unsigned,
> which is why you had to write the check as > 0xf rather than < 0. So in
> this sense, g_assert_non_reached() would look better to me. But we could
> also just change it to return UINT_MAX instead, which should be the
> same, just written in a less confusing way.

I was not super happy with the -1 either. Happy to change that
to 'return UINT_MAX'.

-- PMM