| Message ID | 20190325174706.6741-6-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/15] multifd: Only send pages when packet are not empty | expand |
On Mon, 25 Mar 2019 at 18:13, Juan Quintela <quintela@redhat.com> wrote: > > This way we can change the packet size in the future and everything > will work. We choose an arbitrary big number (100 times configured > size) as a limit about how big we will reallocate. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Signed-off-by: Juan Quintela <quintela@redhat.com> > -- Hi; Coverity reports a use-after-free in this code (CID 1400442): > @@ -832,12 +832,24 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp) > p->flags = be32_to_cpu(packet->flags); > > packet->pages_alloc = be32_to_cpu(packet->pages_alloc); > - if (packet->pages_alloc > page_count) { > + /* > + * If we recevied a packet that is 100 times bigger than expected > + * just stop migration. It is a magic number. > + */ > + if (packet->pages_alloc > pages_max * 100) { > error_setg(errp, "multifd: received packet " > - "with size %d and expected maximum size %d", > - packet->pages_alloc, page_count) ; > + "with size %d and expected a maximum size of %d", > + packet->pages_alloc, pages_max * 100) ; > return -1; > } > + /* > + * We received a packet that is bigger than expected but inside > + * reasonable limits (see previous comment). Just reallocate. > + */ > + if (packet->pages_alloc > p->pages->allocated) { > + multifd_pages_clear(p->pages); multifd_pages_clear() calls g_free() on the pointer it is passed... > + multifd_pages_init(packet->pages_alloc); > + } > > p->pages->used = be32_to_cpu(packet->pages_used); ...but here we fall through and dereference p->pages, which we might have just freed. > if (p->pages->used > packet->pages_alloc) { > -- > 2.20.1 thanks -- PMM
On Tue, 26 Mar 2019 at 12:26, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 25 Mar 2019 at 18:13, Juan Quintela <quintela@redhat.com> wrote: > > > > This way we can change the packet size in the future and everything > > will work. We choose an arbitrary big number (100 times configured > > size) as a limit about how big we will reallocate. > > > > Signed-off-by: Juan Quintela <quintela@redhat.com> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Signed-off-by: Juan Quintela <quintela@redhat.com> > > -- > > Hi; Coverity reports a use-after-free in this code > (CID 1400442): Ping? Have we got a fix for this yet ? thanks -- PMM > > > @@ -832,12 +832,24 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp) > > p->flags = be32_to_cpu(packet->flags); > > > > packet->pages_alloc = be32_to_cpu(packet->pages_alloc); > > - if (packet->pages_alloc > page_count) { > > + /* > > + * If we recevied a packet that is 100 times bigger than expected > > + * just stop migration. It is a magic number. > > + */ > > + if (packet->pages_alloc > pages_max * 100) { > > error_setg(errp, "multifd: received packet " > > - "with size %d and expected maximum size %d", > > - packet->pages_alloc, page_count) ; > > + "with size %d and expected a maximum size of %d", > > + packet->pages_alloc, pages_max * 100) ; > > return -1; > > } > > + /* > > + * We received a packet that is bigger than expected but inside > > + * reasonable limits (see previous comment). Just reallocate. > > + */ > > + if (packet->pages_alloc > p->pages->allocated) { > > + multifd_pages_clear(p->pages); > > multifd_pages_clear() calls g_free() on the pointer it is passed... > > > + multifd_pages_init(packet->pages_alloc); > > + } > > > > p->pages->used = be32_to_cpu(packet->pages_used); > > ...but here we fall through and dereference p->pages, which > we might have just freed. > > > if (p->pages->used > packet->pages_alloc) { > > -- > > 2.20.1 > > thanks > -- PMM
diff --git a/migration/ram.c b/migration/ram.c index 454d3eb539..77c1878292 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -786,13 +786,13 @@ static void multifd_pages_clear(MultiFDPages_t *pages) static void multifd_send_fill_packet(MultiFDSendParams *p) { MultiFDPacket_t *packet = p->packet; - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); + uint32_t page_max = MULTIFD_PACKET_SIZE / qemu_target_page_size(); int i; packet->magic = cpu_to_be32(MULTIFD_MAGIC); packet->version = cpu_to_be32(MULTIFD_VERSION); packet->flags = cpu_to_be32(p->flags); - packet->pages_alloc = cpu_to_be32(page_count); + packet->pages_alloc = cpu_to_be32(page_max); packet->pages_used = cpu_to_be32(p->pages->used); packet->next_packet_size = cpu_to_be32(p->next_packet_size); packet->packet_num = cpu_to_be64(p->packet_num); @@ -809,7 +809,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p) static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp) { MultiFDPacket_t *packet = p->packet; - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); + uint32_t pages_max = MULTIFD_PACKET_SIZE / qemu_target_page_size(); RAMBlock *block; int i; @@ -832,12 +832,24 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp) p->flags = be32_to_cpu(packet->flags); packet->pages_alloc = be32_to_cpu(packet->pages_alloc); - if (packet->pages_alloc > page_count) { + /* + * If we recevied a packet that is 100 times bigger than expected + * just stop migration. It is a magic number. + */ + if (packet->pages_alloc > pages_max * 100) { error_setg(errp, "multifd: received packet " - "with size %d and expected maximum size %d", - packet->pages_alloc, page_count) ; + "with size %d and expected a maximum size of %d", + packet->pages_alloc, pages_max * 100) ; return -1; } + /* + * We received a packet that is bigger than expected but inside + * reasonable limits (see previous comment). Just reallocate. + */ + if (packet->pages_alloc > p->pages->allocated) { + multifd_pages_clear(p->pages); + multifd_pages_init(packet->pages_alloc); + } p->pages->used = be32_to_cpu(packet->pages_used); if (p->pages->used > packet->pages_alloc) {