Re: [qemu-s390x] [PATCH v2 1/5] s390-ccw: update libc

[Thomas Huth]

On 11.12.2017 23:19, Collin L. Walling wrote:
> Moved:
>   memcmp from bootmap.h to libc.h (renamed from _memcmp)
>   strlen from sclp.c to libc.h (renamed from _strlen)
> 
> Added C standard functions:
>   isdigit
>   atoi
> 
> Added non-C standard function:
>   itostr
> 
> Signed-off-by: Collin L. Walling <address@hidden>
> Acked-by: Christian Borntraeger <address@hidden>
> Reviewed-by: Janosch Frank <address@hidden>
> ---
>  pc-bios/s390-ccw/Makefile  |  2 +-
>  pc-bios/s390-ccw/bootmap.c |  4 +--
>  pc-bios/s390-ccw/bootmap.h | 16 +---------
>  pc-bios/s390-ccw/libc.c    | 75 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  pc-bios/s390-ccw/libc.h    | 31 +++++++++++++++++++
>  pc-bios/s390-ccw/main.c    | 17 +----------
>  pc-bios/s390-ccw/sclp.c    | 10 +------
>  7 files changed, 112 insertions(+), 43 deletions(-)
>  create mode 100644 pc-bios/s390-ccw/libc.c
[...]
> +
> +/**
> + * itostr:
> + * @num: the integer to be converted.
> + * @str: a pointer to a string to store the conversion.
> + * @len: the length of the passed string.
> + *
> + * Given an integer @num, convert it to a string. The string @str must be
> + * allocated beforehand. The resulting string will be null terminated and
> + * returned.
> + *
> + * Returns: the string @str of the converted integer @num.
> + */
> +char *itostr(int num, char *str, size_t len)
> +{
> +    long num_len = 1;
> +    int tmp = num;
> +    int i;
> +
> +    /* Count length of num */
> +    while ((tmp /= 10) > 0) {
> +        num_len++;
> +    }
> +
> +    /* Check if we have enough space for num and null */
> +    if (len < num_len) {
> +        return 0;
> +    }

I'm afraid, but I think you've got an off-by-one bug in this code.

In patch 5, you're using this function like this:

    char tmp[4];

    sclp_print(itostr(entries, tmp, sizeof(tmp)));

That means if entries is >= 1000 for example, num_len is 4 ...

> +    /* Convert int to string */
> +    for (i = num_len - 1; i >= 0; i--) {
> +        str[i] = num % 10 + '0';
> +        num /= 10;
> +    }
> +
> +    str[num_len] = '\0';

... and then you run into a buffer overflow here.

> +    return str;
> +}

Maybe it would also make more sense to panic() instead of "return 0"
since you don't check the return value in patch 5 ?

 Thomas