On 06.10.23 01:53, Paolo Bonzini wrote:
On Thu, Oct 5, 2023 at 4:04 PM Vladimir Sementsov-Ogievskiy
<vsement...@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
--
Best regards,
Vladimir
