[PATCH 0/2] gcc 9.2 strncpy(3) warnings fixes

[PATCH 0/2] gcc 9.2 strncpy(3) warnings fixes

[Stefan Hajnoczi]

I hit two strncpy(3) compiler warnings with gcc 9.2 and sanitizers enabled
recently.  Use QEMU's pstrcpy() function instead - it always NUL-terminates and
therefore eliminates the concerns behind this compiler warning.

Stefan Hajnoczi (2):
  migration: avoid suspicious strncpy() use
  hw/rdma: avoid suspicious strncpy() use

 hw/rdma/vmw/pvrdma_dev_ring.c | 4 ++--
 migration/global_state.c      | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.24.1

[PATCH 1/2] migration: avoid suspicious strncpy() use

[Stefan Hajnoczi]

gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled
reports the following error:

    CC      migration/global_state.o
  In file included from /usr/include/string.h:495,
                   from /home/stefanha/qemu/include/qemu/osdep.h:101,
                   from migration/global_state.c:13:
  In function ‘strncpy’,
      inlined from ‘global_state_store_running’ at migration/global_state.c:47:5:
  /usr/include/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ specified bound 100 equals destination size [-Werror=stringop-truncation]
    106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Use pstrcpy() instead of strncpy().  It is guaranteed to NUL-terminate
strings.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 migration/global_state.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/global_state.c b/migration/global_state.c
index 25311479a4..cbe07f21a8 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -44,8 +44,8 @@ void global_state_store_running(void)
 {
     const char *state = RunState_str(RUN_STATE_RUNNING);
     assert(strlen(state) < sizeof(global_state.runstate));
-    strncpy((char *)global_state.runstate,
-           state, sizeof(global_state.runstate));
+    pstrcpy((char *)global_state.runstate,
+            sizeof(global_state.runstate), state);
 }
 
 bool global_state_received(void)
-- 
2.24.1

[PATCH 2/2] hw/rdma: avoid suspicious strncpy() use

[Stefan Hajnoczi]

gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled
reports the following error:

  CC      x86_64-softmmu/hw/rdma/vmw/pvrdma_dev_ring.o
In file included from /usr/include/string.h:495,
                 from include/qemu/osdep.h:101,
                 from hw/rdma/vmw/pvrdma_dev_ring.c:16:
In function ‘strncpy’,
    inlined from ‘pvrdma_ring_init’ at hw/rdma/vmw/pvrdma_dev_ring.c:33:5:
/usr/include/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ specified bound 32 equals destination size [-Werror=stringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Use pstrcpy() instead of strncpy().  It is guaranteed to NUL-terminate
strings.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/rdma/vmw/pvrdma_dev_ring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/rdma/vmw/pvrdma_dev_ring.c b/hw/rdma/vmw/pvrdma_dev_ring.c
index d7bc7f5ccc..74b8fa834c 100644
--- a/hw/rdma/vmw/pvrdma_dev_ring.c
+++ b/hw/rdma/vmw/pvrdma_dev_ring.c
@@ -14,6 +14,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "hw/pci/pci.h"
 #include "cpu.h"
 
@@ -30,8 +31,7 @@ int pvrdma_ring_init(PvrdmaRing *ring, const char *name, PCIDevice *dev,
     int i;
     int rc = 0;
 
-    strncpy(ring->name, name, MAX_RING_NAME_SZ);
-    ring->name[MAX_RING_NAME_SZ - 1] = 0;
+    pstrcpy(ring->name, MAX_RING_NAME_SZ, name);
     ring->dev = dev;
     ring->ring_state = ring_state;
     ring->max_elems = max_elems;
-- 
2.24.1

Re: [PATCH 1/2] migration: avoid suspicious strncpy() use

[Juan Quintela]

Stefan Hajnoczi <stefanha@redhat.com> wrote:
> gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled
> reports the following error:
>
>     CC      migration/global_state.o
>   In file included from /usr/include/string.h:495,
>                    from /home/stefanha/qemu/include/qemu/osdep.h:101,
>                    from migration/global_state.c:13:
>   In function ‘strncpy’,
>       inlined from ‘global_state_store_running’ at migration/global_state.c:47:5:
>   /usr/include/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ specified bound 100 equals destination size [-Werror=stringop-truncation]
>     106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
>         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Use pstrcpy() instead of strncpy().  It is guaranteed to NUL-terminate
> strings.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

This is the film where:
- one put a size 100
- none of the current elements has a size of 20
- for extra security one put one assert to make sure tha there are
  space.

And compiler still find a reason for complaining O:-)

Re: [PATCH 2/2] hw/rdma: avoid suspicious strncpy() use

[Juan Quintela]

Stefan Hajnoczi <stefanha@redhat.com> wrote:
> gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled
> reports the following error:
>
>   CC      x86_64-softmmu/hw/rdma/vmw/pvrdma_dev_ring.o
> In file included from /usr/include/string.h:495,
>                  from include/qemu/osdep.h:101,
>                  from hw/rdma/vmw/pvrdma_dev_ring.c:16:
> In function ‘strncpy’,
>     inlined from ‘pvrdma_ring_init’ at hw/rdma/vmw/pvrdma_dev_ring.c:33:5:
> /usr/include/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ specified bound 32 equals destination size [-Werror=stringop-truncation]
>   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Use pstrcpy() instead of strncpy().  It is guaranteed to NUL-terminate
> strings.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH 1/2] migration: avoid suspicious strncpy() use

