[PATCH 0/2] zero pointer after bdrv_unref_child

[PATCH 0/2] zero pointer after bdrv_unref_child

[Vladimir Sementsov-Ogievskiy]

Hi all!

I faced use-after-free of bs->backing pointer after bdrv_unref_child in
bdrv_set_backing_hd.

Fix it, and do similar thing for s->data_file in qcow2.c.

I'm not sure that this is the full fix. Is it safe to keep bs->backing
during bdrv_unref_child itself? Is it safe to keep bs->backing during
all-child-unref loop in bdrv_close?


Vladimir Sementsov-Ogievskiy (2):
  block: bdrv_set_backing_bs: fix use-after-free
  block/qcow2: zero data_file child after free

 block.c       | 2 +-
 block/qcow2.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

-- 
2.21.0

[PATCH 1/2] block: bdrv_set_backing_bs: fix use-after-free

[Vladimir Sementsov-Ogievskiy]

There is a use-after-free possible: bdrv_unref_child() leaves
bs->backing freed but not NULL. bdrv_attach_child may produce nested
polling loop due to drain, than access of freed pointer is possible.

I've produced the following crash on 30 iotest with modified code. It
does not reproduce on master, but still seems possible:

    #0  __strcmp_avx2 () at /lib64/libc.so.6
    #1  bdrv_backing_overridden (bs=0x55c9d3cc2060) at block.c:6350
    #2  bdrv_refresh_filename (bs=0x55c9d3cc2060) at block.c:6404
    #3  bdrv_backing_attach (c=0x55c9d48e5520) at block.c:1063
    #4  bdrv_replace_child_noperm
        (child=child@entry=0x55c9d48e5520,
        new_bs=new_bs@entry=0x55c9d3cc2060) at block.c:2290
    #5  bdrv_replace_child
        (child=child@entry=0x55c9d48e5520,
        new_bs=new_bs@entry=0x55c9d3cc2060) at block.c:2320
    #6  bdrv_root_attach_child
        (child_bs=child_bs@entry=0x55c9d3cc2060,
        child_name=child_name@entry=0x55c9d241d478 "backing",
        child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
        ctx=<optimized out>, perm=<optimized out>, shared_perm=21,
        opaque=0x55c9d3c5a3d0, errp=0x7ffd117108e0) at block.c:2424
    #7  bdrv_attach_child
        (parent_bs=parent_bs@entry=0x55c9d3c5a3d0,
        child_bs=child_bs@entry=0x55c9d3cc2060,
        child_name=child_name@entry=0x55c9d241d478 "backing",
        child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
        errp=errp@entry=0x7ffd117108e0) at block.c:5876
    #8  in bdrv_set_backing_hd
        (bs=bs@entry=0x55c9d3c5a3d0,
        backing_hd=backing_hd@entry=0x55c9d3cc2060,
        errp=errp@entry=0x7ffd117108e0)
        at block.c:2576
    #9  stream_prepare (job=0x55c9d49d84a0) at block/stream.c:150
    #10 job_prepare (job=0x55c9d49d84a0) at job.c:761
    #11 job_txn_apply (txn=<optimized out>, fn=<optimized out>) at
        job.c:145
    #12 job_do_finalize (job=0x55c9d49d84a0) at job.c:778
    #13 job_completed_txn_success (job=0x55c9d49d84a0) at job.c:832
    #14 job_completed (job=0x55c9d49d84a0) at job.c:845
    #15 job_completed (job=0x55c9d49d84a0) at job.c:836
    #16 job_exit (opaque=0x55c9d49d84a0) at job.c:864
    #17 aio_bh_call (bh=0x55c9d471a160) at util/async.c:117
    #18 aio_bh_poll (ctx=ctx@entry=0x55c9d3c46720) at util/async.c:117
    #19 aio_poll (ctx=ctx@entry=0x55c9d3c46720,
        blocking=blocking@entry=true)
        at util/aio-posix.c:728
    #20 bdrv_parent_drained_begin_single (poll=true, c=0x55c9d3d558f0)
        at block/io.c:121
    #21 bdrv_parent_drained_begin_single (c=c@entry=0x55c9d3d558f0,
        poll=poll@entry=true)
        at block/io.c:114
    #22 bdrv_replace_child_noperm
        (child=child@entry=0x55c9d3d558f0,
        new_bs=new_bs@entry=0x55c9d3d27300) at block.c:2258
    #23 bdrv_replace_child
        (child=child@entry=0x55c9d3d558f0,
        new_bs=new_bs@entry=0x55c9d3d27300) at block.c:2320
    #24 bdrv_root_attach_child
        (child_bs=child_bs@entry=0x55c9d3d27300,
        child_name=child_name@entry=0x55c9d241d478 "backing",
        child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
        ctx=<optimized out>, perm=<optimized out>, shared_perm=21,
        opaque=0x55c9d3cc2060, errp=0x7ffd11710c60) at block.c:2424
    #25 bdrv_attach_child
        (parent_bs=parent_bs@entry=0x55c9d3cc2060,
        child_bs=child_bs@entry=0x55c9d3d27300,
        child_name=child_name@entry=0x55c9d241d478 "backing",
        child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
        errp=errp@entry=0x7ffd11710c60) at block.c:5876
    #26 bdrv_set_backing_hd
        (bs=bs@entry=0x55c9d3cc2060,
        backing_hd=backing_hd@entry=0x55c9d3d27300,
        errp=errp@entry=0x7ffd11710c60)
        at block.c:2576
    #27 stream_prepare (job=0x55c9d495ead0) at block/stream.c:150
    ...

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 957630b1c5..a862ce4df9 100644
--- a/block.c
+++ b/block.c
@@ -2735,10 +2735,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
 
     if (bs->backing) {
         bdrv_unref_child(bs, bs->backing);
+        bs->backing = NULL;
     }
 
     if (!backing_hd) {
-        bs->backing = NULL;
         goto out;
     }
 
