diff mbox series

[v3,5/5] migration: Use strnlen() for fixed-size string

Message ID 20181218175122.3229-6-philmd@redhat.com
State New
Headers show
Series Fix strncpy() warnings for GCC8 new -Wstringop-truncation | expand

Commit Message

Philippe Mathieu-Daudé Dec. 18, 2018, 5:51 p.m. UTC 
GCC 8 introduced the -Wstringop-overflow, which detect buffer overflow
by string-modifying functions declared in <string.h>, such strncpy(),
used in global_state_store_running().

Since the global_state.runstate does not necessarily contains a
terminating NUL character, We had to use the QEMU_NONSTRING attribute.

The GCC manual says about the nonstring attribute:

  However, when the array is declared with the attribute the call to
  strlen is diagnosed because when the array doesn’t contain a
  NUL-terminated string the call is undefined. [...]
  In addition, calling strnlen and strndup with such arrays is safe
  provided a suitable bound is specified, and not diagnosed.

GCC indeed found an incorrect use of strlen(), because this array
is loaded by VMSTATE_BUFFER(runstate, GlobalState) then parsed
using qapi_enum_parse which does not get the buffer length.

Use strnlen() which returns sizeof(s->runstate) if the array is not
NUL-terminated.

This fixes:

    CC      migration/global_state.o
  qemu/migration/global_state.c: In function 'global_state_pre_save':
  qemu/migration/global_state.c:109:15: error: 'strlen' argument 1 declared attribute 'nonstring' [-Werror=stringop-overflow=]
       s->size = strlen((char *)s->runstate) + 1;
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~
  qemu/migration/global_state.c:24:13: note: argument 'runstate' declared here
       uint8_t runstate[100] QEMU_NONSTRING;
               ^~~~~~~~
  cc1: all warnings being treated as errors
  make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 migration/global_state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eric Blake Dec. 18, 2018, 7:33 p.m. UTC | #1 
On 12/18/18 11:51 AM, Philippe Mathieu-Daudé wrote:
> GCC 8 introduced the -Wstringop-overflow, which detect buffer overflow
> by string-modifying functions declared in <string.h>, such strncpy(),
> used in global_state_store_running().
> 
> Since the global_state.runstate does not necessarily contains a
> terminating NUL character, We had to use the QEMU_NONSTRING attribute.

s/We/we/

> 
> The GCC manual says about the nonstring attribute:
> 
>    However, when the array is declared with the attribute the call to
>    strlen is diagnosed because when the array doesn’t contain a
>    NUL-terminated string the call is undefined. [...]
>    In addition, calling strnlen and strndup with such arrays is safe
>    provided a suitable bound is specified, and not diagnosed.
> 
> GCC indeed found an incorrect use of strlen(), because this array
> is loaded by VMSTATE_BUFFER(runstate, GlobalState) then parsed
> using qapi_enum_parse which does not get the buffer length.
> 
> Use strnlen() which returns sizeof(s->runstate) if the array is not
> NUL-terminated.
> 
> This fixes:
> 
>      CC      migration/global_state.o
>    qemu/migration/global_state.c: In function 'global_state_pre_save':
>    qemu/migration/global_state.c:109:15: error: 'strlen' argument 1 declared attribute 'nonstring' [-Werror=stringop-overflow=]
>         s->size = strlen((char *)s->runstate) + 1;
>                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>    qemu/migration/global_state.c:24:13: note: argument 'runstate' declared here
>         uint8_t runstate[100] QEMU_NONSTRING;
>                 ^~~~~~~~
>    cc1: all warnings being treated as errors
>    make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   migration/global_state.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/global_state.c b/migration/global_state.c
> index 6e19333422..c19030ef62 100644
> --- a/migration/global_state.c
> +++ b/migration/global_state.c
> @@ -106,7 +106,7 @@ static int global_state_pre_save(void *opaque)
>       GlobalState *s = opaque;
>   
>       trace_migrate_global_state_pre_save((char *)s->runstate);
> -    s->size = strlen((char *)s->runstate) + 1;