[Philippe Mathieu-Daudé]

On 3/16/20 5:07 PM, Stefan Hajnoczi wrote:
> gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled
> reports the following error:
> 
>      CC      migration/global_state.o
>    In file included from /usr/include/string.h:495,
>                     from /home/stefanha/qemu/include/qemu/osdep.h:101,
>                     from migration/global_state.c:13:
>    In function ‘strncpy’,
>        inlined from ‘global_state_store_running’ at migration/global_state.c:47:5:
>    /usr/include/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ specified bound 100 equals destination size [-Werror=stringop-truncation]
>      106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
>          |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Use pstrcpy() instead of strncpy().  It is guaranteed to NUL-terminate
> strings.

There was a long discussion 1 year ago with it, and Eric suggested to 
use strpadcpy after the assert() and I sent this patch:
https://www.mail-archive.com/qemu-block@nongnu.org/msg44925.html
Not sure what's best.

> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   migration/global_state.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/global_state.c b/migration/global_state.c
> index 25311479a4..cbe07f21a8 100644
> --- a/migration/global_state.c
> +++ b/migration/global_state.c
> @@ -44,8 +44,8 @@ void global_state_store_running(void)
>   {
>       const char *state = RunState_str(RUN_STATE_RUNNING);
>       assert(strlen(state) < sizeof(global_state.runstate));
> -    strncpy((char *)global_state.runstate,
> -           state, sizeof(global_state.runstate));
> +    pstrcpy((char *)global_state.runstate,
> +            sizeof(global_state.runstate), state);
>   }
>   
>   bool global_state_received(void)
> 

Re: [PATCH 1/2] migration: avoid suspicious strncpy() use

[Eric Blake]

On 3/16/20 1:09 PM, Philippe Mathieu-Daudé wrote:
> On 3/16/20 5:07 PM, Stefan Hajnoczi wrote:
>> gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled
>> reports the following error:
>>
>>      CC      migration/global_state.o
>>    In file included from /usr/include/string.h:495,
>>                     from /home/stefanha/qemu/include/qemu/osdep.h:101,
>>                     from migration/global_state.c:13:
>>    In function ‘strncpy’,
>>        inlined from ‘global_state_store_running’ at 
>> migration/global_state.c:47:5:
>>    /usr/include/bits/string_fortified.h:106:10: error: 
>> ‘__builtin_strncpy’ specified bound 100 equals destination size 
>> [-Werror=stringop-truncation]
>>      106 |   return __builtin___strncpy_chk (__dest, __src, __len, 
>> __bos (__dest));
>>          |          
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Use pstrcpy() instead of strncpy().  It is guaranteed to NUL-terminate
>> strings.
> 
> There was a long discussion 1 year ago with it, and Eric suggested to 
> use strpadcpy after the assert() and I sent this patch:
> https://www.mail-archive.com/qemu-block@nongnu.org/msg44925.html
> Not sure what's best.

