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

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

[arei.gonglei]

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>
---
v2 -> v1:
Using do/while block, make the badly chosen return
values of get_next_block() more visible and fix
this issue. (Markus)
---
 dump.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/dump.c b/dump.c
index 06a4915..9c7dad8 100644
--- a/dump.c
+++ b/dump.c
@@ -604,10 +604,9 @@ static void dump_iterate(DumpState *s, Error **errp)
 {
     GuestPhysBlock *block;
     int64_t size;
-    int ret;
     Error *local_err = NULL;
 
-    while (1) {
+    do {
         block = s->next_block;
 
         size = block->target_end - block->target_start;
@@ -623,11 +622,9 @@ static void dump_iterate(DumpState *s, Error **errp)
             return;
         }
 
-        ret = get_next_block(s, block);
-        if (ret == 1) {
-            dump_completed(s);
-        }
-    }
+    } while (!get_next_block(s, block));
+
+    dump_completed(s);
 }
 
 static void create_vmcore(DumpState *s, Error **errp)
-- 
1.7.12.4

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

[Markus Armbruster]

<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?

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.

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

[zhanghailiang]

On 2014/10/30 15: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?
>

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.
>
>
>

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

[Michael Tokarev]

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.
> 

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

[Markus Armbruster]

Michael Tokarev <mjt@tls.msk.ru> writes:

> 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.

Distinguished old math professor does a proof on the blackboard.  At
some point he faces the audience and says "this is trivial".  Faces
blackboard, pauses.  "Is it trivial?"  Pauses again.  Storms out of the
classroom.  After ten minutes, he comes back and exclaims "it *is*
trivial!"

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

Appreciated!

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

[Gonglei]

On 2014/10/30 17:23, Markus Armbruster wrote:

> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> 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.
> 
> Distinguished old math professor does a proof on the blackboard.  At
> some point he faces the audience and says "this is trivial".  Faces
> blackboard, pauses.  "Is it trivial?"  Pauses again.  Storms out of the
> classroom.  After ten minutes, he comes back and exclaims "it *is*
> trivial!"
> 

:)

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

Do I need to send v3 for changing commit message, or
/mjt do it directly? Thanks

Best regards,
-Gonglei

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

[Michael Tokarev]

30.10.2014 10:10, Markus Armbruster wrote:
[]
> 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?
> 
> 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.

So I'm applying this -- which is your patch and your commit message, and
I really wonder why this is Reviewed-by and not Signed-off-by, with your
authorship?  It really should be...

Thanks,

/mjt

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

[Gonglei]

On 2014/10/30 21:54, Michael Tokarev wrote:

> So I'm applying this -- which is your patch and your commit message, and
> I really wonder why this is Reviewed-by and not Signed-off-by, with your
> authorship?  It really should be...

Yes, maybe it should be. But I have to say something:
First, I posted a patch fix the fd leak problem. Markus reviewed it and gave
his reviewing comments which I think is better, and then I posted
the version 2 with Markus' suggestion. As your meaning, I should add the
Signed-off-by tag of Markus? But for me, I don't get Markus' authorization,
so I can't do this, and maybe he have other comments for version 2.

Best regards,
-Gonglei

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

[Michael Tokarev]

31.10.2014 04:43, Gonglei wrote:
> On 2014/10/30 21:54, Michael Tokarev wrote:
> 
>> So I'm applying this -- which is your patch and your commit message, and
>> I really wonder why this is Reviewed-by and not Signed-off-by, with your
>> authorship?  It really should be...
> 
> Yes, maybe it should be. But I have to say something:
> First, I posted a patch fix the fd leak problem. Markus reviewed it and gave
> his reviewing comments which I think is better, and then I posted
> the version 2 with Markus' suggestion. As your meaning, I should add the
> Signed-off-by tag of Markus? But for me, I don't get Markus' authorization,
> so I can't do this, and maybe he have other comments for version 2.

Um.  I didn't want to offend you in any way.  I was just pointing out that
it was actually Marcus who did the rest of the work, besides discovering the
original problem.  Again, it is both his change and his commit message...

Let's agree to have Signed-off-by from both of you ;)

Thanks,

/mjt

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

[Gonglei]

On 2014/10/31 15:18, Michael Tokarev wrote:

> 31.10.2014 04:43, Gonglei wrote:
>> On 2014/10/30 21:54, Michael Tokarev wrote:
>>
>>> So I'm applying this -- which is your patch and your commit message, and
>>> I really wonder why this is Reviewed-by and not Signed-off-by, with your
>>> authorship?  It really should be...
>>
>> Yes, maybe it should be. But I have to say something:
>> First, I posted a patch fix the fd leak problem. Markus reviewed it and gave
>> his reviewing comments which I think is better, and then I posted
>> the version 2 with Markus' suggestion. As your meaning, I should add the
>> Signed-off-by tag of Markus? But for me, I don't get Markus' authorization,
>> so I can't do this, and maybe he have other comments for version 2.
> 
> Um.  I didn't want to offend you in any way.  I was just pointing out that
> it was actually Marcus who did the rest of the work, besides discovering the
> original problem.  Again, it is both his change and his commit message...
> 
> Let's agree to have Signed-off-by from both of you ;)

Yes, of course.

Best regards,
-Gonglei

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

[Markus Armbruster]

Michael Tokarev <mjt@tls.msk.ru> writes:

> 30.10.2014 10:10, Markus Armbruster wrote:
> []
>> 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?
>> 
>> 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.
>
> So I'm applying this -- which is your patch and your commit message, and
> I really wonder why this is Reviewed-by and not Signed-off-by, with your
> authorship?  It really should be...

You can add mine in addition to Gonglei's.

Signed-off-by: Markus Armbruster <armbru@redhat.com>