[Qemu-devel] [PATCH v2] coverity-model: model address_space_read/write

[Qemu-devel] [PATCH v2] coverity-model: model address_space_read/write

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH v2] coverity-model: model address_space_read/write

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2] coverity-model: model address_space_read/write

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v2] coverity-model: model address_space_read/write

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2] coverity-model: model address_space_read/write

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Expect a pull request shortly.