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

[Juan Quintela <quintela@redhat.com>]

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.