strncpy() pads the tail, guaranteeing that for our fixed-size buffer, we 
guarantee the contents of all bytes in the buffer.  pstrcpy() does not 
(but pstrcpy() can be followed up with a memset() to emulate the 
remaining effects of strncpy() - at which point you have reimplemented 
strpadcpy).

> 
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   migration/global_state.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/global_state.c b/migration/global_state.c
>> index 25311479a4..cbe07f21a8 100644
>> --- a/migration/global_state.c
>> +++ b/migration/global_state.c
>> @@ -44,8 +44,8 @@ void global_state_store_running(void)
>>   {
>>       const char *state = RunState_str(RUN_STATE_RUNNING);
>>       assert(strlen(state) < sizeof(global_state.runstate));
>> -    strncpy((char *)global_state.runstate,
>> -           state, sizeof(global_state.runstate));
>> +    pstrcpy((char *)global_state.runstate,
>> +            sizeof(global_state.runstate), state);

Can we guarantee that the padding bytes have been previously set to 0, 
or do we need to go the extra mile with a memset() or strpadcpy() to 
guarantee that we have set the entire buffer?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/2] migration: avoid suspicious strncpy() use

[Stefan Hajnoczi]

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

On Mon, Mar 16, 2020 at 01:15:35PM -0500, Eric Blake wrote:
> On 3/16/20 1:09 PM, Philippe Mathieu-Daudé wrote:
> > On 3/16/20 5:07 PM, Stefan Hajnoczi wrote:
> > > gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled
> > > reports the following error:
> > > 
> > >      CC      migration/global_state.o
> > >    In file included from /usr/include/string.h:495,
> > >                     from /home/stefanha/qemu/include/qemu/osdep.h:101,
> > >                     from migration/global_state.c:13:
> > >    In function ‘strncpy’,
> > >        inlined from ‘global_state_store_running’ at
> > > migration/global_state.c:47:5:
> > >    /usr/include/bits/string_fortified.h:106:10: error:
> > > ‘__builtin_strncpy’ specified bound 100 equals destination size
> > > [-Werror=stringop-truncation]
> > >      106 |   return __builtin___strncpy_chk (__dest, __src, __len,
> > > __bos (__dest));
> > >          |
> > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > 
> > > Use pstrcpy() instead of strncpy().  It is guaranteed to NUL-terminate
> > > strings.
> > 
> > There was a long discussion 1 year ago with it, and Eric suggested to
> > use strpadcpy after the assert() and I sent this patch:
> > https://www.mail-archive.com/qemu-block@nongnu.org/msg44925.html
> > Not sure what's best.
> 
> strncpy() pads the tail, guaranteeing that for our fixed-size buffer, we
> guarantee the contents of all bytes in the buffer.  pstrcpy() does not (but
> pstrcpy() can be followed up with a memset() to emulate the remaining
> effects of strncpy() - at which point you have reimplemented strpadcpy).
> 
> > 
> > > 
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >   migration/global_state.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/migration/global_state.c b/migration/global_state.c
> > > index 25311479a4..cbe07f21a8 100644
> > > --- a/migration/global_state.c
> > > +++ b/migration/global_state.c
> > > @@ -44,8 +44,8 @@ void global_state_store_running(void)
> > >   {
> > >       const char *state = RunState_str(RUN_STATE_RUNNING);
> > >       assert(strlen(state) < sizeof(global_state.runstate));
> > > -    strncpy((char *)global_state.runstate,
> > > -           state, sizeof(global_state.runstate));
> > > +    pstrcpy((char *)global_state.runstate,
> > > +            sizeof(global_state.runstate), state);
> 
> Can we guarantee that the padding bytes have been previously set to 0, or do
> we need to go the extra mile with a memset() or strpadcpy() to guarantee
> that we have set the entire buffer?

I don't understand GlobalState:

1. You ask if runstate[] must be padded with NULs but neither
   global_state_store() nor register_global_state() do that.  Is it
   really necessary to pad runstate[]?

   If yes, is it safe for global_state_store() and
   register_global_state() to not pad runstate[]?

   If we decide the pad runstate[] to prevent information leaks to the
   migration destination then I think it should be done in the pre-save
   function so that it's guaranteed to happen no matter which of the 3
   functions that write runstate[] has been called.

2. There is a GlobalState::size field that is only written and then
   migrated but never read from what I can tell.  :?

Juan: Please clarify the above.  Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] migration: avoid suspicious strncpy() use

