qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: zhanghailiang
Subject: Re: [Qemu-devel] [PATCH v2] dump: fix use-after-free for s->fd
Date: Thu, 30 Oct 2014 15:33:48 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Thunderbird/31.1.1

On 2014/10/30 15:10, Markus Armbruster wrote:
<address@hidden> writes:

From: Gonglei <address@hidden>

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 <address@hidden>

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?


Yep, this is really a stupid mistake i made when i do clean work for dump.c,
what lucky thing is there is no version release after the commit.
Thanks for your good catch.;)

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.








reply via email to

[Prev in Thread] Current Thread [Next in Thread]