The old code sets s->size to the string length + space for the NUL byte 
(by assuming that a NUL byte was present), and accidentally sets it 
beyond the s->runstate array if there was no NUL byte (our existing 
runstate names are shorter than 100 bytes, so this could only happen on 
a malicious stream).

> +    s->size = strnlen((char *)s->runstate, sizeof(s->runstate)) + 1;

The new code can still end up setting s->size beyond the array.  Is that 
intended, or would it be better to use strnlen(s->runstate, 
sizeof(s->runstate) - 1) + 1?

Also, as I argued on 4/5, why isn't this squashed in with the patch that 
marks the field NONSTRING?
Paolo Bonzini Dec. 18, 2018, 9:24 p.m. UTC | #2 
On 18/12/18 20:33, Eric Blake wrote:
>> diff --git a/migration/global_state.c b/migration/global_state.c
>> index 6e19333422..c19030ef62 100644
>> --- a/migration/global_state.c
>> +++ b/migration/global_state.c
>> @@ -106,7 +106,7 @@ static int global_state_pre_save(void *opaque)
>>       GlobalState *s = opaque;
>>         trace_migrate_global_state_pre_save((char *)s->runstate);
>> -    s->size = strlen((char *)s->runstate) + 1;
> 
> The old code sets s->size to the string length + space for the NUL byte
> (by assuming that a NUL byte was present), and accidentally sets it
> beyond the s->runstate array if there was no NUL byte (our existing
> runstate names are shorter than 100 bytes, so this could only happen on
> a malicious stream).

It cannot---this is a pre_save hook.  A possible overflow bug exists,
but it is in the call to qapi_enum_parse.

Paolo
Michael S. Tsirkin Dec. 18, 2018, 11:16 p.m. UTC | #3 
On Tue, Dec 18, 2018 at 06:51:22PM +0100, Philippe Mathieu-Daudé wrote:
> GCC 8 introduced the -Wstringop-overflow, which detect buffer overflow
> by string-modifying functions declared in <string.h>, such strncpy(),
> used in global_state_store_running().
> 
> Since the global_state.runstate does not necessarily contains a
> terminating NUL character, We had to use the QEMU_NONSTRING attribute.
> 
> The GCC manual says about the nonstring attribute:
> 
>   However, when the array is declared with the attribute the call to
>   strlen is diagnosed because when the array doesn’t contain a
>   NUL-terminated string the call is undefined. [...]
>   In addition, calling strnlen and strndup with such arrays is safe
>   provided a suitable bound is specified, and not diagnosed.
> 
> GCC indeed found an incorrect use of strlen(), because this array
> is loaded by VMSTATE_BUFFER(runstate, GlobalState) then parsed
> using qapi_enum_parse which does not get the buffer length.
> 
> Use strnlen() which returns sizeof(s->runstate) if the array is not
> NUL-terminated.
> 
> This fixes:
> 
>     CC      migration/global_state.o
>   qemu/migration/global_state.c: In function 'global_state_pre_save':
>   qemu/migration/global_state.c:109:15: error: 'strlen' argument 1 declared attribute 'nonstring' [-Werror=stringop-overflow=]
>        s->size = strlen((char *)s->runstate) + 1;
>                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>   qemu/migration/global_state.c:24:13: note: argument 'runstate' declared here
>        uint8_t runstate[100] QEMU_NONSTRING;
>                ^~~~~~~~
>   cc1: all warnings being treated as errors
>   make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  migration/global_state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/global_state.c b/migration/global_state.c
> index 6e19333422..c19030ef62 100644
> --- a/migration/global_state.c
> +++ b/migration/global_state.c
> @@ -106,7 +106,7 @@ static int global_state_pre_save(void *opaque)
>      GlobalState *s = opaque;
>  
>      trace_migrate_global_state_pre_save((char *)s->runstate);
> -    s->size = strlen((char *)s->runstate) + 1;
> +    s->size = strnlen((char *)s->runstate, sizeof(s->runstate)) + 1;
>  
>      return 0;

