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 <address@hidden>
> 
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> 
>> ---
>>   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 <address@hidden>

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.