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

[Stefan Hajnoczi]

On Tue, Mar 17, 2020 at 11:35:14AM +0100, Juan Quintela wrote:
> Stefan Hajnoczi <address@hidden> 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 <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:
> 
> 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

signature.asc
Description: PGP signature