I don't think this works correctly if strnlen returns
sizeof(s->runstate). Which never happens so we probably should
jus add

	assert(e->size is <= sizeof(s->runstate));

But also I think this is not enough, there's a problem in post-load
in the call to qapi_enum_parse. You probably want to force
the last character to 0 there.


>  }
> -- 
> 2.17.2
Philippe Mathieu-Daudé Dec. 19, 2018, 9:24 a.m. UTC | #4 
Le mer. 19 déc. 2018 00:16, Michael S. Tsirkin <mst@redhat.com> a écrit :

> On Tue, Dec 18, 2018 at 06:51:22PM +0100, Philippe Mathieu-Daudé wrote:
> > GCC 8 introduced the -Wstringop-overflow, which detect buffer overflow
> > by string-modifying functions declared in <string.h>, such strncpy(),
> > used in global_state_store_running().
> >
> > Since the global_state.runstate does not necessarily contains a
> > terminating NUL character, We had to use the QEMU_NONSTRING attribute.
> >
> > The GCC manual says about the nonstring attribute:
> >
> >   However, when the array is declared with the attribute the call to
> >   strlen is diagnosed because when the array doesn’t contain a
> >   NUL-terminated string the call is undefined. [...]
> >   In addition, calling strnlen and strndup with such arrays is safe
> >   provided a suitable bound is specified, and not diagnosed.
> >
> > GCC indeed found an incorrect use of strlen(), because this array
> > is loaded by VMSTATE_BUFFER(runstate, GlobalState) then parsed
> > using qapi_enum_parse which does not get the buffer length.
> >
> > Use strnlen() which returns sizeof(s->runstate) if the array is not
> > NUL-terminated.
> >
> > This fixes:
> >
> >     CC      migration/global_state.o
> >   qemu/migration/global_state.c: In function 'global_state_pre_save':
> >   qemu/migration/global_state.c:109:15: error: 'strlen' argument 1
> declared attribute 'nonstring' [-Werror=stringop-overflow=]
> >        s->size = strlen((char *)s->runstate) + 1;
> >                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   qemu/migration/global_state.c:24:13: note: argument 'runstate'
> declared here
> >        uint8_t runstate[100] QEMU_NONSTRING;
> >                ^~~~~~~~
> >   cc1: all warnings being treated as errors
> >   make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >  migration/global_state.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/migration/global_state.c b/migration/global_state.c
> > index 6e19333422..c19030ef62 100644
> > --- a/migration/global_state.c
> > +++ b/migration/global_state.c
> > @@ -106,7 +106,7 @@ static int global_state_pre_save(void *opaque)
> >      GlobalState *s = opaque;
> >
> >      trace_migrate_global_state_pre_save((char *)s->runstate);
> > -    s->size = strlen((char *)s->runstate) + 1;
> > +    s->size = strnlen((char *)s->runstate, sizeof(s->runstate)) + 1;
> >
> >      return 0;
>
> I don't think this works correctly if strnlen returns
> sizeof(s->runstate). Which never happens so we probably should
> jus add
>
>         assert(e->size is <= sizeof(s->runstate));
>

OK I'll resend Marc-André previous patch.


> But also I think this is not enough, there's a problem in post-load
> in the call to qapi_enum_parse. You probably want to force
> the last character to 0 there.
>

OK, I'll have a look there.


>
> >  }
> > --
> > 2.17.2
>
diff mbox series

Patch

diff --git a/migration/global_state.c b/migration/global_state.c
index 6e19333422..c19030ef62 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -106,7 +106,7 @@  static int global_state_pre_save(void *opaque)
     GlobalState *s = opaque;
 
     trace_migrate_global_state_pre_save((char *)s->runstate);
-    s->size = strlen((char *)s->runstate) + 1;
+    s->size = strnlen((char *)s->runstate, sizeof(s->runstate)) + 1;
 
     return 0;
 }