Re: [Qemu-block] [Qemu-devel] [PULL 5/8] commit: Fix use after free in completion

[Peter Maydell]

On 7 June 2017 at 18:50, Kevin Wolf <address@hidden> wrote:
> The final bdrv_set_backing_hd() could be working on already freed nodes
> because the commit job drops its references (through BlockBackends) to
> both overlay_bs and top already a bit earlier.
>
> One way to trigger the bug is hot unplugging a disk for which
> blockdev_mark_auto_del() cancels the block job.
>
> Fix this by taking BDS-level references while we're still using the
> nodes.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> Reviewed-by: John Snow <address@hidden>
> ---
>  block/commit.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/block/commit.c b/block/commit.c
> index a3028b2..af6fa68 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -89,6 +89,10 @@ static void commit_complete(BlockJob *job, void *opaque)
>      int ret = data->ret;
>      bool remove_commit_top_bs = false;
>
> +    /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() 
> */
> +    bdrv_ref(top);
> +    bdrv_ref(overlay_bs);
> +
>      /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
>       * the normal backing chain can be restored. */
>      blk_unref(s->base);

Hi -- coverity complains about this change, because bdrv_ref()
assumes that its argument is not NULL, but later on in commit_complete()
we have a check
    "if (overlay_bs && ...)"
which assumes its argument might be NULL. (CID 1376205)

Which is correct?

thanks
-- PMM