[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] block: bdrv_set_backing_bs: fix use-after-free
From: |
John Snow |
Subject: |
Re: [PATCH 1/2] block: bdrv_set_backing_bs: fix use-after-free |
Date: |
Mon, 16 Mar 2020 13:37:38 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 |
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.