Commit eb7eeb8 ("memory: split address_space_read and
address_space_write", 2015-12-17) made address_space_rw
dispatch to one of address_space_read or address_space_write,
rather than vice versa.
For callers of address_space_read and address_space_write this
causes false positive defects when Coverity sees a length-8 write in
address_space_read and a length-4 (e.g. int*) buffer to read into.
As long as the size of the buffer is okay, this is a false positive.
Reflect the code change into the model.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
scripts/coverity-model.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
index ee5bf9d..c702804 100644
--- a/scripts/coverity-model.c
+++ b/scripts/coverity-model.c
@@ -67,18 +67,27 @@ static void __bufread(uint8_t *buf, ssize_t len)
int last = buf[len-1];
}
-MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
- uint8_t *buf, int len, bool is_write)
+MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
+ MemTxAttrs attrs,
+ uint8_t *buf, int len)
{
MemTxResult result;
-
// TODO: investigate impact of treating reads as producing
// tainted data, with __coverity_tainted_data_argument__(buf).
- if (is_write) __bufread(buf, len); else __bufwrite(buf, len);
+ __bufwrite(buf, len);
+ return result;
+}
+MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
+ MemTxAttrs attrs,
+ const uint8_t *buf, int len)
+{
+ MemTxResult result;
+ __bufread(buf, len);
return result;
}
+
/* Tainting */
typedef struct {} name2keysym_t;
--
2.9.3
On 03/15/2017 03:16 AM, Paolo Bonzini wrote: > Commit eb7eeb8 ("memory: split address_space_read and > address_space_write", 2015-12-17) made address_space_rw > dispatch to one of address_space_read or address_space_write, > rather than vice versa. > > For callers of address_space_read and address_space_write this > causes false positive defects when Coverity sees a length-8 write in > address_space_read and a length-4 (e.g. int*) buffer to read into. > As long as the size of the buffer is okay, this is a false positive. > > Reflect the code change into the model. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > scripts/coverity-model.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > -MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, > - uint8_t *buf, int len, bool is_write) > +MemTxResult address_space_read(AddressSpace *as, hwaddr addr, > + MemTxAttrs attrs, > + uint8_t *buf, int len) > { > MemTxResult result; > - > // TODO: investigate impact of treating reads as producing > // tainted data, with __coverity_tainted_data_argument__(buf). > - if (is_write) __bufread(buf, len); else __bufwrite(buf, len); Old code did __bufread for reads, > + __bufwrite(buf, len); but the new does __bufwrite. > + return result; > +} > > +MemTxResult address_space_write(AddressSpace *as, hwaddr addr, > + MemTxAttrs attrs, > + const uint8_t *buf, int len) > +{ > + MemTxResult result; > + __bufread(buf, len); And __bufread for writes. Did you get this backwards? > return result; > } > > + > /* Tainting */ > > typedef struct {} name2keysym_t; > -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 15 March 2017 at 11:55, Eric Blake <eblake@redhat.com> wrote: > On 03/15/2017 03:16 AM, Paolo Bonzini wrote: >> -MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, >> - uint8_t *buf, int len, bool is_write) >> +MemTxResult address_space_read(AddressSpace *as, hwaddr addr, >> + MemTxAttrs attrs, >> + uint8_t *buf, int len) >> { >> MemTxResult result; >> - >> // TODO: investigate impact of treating reads as producing >> // tainted data, with __coverity_tainted_data_argument__(buf). >> - if (is_write) __bufread(buf, len); else __bufwrite(buf, len); > > Old code did __bufread for reads, Eh? for a read is_write is false, and we use the else clause, which is __bufwrite... thanks -- PMM
On 03/15/2017 06:58 AM, Peter Maydell wrote: > On 15 March 2017 at 11:55, Eric Blake <eblake@redhat.com> wrote: >> On 03/15/2017 03:16 AM, Paolo Bonzini wrote: >>> -MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, >>> - uint8_t *buf, int len, bool is_write) >>> +MemTxResult address_space_read(AddressSpace *as, hwaddr addr, >>> + MemTxAttrs attrs, >>> + uint8_t *buf, int len) >>> { >>> MemTxResult result; >>> - >>> // TODO: investigate impact of treating reads as producing >>> // tainted data, with __coverity_tainted_data_argument__(buf). >>> - if (is_write) __bufread(buf, len); else __bufwrite(buf, len); >> >> Old code did __bufread for reads, > > Eh? for a read is_write is false, and we use the else clause, > which is __bufwrite... Maybe I shouldn't send emails when I've just woken up? It threw me that we have a function named 'read' relying on coverity's 'write' - but you're correct that it has always been that way, and thinking about it more, what is really happening is: our function named 'read' is emulating getting data from hardware (the 'read' portion) and copying it into the buffer (the 'write' portion); the Coverity model needs to know about the effects to the buffer, but could care less about the hardware emulation side. Okay, you've straightened me out, so I can give: Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
© 2016 - 2024 Red Hat, Inc.