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

[Stefan Hajnoczi]

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.  :?

Juan: Please clarify the above.  Thanks!

signature.asc
Description: PGP signature