[Philippe Mathieu-Daudé]

On 3/17/20 10:52 AM, Stefan Hajnoczi wrote:
> On Mon, Mar 16, 2020 at 01:15:35PM -0500, Eric Blake wrote:
>> On 3/16/20 1:09 PM, Philippe Mathieu-Daudé wrote:
>>> On 3/16/20 5:07 PM, Stefan Hajnoczi wrote:
>>>> gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled
>>>> reports the following error:
>>>>
>>>>       CC      migration/global_state.o
>>>>     In file included from /usr/include/string.h:495,
>>>>                      from /home/stefanha/qemu/include/qemu/osdep.h:101,
>>>>                      from migration/global_state.c:13:
>>>>     In function ‘strncpy’,
>>>>         inlined from ‘global_state_store_running’ at
>>>> migration/global_state.c:47:5:
>>>>     /usr/include/bits/string_fortified.h:106:10: error:
>>>> ‘__builtin_strncpy’ specified bound 100 equals destination size
>>>> [-Werror=stringop-truncation]
>>>>       106 |   return __builtin___strncpy_chk (__dest, __src, __len,
>>>> __bos (__dest));
>>>>           |
>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> Use pstrcpy() instead of strncpy().  It is guaranteed to NUL-terminate
>>>> strings.
>>>
>>> There was a long discussion 1 year ago with it, and Eric suggested to
>>> use strpadcpy after the assert() and I sent this patch:
>>> https://www.mail-archive.com/qemu-block@nongnu.org/msg44925.html
>>> Not sure what's best.
>>
>> strncpy() pads the tail, guaranteeing that for our fixed-size buffer, we
>> guarantee the contents of all bytes in the buffer.  pstrcpy() does not (but
>> pstrcpy() can be followed up with a memset() to emulate the remaining
>> effects of strncpy() - at which point you have reimplemented strpadcpy).
>>
>>>
>>>>
>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>>    migration/global_state.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/migration/global_state.c b/migration/global_state.c
>>>> index 25311479a4..cbe07f21a8 100644
>>>> --- a/migration/global_state.c
>>>> +++ b/migration/global_state.c
>>>> @@ -44,8 +44,8 @@ void global_state_store_running(void)
>>>>    {
>>>>        const char *state = RunState_str(RUN_STATE_RUNNING);
>>>>        assert(strlen(state) < sizeof(global_state.runstate));
>>>> -    strncpy((char *)global_state.runstate,
>>>> -           state, sizeof(global_state.runstate));
>>>> +    pstrcpy((char *)global_state.runstate,
>>>> +            sizeof(global_state.runstate), state);
>>
>> Can we guarantee that the padding bytes have been previously set to 0, or do
>> we need to go the extra mile with a memset() or strpadcpy() to guarantee
>> that we have set the entire buffer?
> 
> I don't understand GlobalState:
> 
> 1. You ask if runstate[] must be padded with NULs but neither
>     global_state_store() nor register_global_state() do that.  Is it
>     really necessary to pad runstate[]?
> 
>     If yes, is it safe for global_state_store() and
>     register_global_state() to not pad runstate[]?
> 
>     If we decide the pad runstate[] to prevent information leaks to the
>     migration destination then I think it should be done in the pre-save
>     function so that it's guaranteed to happen no matter which of the 3
>     functions that write runstate[] has been called.
> 
> 2. There is a GlobalState::size field that is only written and then
>     migrated but never read from what I can tell.  :?

Isn't it in process_incoming_migration_bh()?

     /* If global state section was not received or we are in running
        state, we need to obey autostart. Any other state is set with
        runstate_set. */
     ...
     runstate_set(global_state_get_runstate());

> 
> Juan: Please clarify the above.  Thanks!
> 

Re: [PATCH 1/2] migration: avoid suspicious strncpy() use

[Juan Quintela]

Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Mon, Mar 16, 2020 at 01:15:35PM -0500, Eric Blake wrote:
>> On 3/16/20 1:09 PM, Philippe Mathieu-Daudé wrote:
>> > On 3/16/20 5:07 PM, Stefan Hajnoczi wrote:
>> 
>> > 
>> > > 
>> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> > > ---
>> > >   migration/global_state.c | 4 ++--
>> > >   1 file changed, 2 insertions(+), 2 deletions(-)
>> > > 
>> > > diff --git a/migration/global_state.c b/migration/global_state.c
>> > > index 25311479a4..cbe07f21a8 100644
>> > > --- a/migration/global_state.c
>> > > +++ b/migration/global_state.c
>> > > @@ -44,8 +44,8 @@ void global_state_store_running(void)
>> > >   {
>> > >       const char *state = RunState_str(RUN_STATE_RUNNING);
>> > >       assert(strlen(state) < sizeof(global_state.runstate));
>> > > -    strncpy((char *)global_state.runstate,
>> > > -           state, sizeof(global_state.runstate));
>> > > +    pstrcpy((char *)global_state.runstate,
>> > > +            sizeof(global_state.runstate), state);
>> 
>> Can we guarantee that the padding bytes have been previously set to 0, or do
>> we need to go the extra mile with a memset() or strpadcpy() to guarantee
>> that we have set the entire buffer?
>
> I don't understand GlobalState:

Welcome to the club O:-)

And I thought that with the reviewed-by I had finished here O:-)

