[Qemu-devel] [PATCH] coverity-model: replay data is considered trusted

[Qemu-devel] [PATCH] coverity-model: replay data is considered trusted

[Paolo Bonzini]

Replay data is not considered a possible attack vector; add a model that
does not use getc so that "tainted data" warnings are suppressed.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-model.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
index c702804f41..576f48de33 100644
--- a/scripts/coverity-model.c
+++ b/scripts/coverity-model.c
@@ -103,6 +103,19 @@ static int get_keysym(const name2keysym_t *table,
     }
 }
 
+
+/* Replay data is considered trusted.  */
+uint8_t replay_get_byte(void)
+{
+     uint8_t byte = 0;
+     if (replay_file) {
+         uint8_t c;
+         byte = c;
+     }
+     return byte;
+}
+
+
 /*
  * GLib memory allocation functions.
  *
-- 
2.17.0

Re: [Qemu-devel] [PATCH] coverity-model: replay data is considered trusted

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180514141218.28438-1-pbonzini@redhat.com
Subject: [Qemu-devel] [PATCH] coverity-model: replay data is considered trusted

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20180512004311.9299-1-richard.henderson@linaro.org -> patchew/20180512004311.9299-1-richard.henderson@linaro.org
 * [new tag]               patchew/20180514141218.28438-1-pbonzini@redhat.com -> patchew/20180514141218.28438-1-pbonzini@redhat.com
Switched to a new branch 'test'
bb44707cf0 coverity-model: replay data is considered trusted

=== OUTPUT BEGIN ===
Checking PATCH 1/1: coverity-model: replay data is considered trusted...
ERROR: suspect code indent for conditional statements (5, 9)
#25: FILE: scripts/coverity-model.c:111:
+     if (replay_file) {
+         uint8_t c;

total: 1 errors, 0 warnings, 19 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] coverity-model: replay data is considered trusted

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Replay data is not considered a possible attack vector; add a model that
> does not use getc so that "tainted data" warnings are suppressed.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/coverity-model.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
> index c702804f41..576f48de33 100644
> --- a/scripts/coverity-model.c
> +++ b/scripts/coverity-model.c
> @@ -103,6 +103,19 @@ static int get_keysym(const name2keysym_t *table,
   /* Tainting */

   typedef struct {} name2keysym_t;
   static int get_keysym(const name2keysym_t *table,
                         const char *name)
   {
       int result;
       if (result > 0) {
           __coverity_tainted_string_sanitize_content__(name);
           return result;
       } else {
           return 0;
>      }
>  }
>  
> +

Does the new model go under /* Tainting */ ?

If yes, I'd like to have just one blank line here.

> +/* Replay data is considered trusted.  */

If no, I'd like to insert one here.

> +uint8_t replay_get_byte(void)
> +{
> +     uint8_t byte = 0;
> +     if (replay_file) {
> +         uint8_t c;
> +         byte = c;
> +     }
> +     return byte;
> +}
> +
> +
>  /*
>   * GLib memory allocation functions.
>   *

Re: [Qemu-devel] [PATCH] coverity-model: replay data is considered trusted

[Paolo Bonzini]

On 15/05/2018 14:00, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Replay data is not considered a possible attack vector; add a model that
>> does not use getc so that "tainted data" warnings are suppressed.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  scripts/coverity-model.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
>> index c702804f41..576f48de33 100644
>> --- a/scripts/coverity-model.c
>> +++ b/scripts/coverity-model.c
>> @@ -103,6 +103,19 @@ static int get_keysym(const name2keysym_t *table,
>    /* Tainting */
> 
>    typedef struct {} name2keysym_t;
>    static int get_keysym(const name2keysym_t *table,
>                          const char *name)
>    {
>        int result;
>        if (result > 0) {
>            __coverity_tainted_string_sanitize_content__(name);
>            return result;
>        } else {
>            return 0;
>>      }
>>  }
>>  
>> +
> 
> Does the new model go under /* Tainting */ ?

Yes, it does.  Any chance you can do the change yourself?...

Paolo

> If yes, I'd like to have just one blank line here.
> 
>> +/* Replay data is considered trusted.  */
> 
> If no, I'd like to insert one here.
> 
>> +uint8_t replay_get_byte(void)
>> +{
>> +     uint8_t byte = 0;
>> +     if (replay_file) {
>> +         uint8_t c;
>> +         byte = c;
>> +     }
>> +     return byte;
>> +}
>> +
>> +
>>  /*
>>   * GLib memory allocation functions.
>>   *

Re: [Qemu-devel] [PATCH] coverity-model: replay data is considered trusted

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 15/05/2018 14:00, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Replay data is not considered a possible attack vector; add a model that
>>> does not use getc so that "tainted data" warnings are suppressed.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  scripts/coverity-model.c | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
>>> index c702804f41..576f48de33 100644
>>> --- a/scripts/coverity-model.c
>>> +++ b/scripts/coverity-model.c
>>> @@ -103,6 +103,19 @@ static int get_keysym(const name2keysym_t *table,
>>    /* Tainting */
>> 
>>    typedef struct {} name2keysym_t;
>>    static int get_keysym(const name2keysym_t *table,
>>                          const char *name)
>>    {
>>        int result;
>>        if (result > 0) {
>>            __coverity_tainted_string_sanitize_content__(name);
>>            return result;
>>        } else {
>>            return 0;
>>>      }
>>>  }
>>>  
>>> +
>> 
>> Does the new model go under /* Tainting */ ?
>
> Yes, it does.  Any chance you can do the change yourself?...

