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

[Juan Quintela <quintela@redhat.com>]

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.