| Message ID | 20231005140326.332830-1-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] coverity: physmem: use simple assertions instead of modelling | expand |
On Thu, Oct 5, 2023 at 4:04 PM Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: > + /* > + * 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)); I'll queue it shortly, but perhaps you can try if assert(l <= len) is enough? Alternatively I can try applying the patch on top of the tree that we test with, and see how things go. Paolo > val = ldn_he_p(buf, l); > result |= memory_region_dispatch_write(mr, addr1, val, > size_memop(l), attrs); > @@ -2784,6 +2793,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 */ > -- > 2.34.1 >
On 06.10.23 01:53, Paolo Bonzini wrote: > On Thu, Oct 5, 2023 at 4:04 PM Vladimir Sementsov-Ogievskiy > <vsementsov@yandex-team.ru> wrote: >> + /* >> + * 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)); > > I'll queue it shortly, but perhaps you can try if assert(l <= len) is enough? > > Alternatively I can try applying the patch on top of the tree that we > test with, and see how things go. > I've now made 4 runs: master: patched = master + this patch l_len = master + this patch, but reduce assertion to assert(l <= len) no_assert = master + this patch, but drop assertion at all. (actully, just drop the modelling) results: +---------------+--------+-------+-----------+---------+ | | master | l_len | no_assert | patched | +---------------+--------+-------+-----------+---------+ | total | 673 | 757 | 757 | 665 | +---------------+--------+-------+-----------+---------+ | OVERRUN | 28 | 116 | 116 | 27 | +---------------+--------+-------+-----------+---------+ | RESOURCE_LEAK | 85 | 85 | 85 | 82 | +---------------+--------+-------+-----------+---------+ | UNINIT | 52 | 48 | 48 | 48 | +---------------+--------+-------+-----------+---------+ Command to generate results: DIR=../cov-master; rm -rf $DIR; git clean -fxd && ./configure --target-list=x86_64-softmmu --enable-debug --disable-docs --disable-xen --extra-cflags='-fno-lto' --cc=gcc --host-cc=gcc --cxx=g++ && cov-build --dir $DIR make -j20 && cov-make-library --output-file $DIR/qemu-model scripts/coverity-scan/model.c && cov-analyze --dir $DIR --model-file $DIR/qemu-model (then, change code, change DIR and rerun, and so on) So, assert(l <= len) doesn't help :(. Looking at first OVERRUN problem, that we have only in l_len and no_assert, in win_dump.c: 202 static void check_kdbg(WinDumpHeader *h, bool x64, Error **errp) 203 { 204 const char OwnerTag[] = "KDBG"; 205 char read_OwnerTag[4]; 206 uint64_t KdDebuggerDataBlock = WIN_DUMP_FIELD(KdDebuggerDataBlock); 207 bool try_fallback = true; 208 209 try_again: (1) Event cond_false: Condition "0 /* !(sizeof (cpus.tqh_first) <= 8) */", taking false branch. (2) Event loop_end: Reached end of loop. (3) Event overrun-buffer-val: Overrunning array "read_OwnerTag" of 4 bytes by passing it to a function which accesses it at byte offset 7. 210 if (cpu_memory_rw_debug(first_cpu, 211 KdDebuggerDataBlock + KDBG_OWNER_TAG_OFFSET, 212 (uint8_t *)&read_OwnerTag, sizeof(read_OwnerTag), 0)) { 213 error_setg(errp, "win-dump: failed to read OwnerTag"); 214 return; 215 } So, the problem is that on the code path Coverity knows exact bound (like 4), but unsure about len variable. > >> val = ldn_he_p(buf, l); >> result |= memory_region_dispatch_write(mr, addr1, val, >> size_memop(l), attrs); >> @@ -2784,6 +2793,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 */ >> -- >> 2.34.1 >> >
On 06.10.23 13:27, Vladimir Sementsov-Ogievskiy wrote: > On 06.10.23 01:53, Paolo Bonzini wrote: >> On Thu, Oct 5, 2023 at 4:04 PM Vladimir Sementsov-Ogievskiy >> <vsementsov@yandex-team.ru> wrote: >>> + /* >>> + * 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)); >> >> I'll queue it shortly, but perhaps you can try if assert(l <= len) is enough? >> >> Alternatively I can try applying the patch on top of the tree that we >> test with, and see how things go. >> > > I've now made 4 runs: > > master: I wanted to write: master: 2f3913f4b2ad74baeb5a6f1d36efbd9ecdf1057d > patched = master + this patch > l_len = master + this patch, but reduce assertion to assert(l <= len) also, cov-build version: cov-build 2023.3.2 (build 865d3107dd p-2023.3-push-63)
ping) Is it queued? On 06.10.23 01:53, Paolo Bonzini wrote: > On Thu, Oct 5, 2023 at 4:04 PM Vladimir Sementsov-Ogievskiy > <vsementsov@yandex-team.ru> wrote: >> + /* >> + * 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)); > > I'll queue it shortly, but perhaps you can try if assert(l <= len) is enough? > > Alternatively I can try applying the patch on top of the tree that we > test with, and see how things go. > > Paolo > >> val = ldn_he_p(buf, l); >> result |= memory_region_dispatch_write(mr, addr1, val, >> size_memop(l), attrs); >> @@ -2784,6 +2793,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 */ >> -- >> 2.34.1 >> >
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 309653c722..03e2f9bee6 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -2714,6 +2714,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); @@ -2784,6 +2793,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> Acked-by: David Hildenbrand <david@redhat.com> --- v2: add a-b by Devid scripts/coverity-scan/model.c | 88 ----------------------------------- softmmu/physmem.c | 18 +++++++ 2 files changed, 18 insertions(+), 88 deletions(-)