-- 
2.21.0

[PATCH 2/2] block/qcow2: zero data_file child after free

[Vladimir Sementsov-Ogievskiy]

data_file being NULL doesn't seem to be a correct state, but it's
better than dead pointer and simpler to debug.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index d44b45633d..6cdefe059f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1758,6 +1758,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
     g_free(s->image_data_file);
     if (has_data_file(bs)) {
         bdrv_unref_child(bs, s->data_file);
+        s->data_file = NULL;
     }
     g_free(s->unknown_header_fields);
     cleanup_unknown_header_ext(bs);
@@ -2621,6 +2622,7 @@ static void qcow2_close(BlockDriverState *bs)
 
     if (has_data_file(bs)) {
         bdrv_unref_child(bs, s->data_file);
+        s->data_file = NULL;
     }
 
     qcow2_refcount_close(bs);
-- 
2.21.0

Re: [PATCH 1/2] block: bdrv_set_backing_bs: fix use-after-free

[Philippe Mathieu-Daudé]

On 3/16/20 7:06 AM, Vladimir Sementsov-Ogievskiy wrote:
> There is a use-after-free possible: bdrv_unref_child() leaves
> bs->backing freed but not NULL. bdrv_attach_child may produce nested
> polling loop due to drain, than access of freed pointer is possible.
> 
> I've produced the following crash on 30 iotest with modified code. It
> does not reproduce on master, but still seems possible:
> 
>      #0  __strcmp_avx2 () at /lib64/libc.so.6
>      #1  bdrv_backing_overridden (bs=0x55c9d3cc2060) at block.c:6350
>      #2  bdrv_refresh_filename (bs=0x55c9d3cc2060) at block.c:6404
>      #3  bdrv_backing_attach (c=0x55c9d48e5520) at block.c:1063
>      #4  bdrv_replace_child_noperm
>          (child=child@entry=0x55c9d48e5520,
>          new_bs=new_bs@entry=0x55c9d3cc2060) at block.c:2290
>      #5  bdrv_replace_child
>          (child=child@entry=0x55c9d48e5520,
>          new_bs=new_bs@entry=0x55c9d3cc2060) at block.c:2320
>      #6  bdrv_root_attach_child
>          (child_bs=child_bs@entry=0x55c9d3cc2060,
>          child_name=child_name@entry=0x55c9d241d478 "backing",
>          child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
>          ctx=<optimized out>, perm=<optimized out>, shared_perm=21,
>          opaque=0x55c9d3c5a3d0, errp=0x7ffd117108e0) at block.c:2424
>      #7  bdrv_attach_child
>          (parent_bs=parent_bs@entry=0x55c9d3c5a3d0,
>          child_bs=child_bs@entry=0x55c9d3cc2060,
>          child_name=child_name@entry=0x55c9d241d478 "backing",
>          child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
>          errp=errp@entry=0x7ffd117108e0) at block.c:5876
>      #8  in bdrv_set_backing_hd
>          (bs=bs@entry=0x55c9d3c5a3d0,
>          backing_hd=backing_hd@entry=0x55c9d3cc2060,
>          errp=errp@entry=0x7ffd117108e0)
>          at block.c:2576
>      #9  stream_prepare (job=0x55c9d49d84a0) at block/stream.c:150
>      #10 job_prepare (job=0x55c9d49d84a0) at job.c:761
>      #11 job_txn_apply (txn=<optimized out>, fn=<optimized out>) at
>          job.c:145
>      #12 job_do_finalize (job=0x55c9d49d84a0) at job.c:778
>      #13 job_completed_txn_success (job=0x55c9d49d84a0) at job.c:832
>      #14 job_completed (job=0x55c9d49d84a0) at job.c:845
>      #15 job_completed (job=0x55c9d49d84a0) at job.c:836
>      #16 job_exit (opaque=0x55c9d49d84a0) at job.c:864
>      #17 aio_bh_call (bh=0x55c9d471a160) at util/async.c:117
>      #18 aio_bh_poll (ctx=ctx@entry=0x55c9d3c46720) at util/async.c:117
>      #19 aio_poll (ctx=ctx@entry=0x55c9d3c46720,
>          blocking=blocking@entry=true)
>          at util/aio-posix.c:728
>      #20 bdrv_parent_drained_begin_single (poll=true, c=0x55c9d3d558f0)
>          at block/io.c:121
>      #21 bdrv_parent_drained_begin_single (c=c@entry=0x55c9d3d558f0,
>          poll=poll@entry=true)
>          at block/io.c:114
>      #22 bdrv_replace_child_noperm
>          (child=child@entry=0x55c9d3d558f0,
>          new_bs=new_bs@entry=0x55c9d3d27300) at block.c:2258
>      #23 bdrv_replace_child
>          (child=child@entry=0x55c9d3d558f0,
>          new_bs=new_bs@entry=0x55c9d3d27300) at block.c:2320
>      #24 bdrv_root_attach_child
>          (child_bs=child_bs@entry=0x55c9d3d27300,
>          child_name=child_name@entry=0x55c9d241d478 "backing",
>          child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
>          ctx=<optimized out>, perm=<optimized out>, shared_perm=21,
>          opaque=0x55c9d3cc2060, errp=0x7ffd11710c60) at block.c:2424
>      #25 bdrv_attach_child
>          (parent_bs=parent_bs@entry=0x55c9d3cc2060,
>          child_bs=child_bs@entry=0x55c9d3d27300,
>          child_name=child_name@entry=0x55c9d241d478 "backing",
>          child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
>          errp=errp@entry=0x7ffd11710c60) at block.c:5876
>      #26 bdrv_set_backing_hd
>          (bs=bs@entry=0x55c9d3cc2060,
>          backing_hd=backing_hd@entry=0x55c9d3d27300,
>          errp=errp@entry=0x7ffd11710c60)
>          at block.c:2576
>      #27 stream_prepare (job=0x55c9d495ead0) at block/stream.c:150
>      ...
> 

