Re: [PATCH] coverity: physmem: use simple assertions instead of modelling

[David Hildenbrand <david@redhat.com>]

On 26.12.22 23:03, Vladimir Sementsov-Ogievskiy wrote:
> Unfortunately Coverity doesn't follow the logic aroung "len" and "l"
> variables in stacks finishing with flatview_{read,write}_continue() and
> generate a lot of OVERRUN false-positives. When small buffer (2 or 4
> bytes) is passed to mem read/write path, Coverity assumes the worst
> case of sz=8 in stn_he_p()/ldn_he_p() (defined in
> include/qemu/bswap.h), and reports buffer overrun.
> 
> To silence these false-positives we have model functions, which hide
> real logic from Coverity.
> 
> However, it turned out that these new two assertions are enough to
> quiet Coverity.
> 
> Assertions are better than hiding the logic, so let's drop the
> modelling and move to assertions for memory r/w call stacks.
> 
> After patch, the sequence
> 
>   cov-make-library --output-file /tmp/master.xmldb \
>      scripts/coverity-scan/model.c
>   cov-build --dir ~/covtmp/master make -j9
>   cov-analyze --user-model-file /tmp/master.xmldb \
>      --dir ~/covtmp/master --all --strip-path "$(pwd)
>   cov-format-errors --dir ~/covtmp/master \
>      --html-output ~/covtmp/master_html_report
> 
> Generate for me the same big set of CIDs excepept for 6 disappeared (so
> it becomes even better).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   scripts/coverity-scan/model.c | 88 -----------------------------------
>   softmmu/physmem.c             | 18 +++++++
>   2 files changed, 18 insertions(+), 88 deletions(-)
> 
> diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c
> index 686d1a3008..a064d84084 100644
> --- a/scripts/coverity-scan/model.c
> +++ b/scripts/coverity-scan/model.c
> @@ -42,94 +42,6 @@ typedef _Bool bool;
>   
>   typedef struct va_list_str *va_list;
>   
> -/* exec.c */
> -
> -typedef struct AddressSpace AddressSpace;
> -typedef struct MemoryRegionCache MemoryRegionCache;
> -typedef uint64_t hwaddr;
> -typedef uint32_t MemTxResult;
> -typedef struct MemTxAttrs {} MemTxAttrs;
> -
> -static void __bufwrite(uint8_t *buf, ssize_t len)
> -{
> -    int first, last;
> -    __coverity_negative_sink__(len);
> -    if (len == 0) return;
> -    buf[0] = first;
> -    buf[len-1] = last;
> -    __coverity_writeall__(buf);
> -}
> -
> -static void __bufread(uint8_t *buf, ssize_t len)
> -{
> -    __coverity_negative_sink__(len);
> -    if (len == 0) return;
> -    int first = buf[0];
> -    int last = buf[len-1];
> -}
> -
> -MemTxResult address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
> -                                      MemTxAttrs attrs,
> -                                      void *buf, int len)
> -{
> -    MemTxResult result;
> -    // TODO: investigate impact of treating reads as producing
> -    // tainted data, with __coverity_tainted_data_argument__(buf).
> -    __bufwrite(buf, len);
> -    return result;
> -}
> -
> -MemTxResult address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
> -                                MemTxAttrs attrs,
> -                                const void *buf, int len)
> -{
> -    MemTxResult result;
> -    __bufread(buf, len);
> -    return result;
> -}
> -
> -MemTxResult address_space_rw_cached(MemoryRegionCache *cache, hwaddr addr,
> -                                    MemTxAttrs attrs,
> -                                    void *buf, int len, bool is_write)
> -{
> -    if (is_write) {
> -        return address_space_write_cached(cache, addr, attrs, buf, len);
> -    } else {
> -        return address_space_read_cached(cache, addr, attrs, buf, len);
> -    }
> -}
> -
> -MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
> -                               MemTxAttrs attrs,
> -                               void *buf, int len)
> -{
> -    MemTxResult result;
> -    // TODO: investigate impact of treating reads as producing
> -    // tainted data, with __coverity_tainted_data_argument__(buf).
> -    __bufwrite(buf, len);
> -    return result;
> -}
> -
> -MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
> -                                MemTxAttrs attrs,
> -                                const void *buf, int len)
> -{
> -    MemTxResult result;
> -    __bufread(buf, len);
> -    return result;
> -}
> -
> -MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
> -                             MemTxAttrs attrs,
> -                             void *buf, int len, bool is_write)
> -{
> -    if (is_write) {
> -        return address_space_write(as, addr, attrs, buf, len);
> -    } else {
> -        return address_space_read(as, addr, attrs, buf, len);
> -    }
> -}
> -
>   /* Tainting */
>   
>   typedef struct {} name2keysym_t;
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index edec095c7a..24571002b3 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -2821,6 +2821,15 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
>               l = memory_access_size(mr, l, addr1);
>               /* XXX: could force current_cpu to NULL to avoid
>                  potential bugs */
> +
> +            /*
> +             * Assure Coverity (and ourselves) that we are not going to OVERRUN
> +             * the buffer by following ldn_he_p().
> +             */
> +            assert((l == 1 && len >= 1) ||
> +                   (l == 2 && len >= 2) ||
> +                   (l == 4 && len >= 4) ||
> +                   (l == 8 && len >= 8));
>               val = ldn_he_p(buf, l);
>               result |= memory_region_dispatch_write(mr, addr1, val,
>                                                      size_memop(l), attrs);
> @@ -2891,6 +2900,15 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
>               l = memory_access_size(mr, l, addr1);
>               result |= memory_region_dispatch_read(mr, addr1, &val,
>                                                     size_memop(l), attrs);
> +
> +            /*
> +             * Assure Coverity (and ourselves) that we are not going to OVERRUN
> +             * the buffer by following stn_he_p().
> +             */
> +            assert((l == 1 && len >= 1) ||
> +                   (l == 2 && len >= 2) ||
> +                   (l == 4 && len >= 4) ||
> +                   (l == 8 && len >= 8));
>               stn_he_p(buf, l, val);
>           } else {
>               /* RAM case */

I'm no coverity expert, but if it gets the job done reliably

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb