Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/5] virtio-balloon: Corrections to address verification
On Wed, 27 Feb 2019 10:20:12 +1100 David Gibson wrote: > On Mon, Feb 25, 2019 at 10:26:51AM +0100, Greg Kurz wrote: > > On Mon, 25 Feb 2019 10:37:11 +1100 > > David Gibson wrote: > > > > > On Fri, Feb 22, 2019 at 10:08:22AM +0100, Greg Kurz wrote: > > > > On Thu, 14 Feb 2019 15:39:13 +1100 > > > > David Gibson wrote: > > > > > > > > > The virtio-balloon device's verification of the address given to it > > > > > by the > > > > > guest has a number of faults: > > > > > * The addresses here are guest physical addresses, which should be > > > > > 'hwaddr' rather than 'ram_addr_t' (the distinction is admittedly > > > > > pretty subtle and confusing) > > > > > * We don't check for section.mr being NULL, which is the main way > > > > > that > > > > > memory_region_find() reports basic failures. We really need to > > > > > check > > > > > that before looking at any other section fields, because > > > > > memory_region_find() doesn't initialize them on the failure path > > > > > * We're passing a length of '1' to memory_region_find(), but > > > > > really the > > > > > guest is requesting that we put the entire page into the > > > > > balloon, > > > > > so it makes more sense to call it with BALLOON_PAGE_SIZE > > > > > > > > > > Signed-off-by: David Gibson > > > > > Reviewed-by: David Hildenbrand > > > > > Reviewed-by: Michael S. Tsirkin > > > > > --- > > > > > hw/virtio/virtio-balloon.c | 17 ++--- > > > > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > > > index 43af521884..eb357824d8 100644 > > > > > --- a/hw/virtio/virtio-balloon.c > > > > > +++ b/hw/virtio/virtio-balloon.c > > > > > @@ -221,17 +221,20 @@ static void > > > > > virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > > > } > > > > > > > > > > while (iov_to_buf(elem->out_sg, elem->out_num, offset, , > > > > > 4) == 4) { > > > > > -ram_addr_t pa; > > > > > -ram_addr_t addr; > > > > > +hwaddr pa; > > > > > +hwaddr addr; > > > > > int p = virtio_ldl_p(vdev, ); > > > > > > > > > > -pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT; > > > > > +pa = (hwaddr) p << VIRTIO_BALLOON_PFN_SHIFT; > > > > > offset += 4; > > > > > > > > > > -/* FIXME: remove get_system_memory(), but how? */ > > > > > -section = memory_region_find(get_system_memory(), pa, 1); > > > > > -if (!int128_nz(section.size) || > > > > > -!memory_region_is_ram(section.mr) || > > > > > +section = memory_region_find(get_system_memory(), pa, > > > > > + BALLOON_PAGE_SIZE); > > > > > +if (!section.mr) { > > > > > +trace_virtio_balloon_bad_addr(pa); > > > > > +continue; > > > > > +} > > > > > > > > memory_region_unref(section.mr) with section.mr == NULL is safe and > > > > resolves to a nop. Not sure you need a separate if to handle this > > > > case. > > > > > > memory_region_is_ram() and friends are not, however - they will > > > dereference their argument unconditionally. > > > > > > > Indeed but the two ifs can be merged anyway: > > > > if (!section.mr || > > !memory_region_is_ram(section.mr) || > > memory_region_is_rom(section.mr) || > > memory_region_is_romd(section.mr)) { > > trace_virtio_balloon_bad_addr(pa); > > memory_region_unref(section.mr); > > continue; > > } > > Oh, I see what you mean. Hrm, I still kind of prefer visually > separating the validity check from tests which depend on that validity. > Makes sense. It's alright then :) > > > > > > > > > > Apart from that, > > > > > > > > Reviewed-by: Greg Kurz > > > > > > > > > +if (!memory_region_is_ram(section.mr) || > > > > > memory_region_is_rom(section.mr) || > > > > > memory_region_is_romd(section.mr)) { > > > > > trace_virtio_balloon_bad_addr(pa); > > > > > > > > > > > > pgpSCIyGmBsBZ.pgp Description: OpenPGP digital signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/5] virtio-balloon: Corrections to address verification
On Mon, Feb 25, 2019 at 10:26:51AM +0100, Greg Kurz wrote: > On Mon, 25 Feb 2019 10:37:11 +1100 > David Gibson wrote: > > > On Fri, Feb 22, 2019 at 10:08:22AM +0100, Greg Kurz wrote: > > > On Thu, 14 Feb 2019 15:39:13 +1100 > > > David Gibson wrote: > > > > > > > The virtio-balloon device's verification of the address given to it by > > > > the > > > > guest has a number of faults: > > > > * The addresses here are guest physical addresses, which should be > > > > 'hwaddr' rather than 'ram_addr_t' (the distinction is admittedly > > > > pretty subtle and confusing) > > > > * We don't check for section.mr being NULL, which is the main way > > > > that > > > > memory_region_find() reports basic failures. We really need to > > > > check > > > > that before looking at any other section fields, because > > > > memory_region_find() doesn't initialize them on the failure path > > > > * We're passing a length of '1' to memory_region_find(), but really > > > > the > > > > guest is requesting that we put the entire page into the balloon, > > > > so it makes more sense to call it with BALLOON_PAGE_SIZE > > > > > > > > Signed-off-by: David Gibson > > > > Reviewed-by: David Hildenbrand > > > > Reviewed-by: Michael S. Tsirkin > > > > --- > > > > hw/virtio/virtio-balloon.c | 17 ++--- > > > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > > index 43af521884..eb357824d8 100644 > > > > --- a/hw/virtio/virtio-balloon.c > > > > +++ b/hw/virtio/virtio-balloon.c > > > > @@ -221,17 +221,20 @@ static void > > > > virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > > } > > > > > > > > while (iov_to_buf(elem->out_sg, elem->out_num, offset, , > > > > 4) == 4) { > > > > -ram_addr_t pa; > > > > -ram_addr_t addr; > > > > +hwaddr pa; > > > > +hwaddr addr; > > > > int p = virtio_ldl_p(vdev, ); > > > > > > > > -pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT; > > > > +pa = (hwaddr) p << VIRTIO_BALLOON_PFN_SHIFT; > > > > offset += 4; > > > > > > > > -/* FIXME: remove get_system_memory(), but how? */ > > > > -section = memory_region_find(get_system_memory(), pa, 1); > > > > -if (!int128_nz(section.size) || > > > > -!memory_region_is_ram(section.mr) || > > > > +section = memory_region_find(get_system_memory(), pa, > > > > + BALLOON_PAGE_SIZE); > > > > +if (!section.mr) { > > > > +trace_virtio_balloon_bad_addr(pa); > > > > +continue; > > > > +} > > > > > > memory_region_unref(section.mr) with section.mr == NULL is safe and > > > resolves to a nop. Not sure you need a separate if to handle this > > > case. > > > > memory_region_is_ram() and friends are not, however - they will > > dereference their argument unconditionally. > > > > Indeed but the two ifs can be merged anyway: > > if (!section.mr || > !memory_region_is_ram(section.mr) || > memory_region_is_rom(section.mr) || > memory_region_is_romd(section.mr)) { > trace_virtio_balloon_bad_addr(pa); > memory_region_unref(section.mr); > continue; > } Oh, I see what you mean. Hrm, I still kind of prefer visually separating the validity check from tests which depend on that validity. > > > > > > > Apart from that, > > > > > > Reviewed-by: Greg Kurz > > > > > > > +if (!memory_region_is_ram(section.mr) || > > > > memory_region_is_rom(section.mr) || > > > > memory_region_is_romd(section.mr)) { > > > > trace_virtio_balloon_bad_addr(pa); > > > > > > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/5] virtio-balloon: Corrections to address verification
On Mon, 25 Feb 2019 10:37:11 +1100 David Gibson wrote: > On Fri, Feb 22, 2019 at 10:08:22AM +0100, Greg Kurz wrote: > > On Thu, 14 Feb 2019 15:39:13 +1100 > > David Gibson wrote: > > > > > The virtio-balloon device's verification of the address given to it by the > > > guest has a number of faults: > > > * The addresses here are guest physical addresses, which should be > > > 'hwaddr' rather than 'ram_addr_t' (the distinction is admittedly > > > pretty subtle and confusing) > > > * We don't check for section.mr being NULL, which is the main way that > > > memory_region_find() reports basic failures. We really need to > > > check > > > that before looking at any other section fields, because > > > memory_region_find() doesn't initialize them on the failure path > > > * We're passing a length of '1' to memory_region_find(), but really > > > the > > > guest is requesting that we put the entire page into the balloon, > > > so it makes more sense to call it with BALLOON_PAGE_SIZE > > > > > > Signed-off-by: David Gibson > > > Reviewed-by: David Hildenbrand > > > Reviewed-by: Michael S. Tsirkin > > > --- > > > hw/virtio/virtio-balloon.c | 17 ++--- > > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > index 43af521884..eb357824d8 100644 > > > --- a/hw/virtio/virtio-balloon.c > > > +++ b/hw/virtio/virtio-balloon.c > > > @@ -221,17 +221,20 @@ static void > > > virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > } > > > > > > while (iov_to_buf(elem->out_sg, elem->out_num, offset, , 4) > > > == 4) { > > > -ram_addr_t pa; > > > -ram_addr_t addr; > > > +hwaddr pa; > > > +hwaddr addr; > > > int p = virtio_ldl_p(vdev, ); > > > > > > -pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT; > > > +pa = (hwaddr) p << VIRTIO_BALLOON_PFN_SHIFT; > > > offset += 4; > > > > > > -/* FIXME: remove get_system_memory(), but how? */ > > > -section = memory_region_find(get_system_memory(), pa, 1); > > > -if (!int128_nz(section.size) || > > > -!memory_region_is_ram(section.mr) || > > > +section = memory_region_find(get_system_memory(), pa, > > > + BALLOON_PAGE_SIZE); > > > +if (!section.mr) { > > > +trace_virtio_balloon_bad_addr(pa); > > > +continue; > > > +} > > > > memory_region_unref(section.mr) with section.mr == NULL is safe and > > resolves to a nop. Not sure you need a separate if to handle this > > case. > > memory_region_is_ram() and friends are not, however - they will > dereference their argument unconditionally. > Indeed but the two ifs can be merged anyway: if (!section.mr || !memory_region_is_ram(section.mr) || memory_region_is_rom(section.mr) || memory_region_is_romd(section.mr)) { trace_virtio_balloon_bad_addr(pa); memory_region_unref(section.mr); continue; } > > > > Apart from that, > > > > Reviewed-by: Greg Kurz > > > > > +if (!memory_region_is_ram(section.mr) || > > > memory_region_is_rom(section.mr) || > > > memory_region_is_romd(section.mr)) { > > > trace_virtio_balloon_bad_addr(pa); > > > pgpZTR1oTRNzj.pgp Description: OpenPGP digital signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/5] virtio-balloon: Corrections to address verification
On Fri, Feb 22, 2019 at 10:08:22AM +0100, Greg Kurz wrote: > On Thu, 14 Feb 2019 15:39:13 +1100 > David Gibson wrote: > > > The virtio-balloon device's verification of the address given to it by the > > guest has a number of faults: > > * The addresses here are guest physical addresses, which should be > > 'hwaddr' rather than 'ram_addr_t' (the distinction is admittedly > > pretty subtle and confusing) > > * We don't check for section.mr being NULL, which is the main way that > > memory_region_find() reports basic failures. We really need to check > > that before looking at any other section fields, because > > memory_region_find() doesn't initialize them on the failure path > > * We're passing a length of '1' to memory_region_find(), but really the > > guest is requesting that we put the entire page into the balloon, > > so it makes more sense to call it with BALLOON_PAGE_SIZE > > > > Signed-off-by: David Gibson > > Reviewed-by: David Hildenbrand > > Reviewed-by: Michael S. Tsirkin > > --- > > hw/virtio/virtio-balloon.c | 17 ++--- > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > index 43af521884..eb357824d8 100644 > > --- a/hw/virtio/virtio-balloon.c > > +++ b/hw/virtio/virtio-balloon.c > > @@ -221,17 +221,20 @@ static void virtio_balloon_handle_output(VirtIODevice > > *vdev, VirtQueue *vq) > > } > > > > while (iov_to_buf(elem->out_sg, elem->out_num, offset, , 4) == > > 4) { > > -ram_addr_t pa; > > -ram_addr_t addr; > > +hwaddr pa; > > +hwaddr addr; > > int p = virtio_ldl_p(vdev, ); > > > > -pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT; > > +pa = (hwaddr) p << VIRTIO_BALLOON_PFN_SHIFT; > > offset += 4; > > > > -/* FIXME: remove get_system_memory(), but how? */ > > -section = memory_region_find(get_system_memory(), pa, 1); > > -if (!int128_nz(section.size) || > > -!memory_region_is_ram(section.mr) || > > +section = memory_region_find(get_system_memory(), pa, > > + BALLOON_PAGE_SIZE); > > +if (!section.mr) { > > +trace_virtio_balloon_bad_addr(pa); > > +continue; > > +} > > memory_region_unref(section.mr) with section.mr == NULL is safe and > resolves to a nop. Not sure you need a separate if to handle this > case. memory_region_is_ram() and friends are not, however - they will dereference their argument unconditionally. > > Apart from that, > > Reviewed-by: Greg Kurz > > > +if (!memory_region_is_ram(section.mr) || > > memory_region_is_rom(section.mr) || > > memory_region_is_romd(section.mr)) { > > trace_virtio_balloon_bad_addr(pa); > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/5] virtio-balloon: Corrections to address verification
On Thu, 14 Feb 2019 15:39:13 +1100 David Gibson wrote: > The virtio-balloon device's verification of the address given to it by the > guest has a number of faults: > * The addresses here are guest physical addresses, which should be > 'hwaddr' rather than 'ram_addr_t' (the distinction is admittedly > pretty subtle and confusing) > * We don't check for section.mr being NULL, which is the main way that > memory_region_find() reports basic failures. We really need to check > that before looking at any other section fields, because > memory_region_find() doesn't initialize them on the failure path > * We're passing a length of '1' to memory_region_find(), but really the > guest is requesting that we put the entire page into the balloon, > so it makes more sense to call it with BALLOON_PAGE_SIZE > > Signed-off-by: David Gibson > Reviewed-by: David Hildenbrand > Reviewed-by: Michael S. Tsirkin > --- > hw/virtio/virtio-balloon.c | 17 ++--- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index 43af521884..eb357824d8 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -221,17 +221,20 @@ static void virtio_balloon_handle_output(VirtIODevice > *vdev, VirtQueue *vq) > } > > while (iov_to_buf(elem->out_sg, elem->out_num, offset, , 4) == > 4) { > -ram_addr_t pa; > -ram_addr_t addr; > +hwaddr pa; > +hwaddr addr; > int p = virtio_ldl_p(vdev, ); > > -pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT; > +pa = (hwaddr) p << VIRTIO_BALLOON_PFN_SHIFT; > offset += 4; > > -/* FIXME: remove get_system_memory(), but how? */ > -section = memory_region_find(get_system_memory(), pa, 1); > -if (!int128_nz(section.size) || > -!memory_region_is_ram(section.mr) || > +section = memory_region_find(get_system_memory(), pa, > + BALLOON_PAGE_SIZE); > +if (!section.mr) { > +trace_virtio_balloon_bad_addr(pa); > +continue; > +} memory_region_unref(section.mr) with section.mr == NULL is safe and resolves to a nop. Not sure you need a separate if to handle this case. Apart from that, Reviewed-by: Greg Kurz > +if (!memory_region_is_ram(section.mr) || > memory_region_is_rom(section.mr) || > memory_region_is_romd(section.mr)) { > trace_virtio_balloon_bad_addr(pa);