Apparently:
Fixes: 12fa4af61f (block: Add Error parameter to bdrv_set_backing_hd)
Right?

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

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   block.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 957630b1c5..a862ce4df9 100644
> --- a/block.c
> +++ b/block.c
> @@ -2735,10 +2735,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
>   
>       if (bs->backing) {
>           bdrv_unref_child(bs, bs->backing);
> +        bs->backing = NULL;
>       }
>   
>       if (!backing_hd) {
> -        bs->backing = NULL;
>           goto out;
>       }
>   
> 

Re: [PATCH 1/2] block: bdrv_set_backing_bs: fix use-after-free

[John Snow]

On 3/16/20 4:47 AM, Philippe Mathieu-Daudé wrote:
> On 3/16/20 7:06 AM, Vladimir Sementsov-Ogievskiy wrote:
>> There is a use-after-free possible: bdrv_unref_child() leaves
>> bs->backing freed but not NULL. bdrv_attach_child may produce nested
>> polling loop due to drain, than access of freed pointer is possible.
>>
>> I've produced the following crash on 30 iotest with modified code. It
>> does not reproduce on master, but still seems possible:
>>
>>      #0  __strcmp_avx2 () at /lib64/libc.so.6
>>      #1  bdrv_backing_overridden (bs=0x55c9d3cc2060) at block.c:6350
>>      #2  bdrv_refresh_filename (bs=0x55c9d3cc2060) at block.c:6404
>>      #3  bdrv_backing_attach (c=0x55c9d48e5520) at block.c:1063
>>      #4  bdrv_replace_child_noperm
>>          (child=child@entry=0x55c9d48e5520,
>>          new_bs=new_bs@entry=0x55c9d3cc2060) at block.c:2290
>>      #5  bdrv_replace_child
>>          (child=child@entry=0x55c9d48e5520,
>>          new_bs=new_bs@entry=0x55c9d3cc2060) at block.c:2320
>>      #6  bdrv_root_attach_child
>>          (child_bs=child_bs@entry=0x55c9d3cc2060,
>>          child_name=child_name@entry=0x55c9d241d478 "backing",
>>          child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
>>          ctx=<optimized out>, perm=<optimized out>, shared_perm=21,
>>          opaque=0x55c9d3c5a3d0, errp=0x7ffd117108e0) at block.c:2424
>>      #7  bdrv_attach_child
>>          (parent_bs=parent_bs@entry=0x55c9d3c5a3d0,
>>          child_bs=child_bs@entry=0x55c9d3cc2060,
>>          child_name=child_name@entry=0x55c9d241d478 "backing",
>>          child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
>>          errp=errp@entry=0x7ffd117108e0) at block.c:5876
>>      #8  in bdrv_set_backing_hd
>>          (bs=bs@entry=0x55c9d3c5a3d0,
>>          backing_hd=backing_hd@entry=0x55c9d3cc2060,
>>          errp=errp@entry=0x7ffd117108e0)
>>          at block.c:2576
>>      #9  stream_prepare (job=0x55c9d49d84a0) at block/stream.c:150
>>      #10 job_prepare (job=0x55c9d49d84a0) at job.c:761
>>      #11 job_txn_apply (txn=<optimized out>, fn=<optimized out>) at
>>          job.c:145
>>      #12 job_do_finalize (job=0x55c9d49d84a0) at job.c:778
>>      #13 job_completed_txn_success (job=0x55c9d49d84a0) at job.c:832
>>      #14 job_completed (job=0x55c9d49d84a0) at job.c:845
>>      #15 job_completed (job=0x55c9d49d84a0) at job.c:836
>>      #16 job_exit (opaque=0x55c9d49d84a0) at job.c:864
>>      #17 aio_bh_call (bh=0x55c9d471a160) at util/async.c:117
>>      #18 aio_bh_poll (ctx=ctx@entry=0x55c9d3c46720) at util/async.c:117
>>      #19 aio_poll (ctx=ctx@entry=0x55c9d3c46720,
>>          blocking=blocking@entry=true)
>>          at util/aio-posix.c:728
>>      #20 bdrv_parent_drained_begin_single (poll=true, c=0x55c9d3d558f0)
>>          at block/io.c:121
>>      #21 bdrv_parent_drained_begin_single (c=c@entry=0x55c9d3d558f0,
>>          poll=poll@entry=true)
>>          at block/io.c:114
>>      #22 bdrv_replace_child_noperm
>>          (child=child@entry=0x55c9d3d558f0,
>>          new_bs=new_bs@entry=0x55c9d3d27300) at block.c:2258
>>      #23 bdrv_replace_child
>>          (child=child@entry=0x55c9d3d558f0,
>>          new_bs=new_bs@entry=0x55c9d3d27300) at block.c:2320
>>      #24 bdrv_root_attach_child
>>          (child_bs=child_bs@entry=0x55c9d3d27300,
>>          child_name=child_name@entry=0x55c9d241d478 "backing",
>>          child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
>>          ctx=<optimized out>, perm=<optimized out>, shared_perm=21,
>>          opaque=0x55c9d3cc2060, errp=0x7ffd11710c60) at block.c:2424
>>      #25 bdrv_attach_child
>>          (parent_bs=parent_bs@entry=0x55c9d3cc2060,
>>          child_bs=child_bs@entry=0x55c9d3d27300,
>>          child_name=child_name@entry=0x55c9d241d478 "backing",
>>          child_role=child_role@entry=0x55c9d26ecee0 <child_backing>,
>>          errp=errp@entry=0x7ffd11710c60) at block.c:5876
>>      #26 bdrv_set_backing_hd
>>          (bs=bs@entry=0x55c9d3cc2060,
>>          backing_hd=backing_hd@entry=0x55c9d3d27300,
>>          errp=errp@entry=0x7ffd11710c60)
>>          at block.c:2576
>>      #27 stream_prepare (job=0x55c9d495ead0) at block/stream.c:150
>>      ...
>>
> 
> Apparently:
> Fixes: 12fa4af61f (block: Add Error parameter to bdrv_set_backing_hd)
> Right?
> 

