Re: [PATCH 4/6] coverity-model: clean up the models for array allocation functions

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 02/08/21 14:36, Peter Maydell wrote:
>> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
>> The real g_malloc_n() returns failure if the multiplication
>> would overflow;

Really?  Its documentation:

    This function is similar to g_malloc(), allocating (n_blocks *
    n_block_bytes) bytes, but care is taken to detect possible overflow
    during multiplication.

There's also g_try_malloc_n():

    This function is similar to g_try_malloc(), allocating (n_blocks *
    n_block_bytes) bytes, but care is taken to detect possible overflow
    during multiplication.

I read this as g_malloc_n() can return null only for zero size, and
crashes on error, just like g_malloc_n().  g_try_malloc_n() behaves like
g_try_malloc_n(), i.e. it returns null on failure.

>>                 I guess Coverity currently doesn't have any
>> warnings it generates as a result of assuming overflow
>> might happen?
>
> I couldn't find any Coverity-specific way to detect overflow, but

Does it need one?

Quick peek at Coverity 2012.12 Checker Reference:

    4.188. INTEGER_OVERFLOW
    [...]
    4.188.1. Overview

    Supported Languages: C, C++, Objective-C, Objective-C++

    INTEGER_OVERFLOW finds many cases of integer overflows and
    truncations resulting from arithmetic operations. Some forms of
    integer overflow can cause security vulnerabilities, for example,
    when the overflowed value is used as the argument to an allocation
    function. [...]

    [...]

    Disabled by default: INTEGER_OVERFLOW is disabled by default. To
    enable it, you can use the --enable option to the cov-analyze
    command.

The example that follows shows a memory allocation where the size is
computed like TAINTED * sizeof(MUMBLE), where TAINTED is a "tainted
source", and the allocation's size is a "tainted sink".

Our model for g_malloc_n() uses __coverity_alloc(), which should provide
"tainted sink".

> making nmemb a tainted sink could be an interesting way to ensure that 
> untrusted data does not end up causing such a failure.
>
> Likewise, we should try making __bufwrite taint the buffer it is
> writing to; there's already a TODO for that but I never followed up on
> it.

__bufwrite() tells Coverity that the buf[len] bytes are overwritten with
unspecified data.  I'd expect that to taint the buffer already.