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/address@hidden/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 <address@hidden>
> > > > ---
> > > >    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!