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.mayd...@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 <kw...@redhat.com>