Gladly :)

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

Re: [Qemu-devel] [PATCH] coverity-model: replay data is considered trusted

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Replay data is not considered a possible attack vector; add a model that
> does not use getc so that "tainted data" warnings are suppressed.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/coverity-model.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
> index c702804f41..576f48de33 100644
> --- a/scripts/coverity-model.c
> +++ b/scripts/coverity-model.c
> @@ -103,6 +103,19 @@ static int get_keysym(const name2keysym_t *table,
>      }
>  }
>  
> +
> +/* Replay data is considered trusted.  */
> +uint8_t replay_get_byte(void)
> +{
> +     uint8_t byte = 0;
> +     if (replay_file) {
> +         uint8_t c;
> +         byte = c;
> +     }
> +     return byte;
> +}
> +
> +
>  /*
>   * GLib memory allocation functions.
>   *

Coverity 2018.06 chokes on this:

    $ cov-make-library -of scripts/coverity-model.xmldb scripts/coverity-model.c 
    output file: scripts/coverity-model.xmldb
    Compiling scripts/coverity-model.c with command /opt/cov-sa-2018.06/bin/cov-emit --dir /tmp/cov-armbru/930a6fb31e5f464fc1a53354b2deb66b/cov-make-library-emit -w --no_error_recovery --emit_header_functions --no_implicit_decl --preinclude /opt/cov-sa-2018.06/library/decls.h --c scripts/coverity-model.c
    "scripts/coverity-model.c", line 110: error #20: identifier "replay_file" is
              undefined
           if (replay_file) {
               ^

    Emit for file '/work/armbru/qemu/scripts/coverity-model.c' complete.
    [ERROR] 1 error detected in the compilation of "scripts/coverity-model.c".
    ERROR: cov-emit returned with code 1

Minimal fix:

    diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
    index 48b112393b..f987ce53b8 100644
    --- a/scripts/coverity-model.c
    +++ b/scripts/coverity-model.c
    @@ -106,6 +106,7 @@ static int get_keysym(const name2keysym_t *table,
     /* Replay data is considered trusted.  */
     uint8_t replay_get_byte(void)
     {
    +    void *replay_file;
          uint8_t byte = 0;
          if (replay_file) {
              uint8_t c;

Alternatively, dumb down to:

    /* Replay data is considered trusted.  */
    uint8_t replay_get_byte(void)
    {
        uint8_t byte;
        return byte;
    }

Got a preference?

Re: [Qemu-devel] [PATCH] coverity-model: replay data is considered trusted

[Paolo Bonzini]

On 26/06/2018 09:27, Markus Armbruster wrote:
> 
> Minimal fix:
> 
>     diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
>     index 48b112393b..f987ce53b8 100644
>     --- a/scripts/coverity-model.c
>     +++ b/scripts/coverity-model.c
>     @@ -106,6 +106,7 @@ static int get_keysym(const name2keysym_t *table,
>      /* Replay data is considered trusted.  */
>      uint8_t replay_get_byte(void)
>      {
>     +    void *replay_file;
>           uint8_t byte = 0;
>           if (replay_file) {
>               uint8_t c;
> 
> Alternatively, dumb down to:
> 
>     /* Replay data is considered trusted.  */
>     uint8_t replay_get_byte(void)
>     {
>         uint8_t byte;
>         return byte;
>     }

I wonder why the online service didn't complain.  I guess the dumber
version is more than enough!

Paolo

Re: [Qemu-devel] [PATCH] coverity-model: replay data is considered trusted

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 26/06/2018 09:27, Markus Armbruster wrote:
>> 
>> Minimal fix:
>> 
>>     diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
>>     index 48b112393b..f987ce53b8 100644
>>     --- a/scripts/coverity-model.c
>>     +++ b/scripts/coverity-model.c
>>     @@ -106,6 +106,7 @@ static int get_keysym(const name2keysym_t *table,
>>      /* Replay data is considered trusted.  */
>>      uint8_t replay_get_byte(void)
>>      {
>>     +    void *replay_file;
>>           uint8_t byte = 0;
>>           if (replay_file) {
>>               uint8_t c;
>> 
>> Alternatively, dumb down to:
>> 
>>     /* Replay data is considered trusted.  */
>>     uint8_t replay_get_byte(void)
>>     {
>>         uint8_t byte;
>>         return byte;
>>     }
>
> I wonder why the online service didn't complain.

Me too.  Of course, I should've run cov-make-library myself.  Next
time...

>                                                   I guess the dumber
> version is more than enough!

I'll post it as a proper patch.  Thanks!