[v5,1/6] qcow2: Fix dangling pointer after reopen for 'file'

Without an external data file, s->data_file is a second pointer with the
same value as bs->file. When changing bs->file to a different BdrvChild
and freeing the old BdrvChild, s->data_file must also be updated,
otherwise it points to freed memory and causes crashes.

This problem was caught by iotests case 245.

Fixes: df2b7086f169239ebad5d150efa29c9bb6d4f820
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

06.07.2021 14:23, Kevin Wolf wrote:
> Without an external data file, s->data_file is a second pointer with the
> same value as bs->file. When changing bs->file to a different BdrvChild
> and freeing the old BdrvChild, s->data_file must also be updated,
> otherwise it points to freed memory and causes crashes.
> 
> This problem was caught by iotests case 245.
> 
> Fixes: df2b7086f169239ebad5d150efa29c9bb6d4f820
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Still, some ideas below:

> ---
>   block/qcow2.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index ee4530cdbd..cb459ef6a6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -962,6 +962,7 @@ static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
>   }
>   
>   typedef struct Qcow2ReopenState {
> +    bool had_data_file;
>       Qcow2Cache *l2_table_cache;
>       Qcow2Cache *refcount_block_cache;
>       int l2_slice_size; /* Number of entries in a slice of the L2 table */
> @@ -1932,6 +1933,8 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
>       r = g_new0(Qcow2ReopenState, 1);
>       state->opaque = r;
>   
> +    r->had_data_file = has_data_file(state->bs);
> +

So, during reopen, at some moment s->data_file become invalid. So, we shouldn't rely on it..

Maybe we need

        s->data_file = NULL;

here..

>       ret = qcow2_update_options_prepare(state->bs, r, state->options,
>                                          state->flags, errp);
>       if (ret < 0) {
> @@ -1966,7 +1969,18 @@ fail:
>   
>   static void qcow2_reopen_commit(BDRVReopenState *state)
>   {
> +    BDRVQcow2State *s = state->bs->opaque;
> +    Qcow2ReopenState *r = state->opaque;
> +
>       qcow2_update_options_commit(state->bs, state->opaque);

Worth doing

        assert(r->had_data_file == has_data_file(state->bs));

here, to be double sure?

> +    if (!r->had_data_file && s->data_file != state->bs->file) {
> +        /*
> +         * If s->data_file is just a second pointer to bs->file (which is the
> +         * case without an external data file), it may need to be updated.
> +         */
> +        s->data_file = state->bs->file;
> +        assert(!has_data_file(state->bs));
> +    }
>       g_free(state->opaque);
>   }
>   
>

Am 06.07.2021 um 15:12 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 06.07.2021 14:23, Kevin Wolf wrote:
> > Without an external data file, s->data_file is a second pointer with the
> > same value as bs->file. When changing bs->file to a different BdrvChild
> > and freeing the old BdrvChild, s->data_file must also be updated,
> > otherwise it points to freed memory and causes crashes.
> > 
> > This problem was caught by iotests case 245.
> > 
> > Fixes: df2b7086f169239ebad5d150efa29c9bb6d4f820
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Still, some ideas below:
> 
> > ---
> >   block/qcow2.c | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index ee4530cdbd..cb459ef6a6 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -962,6 +962,7 @@ static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
> >   }
> >   typedef struct Qcow2ReopenState {
> > +    bool had_data_file;
> >       Qcow2Cache *l2_table_cache;
> >       Qcow2Cache *refcount_block_cache;
> >       int l2_slice_size; /* Number of entries in a slice of the L2 table */
> > @@ -1932,6 +1933,8 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
> >       r = g_new0(Qcow2ReopenState, 1);
> >       state->opaque = r;
> > +    r->had_data_file = has_data_file(state->bs);
> > +
> 
> So, during reopen, at some moment s->data_file become invalid. So, we
> shouldn't rely on it..
> 
> Maybe we need
> 
>        s->data_file = NULL;
> 
> here..

"need" is a strong word, but I guess we shouldn't access it between
prepare and commit, so I agree setting it to NULL would make bugs in
this area very visible.

In fact, we wouldn't even need r->had_data_file then because commit
could just set s->data_file = state->bs->file if it's NULL.

> >       ret = qcow2_update_options_prepare(state->bs, r, state->options,
> >                                          state->flags, errp);
> >       if (ret < 0) {
> > @@ -1966,7 +1969,18 @@ fail:
> >   static void qcow2_reopen_commit(BDRVReopenState *state)
> >   {
> > +    BDRVQcow2State *s = state->bs->opaque;
> > +    Qcow2ReopenState *r = state->opaque;
> > +
> >       qcow2_update_options_commit(state->bs, state->opaque);
> 
> Worth doing
> 
>        assert(r->had_data_file == has_data_file(state->bs));
> 
> here, to be double sure?

This would be wrong because at the time that this runs, state->bs->file
is already updated and this is what has_data_file() checks against. So
you can't use has_data_file() any more until it's synced again with the
code below.

In fact, this is why I even added r->had_data_file. At first I directly
used has_data_file(state->bs) here and watched it break.

> > +    if (!r->had_data_file && s->data_file != state->bs->file) {
> > +        /*
> > +         * If s->data_file is just a second pointer to bs->file (which is the
> > +         * case without an external data file), it may need to be updated.
> > +         */
> > +        s->data_file = state->bs->file;
> > +        assert(!has_data_file(state->bs));
> > +    }
> >       g_free(state->opaque);
> >   }

Kevin