It's possible, but the way the bug manifests is very likely to have
changed dramatically since 2.10.0.

This is (probably?) not a regression in any stable version supported
upstream, or caused by that commit. It likely happened later when attach
and recursive flushes were reworked.


>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
>> ---
>>   block.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block.c b/block.c
>> index 957630b1c5..a862ce4df9 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2735,10 +2735,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs,
>> BlockDriverState *backing_hd,
>>         if (bs->backing) {
>>           bdrv_unref_child(bs, bs->backing);
>> +        bs->backing = NULL;
>>       }
>>         if (!backing_hd) {
>> -        bs->backing = NULL;
>>           goto out;
>>       }
>>  
> 
> 

This seems more obviously correct; but what's the error policy if
bdrv_attach_child fails? We'll have dropped the old file but won't pick
the new one up.

(That already seems to happen without this patch, so ... not a new concern.)

I feel comfortable to say:

Reviewed-by: John Snow <jsnow@redhat.com>

But don't know the answer to more structural questions, like if it's
important that the detached child gets flushed or not, in what order
that happens, etc etc.

Re: [PATCH 2/2] block/qcow2: zero data_file child after free

[John Snow]

On 3/16/20 2:06 AM, Vladimir Sementsov-Ogievskiy wrote:
> data_file being NULL doesn't seem to be a correct state, but it's
> better than dead pointer and simpler to debug.
> 

