| Message ID | 20210731062741.301102-5-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Updates for Coverity modeling file | expand |
On Sat, 31 Jul 2021 at 07:31, Paolo Bonzini <pbonzini@redhat.com> wrote: > > sz is only used in one place, so replace it with nmemb * size in > that one place. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > scripts/coverity-scan/model.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c > index 1a5f39d2ae..2d384bdd79 100644 > --- a/scripts/coverity-scan/model.c > +++ b/scripts/coverity-scan/model.c > @@ -178,13 +178,11 @@ uint8_t replay_get_byte(void) > > void *g_malloc_n(size_t nmemb, size_t size) > { > - size_t sz; > void *ptr; > > __coverity_negative_sink__(nmemb); > __coverity_negative_sink__(size); > - sz = nmemb * size; > - ptr = __coverity_alloc__(sz); > + ptr = __coverity_alloc__(nmemb * size); > __coverity_mark_as_uninitialized_buffer__(ptr); > __coverity_mark_as_afm_allocated__(ptr, AFM_free); > return ptr; Reviewed-by: Peter Maydell <peter.maydell@linaro.org> The real g_malloc_n() returns failure if the multiplication would overflow; I guess Coverity currently doesn't have any warnings it generates as a result of assuming overflow might happen? thanks -- PMM
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; 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 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. Paolo
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.
On Wed, 4 Aug 2021 at 09:35, Markus Armbruster <armbru@redhat.com> wrote: > > 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. Yeah, I misspoke -- I just meant to say "treats multiply overflow like an allocation failure", not that it returns NULL. > >> 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 > 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". My point was that we *don't* want Coverity to report INTEGER_OVERFLOW errors based on the way our g_malloc_n() model is written like this: __coverity_alloc__(nmemb * size) That expression in our model *can* overflow. We know the real g_malloc_n() cannot overflow, but we don't do anything to tell Coverity that, so it's possible that (maybe in future) it will report false positives to us based on analysis that assumes possible overflows in the multiplication. thanks -- PMM
diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c index 1a5f39d2ae..2d384bdd79 100644 --- a/scripts/coverity-scan/model.c +++ b/scripts/coverity-scan/model.c @@ -178,13 +178,11 @@ uint8_t replay_get_byte(void) void *g_malloc_n(size_t nmemb, size_t size) { - size_t sz; void *ptr; __coverity_negative_sink__(nmemb); __coverity_negative_sink__(size); - sz = nmemb * size; - ptr = __coverity_alloc__(sz); + ptr = __coverity_alloc__(nmemb * size); __coverity_mark_as_uninitialized_buffer__(ptr); __coverity_mark_as_afm_allocated__(ptr, AFM_free); return ptr; @@ -192,13 +190,11 @@ void *g_malloc_n(size_t nmemb, size_t size) void *g_malloc0_n(size_t nmemb, size_t size) { - size_t sz; void *ptr; __coverity_negative_sink__(nmemb); __coverity_negative_sink__(size); - sz = nmemb * size; - ptr = __coverity_alloc__(sz); + ptr = __coverity_alloc__(nmemb * size); __coverity_writeall0__(ptr); __coverity_mark_as_afm_allocated__(ptr, AFM_free); return ptr; @@ -206,13 +202,10 @@ void *g_malloc0_n(size_t nmemb, size_t size) void *g_realloc_n(void *ptr, size_t nmemb, size_t size) { - size_t sz; - __coverity_negative_sink__(nmemb); __coverity_negative_sink__(size); - sz = nmemb * size; __coverity_escape__(ptr); - ptr = __coverity_alloc__(sz); + ptr = __coverity_alloc__(nmemb * size); /* * Memory beyond the old size isn't actually initialized. Can't * model that. See Coverity's realloc() model
sz is only used in one place, so replace it with nmemb * size in that one place. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- scripts/coverity-scan/model.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)