| Message ID | 20221226220351.754204-1-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | coverity: physmem: use simple assertions instead of modelling | expand |
ping
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>
ping [add Stefan]
On Wed, 15 Feb 2023 at 15:22, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: > > ping > > [add Stefan] I'm not familiar with the Coverity models. Peter Maydell is the maintainer. Stefan
On Wed, 22 Feb 2023 at 14:19, Stefan Hajnoczi <stefanha@gmail.com> wrote: > > On Wed, 15 Feb 2023 at 15:22, Vladimir Sementsov-Ogievskiy > <vsementsov@yandex-team.ru> wrote: > > > > ping > > > > [add Stefan] > > I'm not familiar with the Coverity models. Peter Maydell is the maintainer. We haven't run Coverity scans since September last year. There's no point making changes to our model until we've fixed that. Paolo? thanks -- PMM
On 22.02.23 18:57, Peter Maydell wrote: > On Wed, 22 Feb 2023 at 14:19, Stefan Hajnoczi <stefanha@gmail.com> wrote: >> >> On Wed, 15 Feb 2023 at 15:22, Vladimir Sementsov-Ogievskiy >> <vsementsov@yandex-team.ru> wrote: >>> >>> ping >>> >>> [add Stefan] >> >> I'm not familiar with the Coverity models. Peter Maydell is the maintainer. > > We haven't run Coverity scans since September last year. What's the problem with it? May I help somehow? > There's no point making changes to our model until we've > fixed that. Paolo? > > thanks > -- PMM
On 3/15/23 15:28, Vladimir Sementsov-Ogievskiy wrote: > On 22.02.23 18:57, Peter Maydell wrote: >> On Wed, 22 Feb 2023 at 14:19, Stefan Hajnoczi <stefanha@gmail.com> wrote: >>> >>> On Wed, 15 Feb 2023 at 15:22, Vladimir Sementsov-Ogievskiy >>> <vsementsov@yandex-team.ru> wrote: >>>> >>>> ping >>>> >>>> [add Stefan] >>> >>> I'm not familiar with the Coverity models. Peter Maydell is the >>> maintainer. >> >> We haven't run Coverity scans since September last year. > > What's the problem with it? May I help somehow? The container broke when libslirp was removed, and I've been procrastinating fixing it. :( Paolo > >> There's no point making changes to our model until we've >> fixed that. Paolo? >> >> thanks >> -- PMM >
On 16.03.23 00:22, Paolo Bonzini wrote: > On 3/15/23 15:28, Vladimir Sementsov-Ogievskiy wrote: >> On 22.02.23 18:57, Peter Maydell wrote: >>> On Wed, 22 Feb 2023 at 14:19, Stefan Hajnoczi <stefanha@gmail.com> wrote: >>>> >>>> On Wed, 15 Feb 2023 at 15:22, Vladimir Sementsov-Ogievskiy >>>> <vsementsov@yandex-team.ru> wrote: >>>>> >>>>> ping >>>>> >>>>> [add Stefan] >>>> >>>> I'm not familiar with the Coverity models. Peter Maydell is the maintainer. >>> >>> We haven't run Coverity scans since September last year. >> >> What's the problem with it? May I help somehow? > > The container broke when libslirp was removed, and I've been procrastinating fixing it. 🙁 > > Paolo Hi! I see Coverity works again. Could we give this patch a try? Locally, I now run Coverity on master, on master with dropped model (half of my patch) and with my full patch. The model, that this patch drops, fixes 94 issues. The assertion I propose fixes same 94 issues and two more resource leaks. The model, that this patch drops, also bring 4 issues. The assertion I propose brings no new issues. Of course, my local setup is different from QEMU Coverity cloud run.
ping On 20.04.23 22:06, Vladimir Sementsov-Ogievskiy wrote: > On 16.03.23 00:22, Paolo Bonzini wrote: >> On 3/15/23 15:28, Vladimir Sementsov-Ogievskiy wrote: >>> On 22.02.23 18:57, Peter Maydell wrote: >>>> On Wed, 22 Feb 2023 at 14:19, Stefan Hajnoczi <stefanha@gmail.com> wrote: >>>>> >>>>> On Wed, 15 Feb 2023 at 15:22, Vladimir Sementsov-Ogievskiy >>>>> <vsementsov@yandex-team.ru> wrote: >>>>>> >>>>>> ping >>>>>> >>>>>> [add Stefan] >>>>> >>>>> I'm not familiar with the Coverity models. Peter Maydell is the maintainer. >>>> >>>> We haven't run Coverity scans since September last year. >>> >>> What's the problem with it? May I help somehow? >> >> The container broke when libslirp was removed, and I've been procrastinating fixing it. 🙁 >> >> Paolo > > Hi! > > I see Coverity works again. Could we give this patch a try? > > Locally, I now run Coverity on master, on master with dropped model (half of my patch) and with my full patch. > > The model, that this patch drops, fixes 94 issues. The assertion I propose fixes same 94 issues and two more resource leaks. > > The model, that this patch drops, also bring 4 issues. The assertion I propose brings no new issues. > > Of course, my local setup is different from QEMU Coverity cloud run. >
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 */
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(-)