Re: [Qemu-devel] [Qemu-trivial] [PATCH v2] dump: fix use-after-free for s->fd

[Michael Tokarev <mjt@tls.msk.ru>]

30.10.2014 10:10, Markus Armbruster wrote:
> <arei.gonglei@huawei.com> writes:
> 
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> After commit 4c7e251a (), when dump memory completed,
>> the s->fd will be closed twice. We should return
>> directly when dump completed.
>>
>> Using do/while block, make the badly chosen return
>> values of get_next_block() more visible and fix
>> this issue.
>>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> 
> I'm afraid the commit message is a bit misleading.  Let's examine what
> exactly happens.
> 
> dump_iterate() dumps blocks in a loop.  Eventually, get_next_block()
> returns "no more".  We then call dump_completed().  But we neglect to
> break the loop!  Broken in commit 4c7e251a.
> 
> Because of that, we dump the last block again.  This attempts to write
> to s->fd, which fails if we're lucky.  The error makes dump_iterate()
> return unsuccessfully.  It's the only way it can ever return.
> 
> Theoretical: if we're not so lucky, something else has opened something
> for writing and got the same fd.  dump_iterate() then keeps looping,
> messing up the something else's output, until a write fails, or the
> process mercifully terminates.
> 
> Is this correct?

Heh.  I was starring at all this last 20 minutes, re-reading the
original v1 patch and your (Marcus) followup suggestion, trying to
match the commit description with the actual happening and with the
"no return" case which was before this patch.  Oh well.

Yes, this looks correct indeed, we come to the same conclusion.
But at this stage I really wonder if this is a -trivial material.

(I can apply it to -trivial because no maintainer is listed for
this file and because after some digging it becomes obvious).

/mjt

> If yes, let's use this commit message:
> 
>     dump: Fix dump-guest-memory termination and use-after-close
> 
>     dump_iterate() dumps blocks in a loop.  Eventually, get_next_block()
>     returns "no more".  We then call dump_completed().  But we neglect to
>     break the loop!  Broken in commit 4c7e251a.
> 
>     Because of that, we dump the last block again.  This attempts to write
>     to s->fd, which fails if we're lucky.  The error makes dump_iterate()
>     return failure.  It's the only way it can ever return.
> 
>     Theoretical: if we're not so lucky, something else has opened something
>     for writing and got the same fd.  dump_iterate() then keeps looping,
>     messing up the something else's output, until a write fails, or the
>     process mercifully terminates.
> 
>     The obvious fix is to restore the return lost in commit 4c7e251a.  But
>     the root cause of the bug is needlessly opaque loop control.  Replace it
>     by a clean do ... while loop.
> 
>     This makes the badly chosen return values of get_next_block() more
>     visible.  Cleaning that up is outside the scope of this bug fix.
> 
> You can then add my R-by.
>