> 1. You ask if runstate[] must be padded with NULs but neither
>    global_state_store() nor register_global_state() do that.  Is it
>    really necessary to pad runstate[]?
>
>    If yes, is it safe for global_state_store() and
>    register_global_state() to not pad runstate[]?

it is an error.  All should be padded.

>    If we decide the pad runstate[] to prevent information leaks to the
>    migration destination then I think it should be done in the pre-save
>    function so that it's guaranteed to happen no matter which of the 3
>    functions that write runstate[] has been called.

Ok.
Taking a look at this.

> 2. There is a GlobalState::size field that is only written and then
>    migrated but never read from what I can tell.  :?

Grrr.  It should be used, but it is not :-(

What we have here:
- A static buffer

    uint8_t runstate[100];

That is partially filled.
size: is the size of that buffer that is filled.

But, as we are using

        VMSTATE_BUFFER(runstate, GlobalState),

We are always sending/receiving the whole buffer.  THat is why we have
trouble with padding.  What should we being doing?

Sending just the size, the filled bytes, and making sure that there is
enough space on destination.

But we aren't donig that.  And at this point, I think that I am only
going to fix the 1st part (always zero pad everything sent).

For fixing the other bit, I need to do an incompatible change.

> Juan: Please clarify the above.  Thanks!

Thanks a lot.

Later, Juan.

PD: Why is it done this way?
    Because at the moment, the problem was that qcow2 (as a system, not
    as a device) didn't have a place where to plug pending requests.  So
    I created this section that always exist, and anything that has not
    a device associated can hang a subsection here.  Once that I created
    it, nobody used it.
    And now, just seing what you are telling, I didn't even used the
    right approach.

Re: [PATCH 1/2] migration: avoid suspicious strncpy() use

[Stefan Hajnoczi]

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

On Tue, Mar 17, 2020 at 11:35:14AM +0100, Juan Quintela wrote:
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Mon, Mar 16, 2020 at 01:15:35PM -0500, Eric Blake wrote:
> >> On 3/16/20 1:09 PM, Philippe Mathieu-Daudé wrote:
> >> > On 3/16/20 5:07 PM, Stefan Hajnoczi wrote:
> >> 
> >> > 
> >> > > 
> >> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> > > ---
> >> > >   migration/global_state.c | 4 ++--
> >> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> >> > > 
> >> > > diff --git a/migration/global_state.c b/migration/global_state.c
> >> > > index 25311479a4..cbe07f21a8 100644
> >> > > --- a/migration/global_state.c
> >> > > +++ b/migration/global_state.c
> >> > > @@ -44,8 +44,8 @@ void global_state_store_running(void)
> >> > >   {
> >> > >       const char *state = RunState_str(RUN_STATE_RUNNING);
> >> > >       assert(strlen(state) < sizeof(global_state.runstate));
> >> > > -    strncpy((char *)global_state.runstate,
> >> > > -           state, sizeof(global_state.runstate));
> >> > > +    pstrcpy((char *)global_state.runstate,
> >> > > +            sizeof(global_state.runstate), state);
> >> 
> >> Can we guarantee that the padding bytes have been previously set to 0, or do
> >> we need to go the extra mile with a memset() or strpadcpy() to guarantee
> >> that we have set the entire buffer?
> >
> > I don't understand GlobalState:
> 
> Welcome to the club O:-)
> 
> And I thought that with the reviewed-by I had finished here O:-)
> 
> > 1. You ask if runstate[] must be padded with NULs but neither
> >    global_state_store() nor register_global_state() do that.  Is it
> >    really necessary to pad runstate[]?
> >
> >    If yes, is it safe for global_state_store() and
> >    register_global_state() to not pad runstate[]?
> 
> it is an error.  All should be padded.
> 
> >    If we decide the pad runstate[] to prevent information leaks to the
> >    migration destination then I think it should be done in the pre-save
> >    function so that it's guaranteed to happen no matter which of the 3
> >    functions that write runstate[] has been called.
> 
> Ok.
> Taking a look at this.
> 
> > 2. There is a GlobalState::size field that is only written and then
> >    migrated but never read from what I can tell.  :?
> 
> Grrr.  It should be used, but it is not :-(
> 
> What we have here:
> - A static buffer
> 
>     uint8_t runstate[100];
> 
> That is partially filled.
> size: is the size of that buffer that is filled.
> 
> But, as we are using
> 
>         VMSTATE_BUFFER(runstate, GlobalState),
> 
> We are always sending/receiving the whole buffer.  THat is why we have
> trouble with padding.  What should we being doing?
> 
> Sending just the size, the filled bytes, and making sure that there is
> enough space on destination.
> 
> But we aren't donig that.  And at this point, I think that I am only
> going to fix the 1st part (always zero pad everything sent).
> 
> For fixing the other bit, I need to do an incompatible change.
> 
> > Juan: Please clarify the above.  Thanks!
> 
> Thanks a lot.
> 
> Later, Juan.
> 
> PD: Why is it done this way?
>     Because at the moment, the problem was that qcow2 (as a system, not
>     as a device) didn't have a place where to plug pending requests.  So
>     I created this section that always exist, and anything that has not
>     a device associated can hang a subsection here.  Once that I created
>     it, nobody used it.
>     And now, just seing what you are telling, I didn't even used the
>     right approach.

Great, thanks for looking into this.

Could you base your patches on top of this series?  Then you can send
them all together in a single pull request.  That way we can be sure
that padding will be added even after switching to pstrcpy() in my
patch.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] migration: avoid suspicious strncpy() use

