| Message ID | 1403602356-13687-1-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
On 06/24/2014 03:32 AM, Peter Lieven wrote: > this patch extends commit db80fac by not only checking > for unknown flags, but also filtering out unknown flag > combinations. > > Suggested-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > arch_init.c | 62 ++++++++++++++++++++++++++++++----------------------------- > 1 file changed, 32 insertions(+), 30 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
Peter Lieven <pl@kamp.de> wrote: > this patch extends commit db80fac by not only checking > for unknown flags, but also filtering out unknown flag > combinations. > > Suggested-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Peter Lieven <pl@kamp.de> Reviewed-by: Juan Quintela <quintela@redhat.com> Will be on next pull request, thanks.
Hi Juan, Am 25.06.2014 um 13:55 schrieb Juan Quintela <quintela@redhat.com>: > Peter Lieven <pl@kamp.de> wrote: >> this patch extends commit db80fac by not only checking >> for unknown flags, but also filtering out unknown flag >> combinations. >> >> Suggested-by: Eric Blake <eblake@redhat.com> >> Signed-off-by: Peter Lieven <pl@kamp.de> > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > Will be on next pull request, thanks. > Have you forgotten to pull this one? It might be too late for 2.1 though. Peter
(CC'ing Peter Maydell for his thoughts) On (Tue) 08 Jul 2014 [22:55:42], Peter Lieven wrote: > Hi Juan, > > Am 25.06.2014 um 13:55 schrieb Juan Quintela <quintela@redhat.com>: > > > Peter Lieven <pl@kamp.de> wrote: > >> this patch extends commit db80fac by not only checking > >> for unknown flags, but also filtering out unknown flag > >> combinations. > >> > >> Suggested-by: Eric Blake <eblake@redhat.com> > >> Signed-off-by: Peter Lieven <pl@kamp.de> > > > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > > > Will be on next pull request, thanks. > > > > Have you forgotten to pull this one? It might be too late for 2.1 though. Juan is away for a couple of weeks. This looks like a good fix to pull in for 2.1, though. Peter, do you agree? Can you pick this up if so? Thanks, Amit
On 9 July 2014 05:25, Amit Shah <amit.shah@redhat.com> wrote: > (CC'ing Peter Maydell for his thoughts) > > On (Tue) 08 Jul 2014 [22:55:42], Peter Lieven wrote: >> Hi Juan, >> >> Am 25.06.2014 um 13:55 schrieb Juan Quintela <quintela@redhat.com>: >> >> > Peter Lieven <pl@kamp.de> wrote: >> >> this patch extends commit db80fac by not only checking >> >> for unknown flags, but also filtering out unknown flag >> >> combinations. >> >> >> >> Suggested-by: Eric Blake <eblake@redhat.com> >> >> Signed-off-by: Peter Lieven <pl@kamp.de> >> > >> > Reviewed-by: Juan Quintela <quintela@redhat.com> >> > >> > Will be on next pull request, thanks. >> > >> >> Have you forgotten to pull this one? It might be too late for 2.1 though. > > Juan is away for a couple of weeks. This looks like a good fix to > pull in for 2.1, though. Peter, do you agree? Can you pick this up > if so? What's the bug it's fixing? I had a look at the commit message, but that suggests it's just tightening up sanity checking, not fixing an actual issue... Maybe you can clarify. thanks -- PMM
On (Wed) 09 Jul 2014 [11:28:27], Peter Maydell wrote: > On 9 July 2014 05:25, Amit Shah <amit.shah@redhat.com> wrote: > > (CC'ing Peter Maydell for his thoughts) > > > > On (Tue) 08 Jul 2014 [22:55:42], Peter Lieven wrote: > >> Hi Juan, > >> > >> Am 25.06.2014 um 13:55 schrieb Juan Quintela <quintela@redhat.com>: > >> > >> > Peter Lieven <pl@kamp.de> wrote: > >> >> this patch extends commit db80fac by not only checking > >> >> for unknown flags, but also filtering out unknown flag > >> >> combinations. > >> >> > >> >> Suggested-by: Eric Blake <eblake@redhat.com> > >> >> Signed-off-by: Peter Lieven <pl@kamp.de> > >> > > >> > Reviewed-by: Juan Quintela <quintela@redhat.com> > >> > > >> > Will be on next pull request, thanks. > >> > > >> > >> Have you forgotten to pull this one? It might be too late for 2.1 though. > > > > Juan is away for a couple of weeks. This looks like a good fix to > > pull in for 2.1, though. Peter, do you agree? Can you pick this up > > if so? > > What's the bug it's fixing? I had a look at the commit message, > but that suggests it's just tightening up sanity checking, not > fixing an actual issue... Maybe you can clarify. Right, it improves correctness: after this patch, we ensure a rogue or corrupt migration stream cannot cause problems on the dest. Amit
On 9 July 2014 11:44, Amit Shah <amit.shah@redhat.com> wrote: > On (Wed) 09 Jul 2014 [11:28:27], Peter Maydell wrote: >> On 9 July 2014 05:25, Amit Shah <amit.shah@redhat.com> wrote: >> > Juan is away for a couple of weeks. This looks like a good fix to >> > pull in for 2.1, though. Peter, do you agree? Can you pick this up >> > if so? >> >> What's the bug it's fixing? I had a look at the commit message, >> but that suggests it's just tightening up sanity checking, not >> fixing an actual issue... Maybe you can clarify. > > Right, it improves correctness: after this patch, we ensure a rogue or > corrupt migration stream cannot cause problems on the dest. OK; we're treating those as bugs so yes, I think this is 2.1 material. Has somebody other than the original author tested it? (That's a step that would usually be done by Juan as the maintainer.) If somebody can provide a Tested-by: I'm happy to apply it to master. thanks -- PMM
On (Wed) 09 Jul 2014 [11:50:18], Peter Maydell wrote: > On 9 July 2014 11:44, Amit Shah <amit.shah@redhat.com> wrote: > > On (Wed) 09 Jul 2014 [11:28:27], Peter Maydell wrote: > >> On 9 July 2014 05:25, Amit Shah <amit.shah@redhat.com> wrote: > >> > Juan is away for a couple of weeks. This looks like a good fix to > >> > pull in for 2.1, though. Peter, do you agree? Can you pick this up > >> > if so? > >> > >> What's the bug it's fixing? I had a look at the commit message, > >> but that suggests it's just tightening up sanity checking, not > >> fixing an actual issue... Maybe you can clarify. > > > > Right, it improves correctness: after this patch, we ensure a rogue or > > corrupt migration stream cannot cause problems on the dest. > > OK; we're treating those as bugs so yes, I think this is 2.1 > material. Has somebody other than the original author tested > it? (That's a step that would usually be done by Juan as the > maintainer.) If somebody can provide a Tested-by: I'm happy > to apply it to master. Not really sure if Juan did that as part of his 'thanks, applied' workflow, but I'll run this through the autotest migration tests and report back. Amit
On 9 July 2014 11:56, Amit Shah <amit.shah@redhat.com> wrote: > On (Wed) 09 Jul 2014 [11:50:18], Peter Maydell wrote: >> OK; we're treating those as bugs so yes, I think this is 2.1 >> material. Has somebody other than the original author tested >> it? (That's a step that would usually be done by Juan as the >> maintainer.) If somebody can provide a Tested-by: I'm happy >> to apply it to master. > > Not really sure if Juan did that as part of his 'thanks, applied' > workflow, but I'll run this through the autotest migration tests and > report back. Thanks; it seems better to double-check given where we are in the release cycle. -- PMM
Am 09.07.2014 13:00, schrieb Peter Maydell: > On 9 July 2014 11:56, Amit Shah <amit.shah@redhat.com> wrote: >> On (Wed) 09 Jul 2014 [11:50:18], Peter Maydell wrote: >>> OK; we're treating those as bugs so yes, I think this is 2.1 >>> material. Has somebody other than the original author tested >>> it? (That's a step that would usually be done by Juan as the >>> maintainer.) If somebody can provide a Tested-by: I'm happy >>> to apply it to master. >> Not really sure if Juan did that as part of his 'thanks, applied' >> workflow, but I'll run this through the autotest migration tests and >> report back. > Thanks; it seems better to double-check given where we are in > the release cycle. We have also an: Reviewed-by: Eric Blake <eblake@redhat.com> Maybe he tested as well. If there are any doubts we should postpone this for 2.2. The preceding patch is in 2.1 and should catch already a lot of cases. Peter
On 07/09/2014 07:23 AM, Peter Lieven wrote: > Am 09.07.2014 13:00, schrieb Peter Maydell: >> On 9 July 2014 11:56, Amit Shah <amit.shah@redhat.com> wrote: >>> On (Wed) 09 Jul 2014 [11:50:18], Peter Maydell wrote: >>>> OK; we're treating those as bugs so yes, I think this is 2.1 >>>> material. Has somebody other than the original author tested >>>> it? (That's a step that would usually be done by Juan as the >>>> maintainer.) If somebody can provide a Tested-by: I'm happy >>>> to apply it to master. >>> Not really sure if Juan did that as part of his 'thanks, applied' >>> workflow, but I'll run this through the autotest migration tests and >>> report back. >> Thanks; it seems better to double-check given where we are in >> the release cycle. > We have also an: > > Reviewed-by: Eric Blake <eblake@redhat.com> > > > Maybe he tested as well. No, while I did a review, and can even do a compile test, I have not done a migration test, so I'm not comfortable with a Tested-by yet. Is it something you need me to try today? > > If there are any doubts we should postpone this for 2.2. The preceding > patch is in 2.1 and should catch already a lot of cases. I'm 50-50 on whether this is 2.1 material - it is a bug fix for hardening against malicious incoming migration streams, but without testing that it doesn't break normal migration, it is rather late in the game.
On (Wed) 09 Jul 2014 [12:00:13], Peter Maydell wrote: > On 9 July 2014 11:56, Amit Shah <amit.shah@redhat.com> wrote: > > On (Wed) 09 Jul 2014 [11:50:18], Peter Maydell wrote: > >> OK; we're treating those as bugs so yes, I think this is 2.1 > >> material. Has somebody other than the original author tested > >> it? (That's a step that would usually be done by Juan as the > >> maintainer.) If somebody can provide a Tested-by: I'm happy > >> to apply it to master. > > > > Not really sure if Juan did that as part of his 'thanks, applied' > > workflow, but I'll run this through the autotest migration tests and > > report back. > > Thanks; it seems better to double-check given where we are in > the release cycle. I tested using the autotest migration tests, which start a guest, migrate it, and then login to the guest to see if everything's fine. I used qemu-2.0 and qemu.git against qemu.git+patch and both tests passed fine. But since Peter and Eric have reservations, let's just postpone this till 2.2. That saves me from further tests as well! Amit
Am 11.07.2014 um 14:52 schrieb Amit Shah <amit.shah@redhat.com>: > On (Wed) 09 Jul 2014 [12:00:13], Peter Maydell wrote: >> On 9 July 2014 11:56, Amit Shah <amit.shah@redhat.com> wrote: >>> On (Wed) 09 Jul 2014 [11:50:18], Peter Maydell wrote: >>>> OK; we're treating those as bugs so yes, I think this is 2.1 >>>> material. Has somebody other than the original author tested >>>> it? (That's a step that would usually be done by Juan as the >>>> maintainer.) If somebody can provide a Tested-by: I'm happy >>>> to apply it to master. >>> >>> Not really sure if Juan did that as part of his 'thanks, applied' >>> workflow, but I'll run this through the autotest migration tests and >>> report back. >> >> Thanks; it seems better to double-check given where we are in >> the release cycle. > > I tested using the autotest migration tests, which start a guest, > migrate it, and then login to the guest to see if everything's fine. > > I used qemu-2.0 and qemu.git against qemu.git+patch and both tests > passed fine. > > But since Peter and Eric have reservations, let's just postpone this > till 2.2. That saves me from further tests as well! Since 2.1 is out meanwhile can you please send a pull request for this patch now? Thanks, Peter
On (Thu) 21 Aug 2014 [23:57:46], Peter Lieven wrote: > > Am 11.07.2014 um 14:52 schrieb Amit Shah <amit.shah@redhat.com>: > > > On (Wed) 09 Jul 2014 [12:00:13], Peter Maydell wrote: > >> On 9 July 2014 11:56, Amit Shah <amit.shah@redhat.com> wrote: > >>> On (Wed) 09 Jul 2014 [11:50:18], Peter Maydell wrote: > >>>> OK; we're treating those as bugs so yes, I think this is 2.1 > >>>> material. Has somebody other than the original author tested > >>>> it? (That's a step that would usually be done by Juan as the > >>>> maintainer.) If somebody can provide a Tested-by: I'm happy > >>>> to apply it to master. > >>> > >>> Not really sure if Juan did that as part of his 'thanks, applied' > >>> workflow, but I'll run this through the autotest migration tests and > >>> report back. > >> > >> Thanks; it seems better to double-check given where we are in > >> the release cycle. > > > > I tested using the autotest migration tests, which start a guest, > > migrate it, and then login to the guest to see if everything's fine. > > > > I used qemu-2.0 and qemu.git against qemu.git+patch and both tests > > passed fine. > > > > But since Peter and Eric have reservations, let's just postpone this > > till 2.2. That saves me from further tests as well! > > Since 2.1 is out meanwhile can you please send a pull request for this > patch now? Juan is back, I'll let him pick it through his tree. Thanks, Amit
On 22.08.2014 05:57, Amit Shah wrote: > On (Thu) 21 Aug 2014 [23:57:46], Peter Lieven wrote: >> Am 11.07.2014 um 14:52 schrieb Amit Shah <amit.shah@redhat.com>: >> >>> On (Wed) 09 Jul 2014 [12:00:13], Peter Maydell wrote: >>>> On 9 July 2014 11:56, Amit Shah <amit.shah@redhat.com> wrote: >>>>> On (Wed) 09 Jul 2014 [11:50:18], Peter Maydell wrote: >>>>>> OK; we're treating those as bugs so yes, I think this is 2.1 >>>>>> material. Has somebody other than the original author tested >>>>>> it? (That's a step that would usually be done by Juan as the >>>>>> maintainer.) If somebody can provide a Tested-by: I'm happy >>>>>> to apply it to master. >>>>> Not really sure if Juan did that as part of his 'thanks, applied' >>>>> workflow, but I'll run this through the autotest migration tests and >>>>> report back. >>>> Thanks; it seems better to double-check given where we are in >>>> the release cycle. >>> I tested using the autotest migration tests, which start a guest, >>> migrate it, and then login to the guest to see if everything's fine. >>> >>> I used qemu-2.0 and qemu.git against qemu.git+patch and both tests >>> passed fine. >>> >>> But since Peter and Eric have reservations, let's just postpone this >>> till 2.2. That saves me from further tests as well! >> Since 2.1 is out meanwhile can you please send a pull request for this >> patch now? > Juan is back, I'll let him pick it through his tree. Juan, have you picked this up? Thanks, Peter
Il 02/09/2014 11:17, Peter Lieven ha scritto: >>> >> Juan is back, I'll let him pick it through his tree. > > Juan, have you picked this up? commit db80facefa62dff42bb50c73b0f03eda5f732b49 Author: Peter Lieven <pl@kamp.de> Date: Tue Jun 10 11:29:16 2014 +0200 migration: catch unknown flags in ram_load if a saved vm has unknown flags in the memory data qemu currently simply ignores this flag and continues which yields in an unpredictable result. This patch catches all unknown flags and aborts the loading of the vm. Additionally error reports are thrown if the migration aborts abnormally. Signed-off-by: Peter Lieven <pl@kamp.de> Signed-off-by: Juan Quintela <quintela@redhat.com> Paolo
Il 23/09/2014 11:46, Paolo Bonzini ha scritto: > Il 02/09/2014 11:17, Peter Lieven ha scritto: >>>> >>> Juan is back, I'll let him pick it through his tree. >> >> Juan, have you picked this up? > > commit db80facefa62dff42bb50c73b0f03eda5f732b49 > Author: Peter Lieven <pl@kamp.de> > Date: Tue Jun 10 11:29:16 2014 +0200 > > migration: catch unknown flags in ram_load > > if a saved vm has unknown flags in the memory data qemu > currently simply ignores this flag and continues which > yields in an unpredictable result. > > This patch catches all unknown flags and aborts the > loading of the vm. Additionally error reports are thrown > if the migration aborts abnormally. > > Signed-off-by: Peter Lieven <pl@kamp.de> > Signed-off-by: Juan Quintela <quintela@redhat.com> Never mind, this is a different patch. Paolo
On 23.09.2014 11:51, Paolo Bonzini wrote: > Il 23/09/2014 11:46, Paolo Bonzini ha scritto: >> Il 02/09/2014 11:17, Peter Lieven ha scritto: >>>> Juan is back, I'll let him pick it through his tree. >>> Juan, have you picked this up? >> commit db80facefa62dff42bb50c73b0f03eda5f732b49 >> Author: Peter Lieven <pl@kamp.de> >> Date: Tue Jun 10 11:29:16 2014 +0200 >> >> migration: catch unknown flags in ram_load >> >> if a saved vm has unknown flags in the memory data qemu >> currently simply ignores this flag and continues which >> yields in an unpredictable result. >> >> This patch catches all unknown flags and aborts the >> loading of the vm. Additionally error reports are thrown >> if the migration aborts abnormally. >> >> Signed-off-by: Peter Lieven <pl@kamp.de> >> Signed-off-by: Juan Quintela <quintela@redhat.com> > Never mind, this is a different patch. However, the diferent patch is still missing. Juan, can you pick up [PATCH] migration: catch unknown flag combinations in ram_load please. Thank you, Peter
diff --git a/arch_init.c b/arch_init.c index 8ddaf35..fb06d07 100644 --- a/arch_init.c +++ b/arch_init.c @@ -1038,8 +1038,7 @@ void ram_handle_compressed(void *host, uint8_t ch, uint64_t size) static int ram_load(QEMUFile *f, void *opaque, int version_id) { - ram_addr_t addr; - int flags, ret = 0; + int flags = 0, ret = 0; static uint64_t seq_iter; seq_iter++; @@ -1048,21 +1047,24 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) ret = -EINVAL; } - while (!ret) { - addr = qemu_get_be64(f); + while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) { + ram_addr_t addr, total_ram_bytes; + void *host; + uint8_t ch; + addr = qemu_get_be64(f); flags = addr & ~TARGET_PAGE_MASK; addr &= TARGET_PAGE_MASK; - if (flags & RAM_SAVE_FLAG_MEM_SIZE) { + switch (flags & ~RAM_SAVE_FLAG_CONTINUE) { + case RAM_SAVE_FLAG_MEM_SIZE: /* Synchronize RAM block list */ - char id[256]; - ram_addr_t length; - ram_addr_t total_ram_bytes = addr; - - while (total_ram_bytes) { + total_ram_bytes = addr; + while (!ret && total_ram_bytes) { RAMBlock *block; uint8_t len; + char id[256]; + ram_addr_t length; len = qemu_get_byte(f); qemu_get_buffer(f, (uint8_t *)id, len); @@ -1086,16 +1088,11 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) "accept migration", id); ret = -EINVAL; } - if (ret) { - break; - } total_ram_bytes -= length; } - } else if (flags & RAM_SAVE_FLAG_COMPRESS) { - void *host; - uint8_t ch; - + break; + case RAM_SAVE_FLAG_COMPRESS: host = host_from_stream_offset(f, addr, flags); if (!host) { error_report("Illegal RAM offset " RAM_ADDR_FMT, addr); @@ -1105,9 +1102,8 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) ch = qemu_get_byte(f); ram_handle_compressed(host, ch, TARGET_PAGE_SIZE); - } else if (flags & RAM_SAVE_FLAG_PAGE) { - void *host; - + break; + case RAM_SAVE_FLAG_PAGE: host = host_from_stream_offset(f, addr, flags); if (!host) { error_report("Illegal RAM offset " RAM_ADDR_FMT, addr); @@ -1116,8 +1112,9 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) } qemu_get_buffer(f, host, TARGET_PAGE_SIZE); - } else if (flags & RAM_SAVE_FLAG_XBZRLE) { - void *host = host_from_stream_offset(f, addr, flags); + break; + case RAM_SAVE_FLAG_XBZRLE: + host = host_from_stream_offset(f, addr, flags); if (!host) { error_report("Illegal RAM offset " RAM_ADDR_FMT, addr); ret = -EINVAL; @@ -1130,17 +1127,22 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) ret = -EINVAL; break; } - } else if (flags & RAM_SAVE_FLAG_HOOK) { - ram_control_load_hook(f, flags); - } else if (flags & RAM_SAVE_FLAG_EOS) { - /* normal exit */ break; - } else { - error_report("Unknown migration flags: %#x", flags); - ret = -EINVAL; + case RAM_SAVE_FLAG_EOS: + /* normal exit */ break; + default: + if (flags & RAM_SAVE_FLAG_HOOK) { + ram_control_load_hook(f, flags); + } else { + error_report("Unknown combination of migration flags: %#x", + flags); + ret = -EINVAL; + } + } + if (!ret) { + ret = qemu_file_get_error(f); } - ret = qemu_file_get_error(f); } DPRINTF("Completed load of VM with exit code %d seq iteration "
this patch extends commit db80fac by not only checking for unknown flags, but also filtering out unknown flag combinations. Suggested-by: Eric Blake <eblake@redhat.com> Signed-off-by: Peter Lieven <pl@kamp.de> --- arch_init.c | 62 ++++++++++++++++++++++++++++++----------------------------- 1 file changed, 32 insertions(+), 30 deletions(-)