How important is it to have correct state in the middle of teardown?

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index d44b45633d..6cdefe059f 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1758,6 +1758,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>      g_free(s->image_data_file);
>      if (has_data_file(bs)) {
>          bdrv_unref_child(bs, s->data_file);
> +        s->data_file = NULL;
>      }

Probably OK to set to NULL, since this is at the end of a failed open
where it would have been set for the first time anyway.

It's an invalid state, but resulting from a failed call. I think that's OK.

(Are there any callers of bdrv_open or qcow2_open that don't just
immediately trash this object if it failed? I don't know of any, but
there's a lot of callers to bdrv_open.)

>      g_free(s->unknown_header_fields);
>      cleanup_unknown_header_ext(bs);
> @@ -2621,6 +2622,7 @@ static void qcow2_close(BlockDriverState *bs)
>  
>      if (has_data_file(bs)) {
>          bdrv_unref_child(bs, s->data_file);
> +        s->data_file = NULL;
>      }
>  

Probably fine here too. I can't imagine it's valid to use this object
after close() ... unless we open it again, and that should handle
setting this back to a realistic value.

>      qcow2_refcount_close(bs);
> 



So I think this is fine? If I understand right this just makes failures
more obvious if we do accidentally use this value after a failed open or
close, so that seems fine.

Reviewed-by: John Snow <jsnow@redhat.com>


(As always, I'll rely on block maintainers to do more serious structural
review for cases I am not aware of)

Re: [PATCH 0/2] zero pointer after bdrv_unref_child

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 681 bytes --]

On 16.03.20 07:06, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> I faced use-after-free of bs->backing pointer after bdrv_unref_child in
> bdrv_set_backing_hd.
> 
> Fix it, and do similar thing for s->data_file in qcow2.c.
> 
> I'm not sure that this is the full fix. Is it safe to keep bs->backing
> during bdrv_unref_child itself? Is it safe to keep bs->backing during
> all-child-unref loop in bdrv_close?
> 
> 
> Vladimir Sementsov-Ogievskiy (2):
>   block: bdrv_set_backing_bs: fix use-after-free
>   block/qcow2: zero data_file child after free

Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]