[Juan Quintela]

Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Tue, Mar 17, 2020 at 11:35:14AM +0100, Juan Quintela wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> > On Mon, Mar 16, 2020 at 01:15:35PM -0500, Eric Blake wrote:
>> >> On 3/16/20 1:09 PM, Philippe Mathieu-Daudé wrote:
>> >> > On 3/16/20 5:07 PM, Stefan Hajnoczi wrote:
>> >> 
>> >> > 
>> >> > > 
>> >> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> >> > > ---
>> >> > >   migration/global_state.c | 4 ++--
>> >> > >   1 file changed, 2 insertions(+), 2 deletions(-)
>> >> > > 
>> >> > > diff --git a/migration/global_state.c b/migration/global_state.c
>> >> > > index 25311479a4..cbe07f21a8 100644
>> >> > > --- a/migration/global_state.c
>> >> > > +++ b/migration/global_state.c
>> >> > > @@ -44,8 +44,8 @@ void global_state_store_running(void)
>> >> > >   {
>> >> > >       const char *state = RunState_str(RUN_STATE_RUNNING);
>> >> > >       assert(strlen(state) < sizeof(global_state.runstate));
>> >> > > -    strncpy((char *)global_state.runstate,
>> >> > > -           state, sizeof(global_state.runstate));
>> >> > > +    pstrcpy((char *)global_state.runstate,
>> >> > > +            sizeof(global_state.runstate), state);
>> >> 
>> >> Can we guarantee that the padding bytes have been previously set to 0, or do
>> >> we need to go the extra mile with a memset() or strpadcpy() to guarantee
>> >> that we have set the entire buffer?
>> >
>> > I don't understand GlobalState:
>> 
>> Welcome to the club O:-)
>> 
>> And I thought that with the reviewed-by I had finished here O:-)
>> 
>> > 1. You ask if runstate[] must be padded with NULs but neither
>> >    global_state_store() nor register_global_state() do that.  Is it
>> >    really necessary to pad runstate[]?
>> >
>> >    If yes, is it safe for global_state_store() and
>> >    register_global_state() to not pad runstate[]?
>> 
>> it is an error.  All should be padded.
>> 
>> >    If we decide the pad runstate[] to prevent information leaks to the
>> >    migration destination then I think it should be done in the pre-save
>> >    function so that it's guaranteed to happen no matter which of the 3
>> >    functions that write runstate[] has been called.
>> 
>> Ok.
>> Taking a look at this.
>> 
>> > 2. There is a GlobalState::size field that is only written and then
>> >    migrated but never read from what I can tell.  :?
>> 
>> Grrr.  It should be used, but it is not :-(
>> 
>> What we have here:
>> - A static buffer
>> 
>>     uint8_t runstate[100];
>> 
>> That is partially filled.
>> size: is the size of that buffer that is filled.
>> 
>> But, as we are using
>> 
>>         VMSTATE_BUFFER(runstate, GlobalState),
>> 
>> We are always sending/receiving the whole buffer.  THat is why we have
>> trouble with padding.  What should we being doing?
>> 
>> Sending just the size, the filled bytes, and making sure that there is
>> enough space on destination.
>> 
>> But we aren't donig that.  And at this point, I think that I am only
>> going to fix the 1st part (always zero pad everything sent).
>> 
>> For fixing the other bit, I need to do an incompatible change.
>> 
>> > Juan: Please clarify the above.  Thanks!
>> 
>> Thanks a lot.
>> 
>> Later, Juan.
>> 
>> PD: Why is it done this way?
>>     Because at the moment, the problem was that qcow2 (as a system, not
>>     as a device) didn't have a place where to plug pending requests.  So
>>     I created this section that always exist, and anything that has not
>>     a device associated can hang a subsection here.  Once that I created
>>     it, nobody used it.
>>     And now, just seing what you are telling, I didn't even used the
>>     right approach.
>
> Great, thanks for looking into this.
>
> Could you base your patches on top of this series?  Then you can send
> them all together in a single pull request.  That way we can be sure
> that padding will be added even after switching to pstrcpy() in my
> patch.

Sure thing.

Thanks, Juan.

Re: [PATCH 2/2] hw/rdma: avoid suspicious strncpy() use

[Yuval Shaia]

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

On Mon, 16 Mar 2020 at 18:07, Stefan Hajnoczi <stefanha@redhat.com> wrote:

> gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled
> reports the following error:
>
>   CC      x86_64-softmmu/hw/rdma/vmw/pvrdma_dev_ring.o
> In file included from /usr/include/string.h:495,
>                  from include/qemu/osdep.h:101,
>                  from hw/rdma/vmw/pvrdma_dev_ring.c:16:
> In function ‘strncpy’,
>     inlined from ‘pvrdma_ring_init’ at hw/rdma/vmw/pvrdma_dev_ring.c:33:5:
> /usr/include/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’
> specified bound 32 equals destination size [-Werror=stringop-truncation]
>   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos
> (__dest));
>       |
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Use pstrcpy() instead of strncpy().  It is guaranteed to NUL-terminate
> strings.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/rdma/vmw/pvrdma_dev_ring.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/rdma/vmw/pvrdma_dev_ring.c b/hw/rdma/vmw/pvrdma_dev_ring.c
> index d7bc7f5ccc..74b8fa834c 100644
> --- a/hw/rdma/vmw/pvrdma_dev_ring.c
> +++ b/hw/rdma/vmw/pvrdma_dev_ring.c
> @@ -14,6 +14,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  #include "hw/pci/pci.h"
>  #include "cpu.h"
>
> @@ -30,8 +31,7 @@ int pvrdma_ring_init(PvrdmaRing *ring, const char *name,
> PCIDevice *dev,
>      int i;
>      int rc = 0;
>
> -    strncpy(ring->name, name, MAX_RING_NAME_SZ);
> -    ring->name[MAX_RING_NAME_SZ - 1] = 0;
> +    pstrcpy(ring->name, MAX_RING_NAME_SZ, name);
>      ring->dev = dev;
>      ring->ring_state = ring_state;
>      ring->max_elems = max_elems;
> --
> 2.24.1
>
>
Thanks,

Reviewed-by: Yuval Shaia <yuval.shaia.ml.gmail.com>

[-- Attachment #2: Type: text/html, Size: 2604 bytes --]

Re: [PATCH 2/2] hw/rdma: avoid suspicious strncpy() use

[Marcel Apfelbaum]

Hi Stefan,

On 3/20/20 1:55 PM, Yuval Shaia wrote:
>
>
> On Mon, 16 Mar 2020 at 18:07, Stefan Hajnoczi <stefanha@redhat.com 
> <mailto:stefanha@redhat.com>> wrote:
>
>     gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled
>     reports the following error:
>
>       CC      x86_64-softmmu/hw/rdma/vmw/pvrdma_dev_ring.o
>     In file included from /usr/include/string.h:495,
>                      from include/qemu/osdep.h:101,
>                      from hw/rdma/vmw/pvrdma_dev_ring.c:16:
>     In function ‘strncpy’,
>         inlined from ‘pvrdma_ring_init’ at
>     hw/rdma/vmw/pvrdma_dev_ring.c:33:5:
>     /usr/include/bits/string_fortified.h:106:10: error:
>     ‘__builtin_strncpy’ specified bound 32 equals destination size
>     [-Werror=stringop-truncation]
>       106 |   return __builtin___strncpy_chk (__dest, __src, __len,
>     __bos (__dest));
>           | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>     Use pstrcpy() instead of strncpy().  It is guaranteed to NUL-terminate
>     strings.
>
>     Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com
>     <mailto:stefanha@redhat.com>>
>     ---
>      hw/rdma/vmw/pvrdma_dev_ring.c | 4 ++--
>      1 file changed, 2 insertions(+), 2 deletions(-)
>
>     diff --git a/hw/rdma/vmw/pvrdma_dev_ring.c
>     b/hw/rdma/vmw/pvrdma_dev_ring.c
>     index d7bc7f5ccc..74b8fa834c 100644
>     --- a/hw/rdma/vmw/pvrdma_dev_ring.c
>     +++ b/hw/rdma/vmw/pvrdma_dev_ring.c
>     @@ -14,6 +14,7 @@
>       */
>
>      #include "qemu/osdep.h"
>     +#include "qemu/cutils.h"
>      #include "hw/pci/pci.h"
>      #include "cpu.h"
>
>     @@ -30,8 +31,7 @@ int pvrdma_ring_init(PvrdmaRing *ring, const
>     char *name, PCIDevice *dev,
>          int i;
>          int rc = 0;
>
>     -    strncpy(ring->name, name, MAX_RING_NAME_SZ);
>     -    ring->name[MAX_RING_NAME_SZ - 1] = 0;
>     +    pstrcpy(ring->name, MAX_RING_NAME_SZ, name);
>          ring->dev = dev;
>          ring->ring_state = ring_state;
>          ring->max_elems = max_elems;
>     -- 
>     2.24.1
>
>
> Thanks,
>
> Reviewed-by: Yuval Shaia <yuval.shaia.ml.gmail.com 
> <http://yuval.shaia.ml.gmail.com>>

I'll add this patch to the pvrdma queue.

Thanks,
Marcel