| Message ID | 20220106064717.7477-2-raphael.norwitz@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up error handling in libvhost-user memory mapping | expand |
On 06.01.22 07:47, Raphael Norwitz wrote: > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > --- > subprojects/libvhost-user/libvhost-user.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c > index 787f4d2d4f..a6dadeb637 100644 > --- a/subprojects/libvhost-user/libvhost-user.c > +++ b/subprojects/libvhost-user/libvhost-user.c > @@ -801,6 +801,12 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) { > VuDevRegion shadow_regions[VHOST_USER_MAX_RAM_SLOTS] = {}; > VhostUserMemoryRegion m = vmsg->payload.memreg.region, *msg_region = &m; > > + if (vmsg->fd_num != 1 || > + vmsg->size != sizeof(vmsg->payload.memreg)) { > + vu_panic(dev, "VHOST_USER_REM_MEM_REG received multiple regions"); > + return false; > + } > + > DPRINT("Removing region:\n"); > DPRINT(" guest_phys_addr: 0x%016"PRIx64"\n", > msg_region->guest_phys_addr); Reviewed-by: David Hildenbrand <david@redhat.com>
On Thu, Jan 06, 2022 at 06:47:26AM +0000, Raphael Norwitz wrote: > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> Raphael any chance you can add a bit more to commit logs? E.g. what happens right now if you pass more? > --- > subprojects/libvhost-user/libvhost-user.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c > index 787f4d2d4f..a6dadeb637 100644 > --- a/subprojects/libvhost-user/libvhost-user.c > +++ b/subprojects/libvhost-user/libvhost-user.c > @@ -801,6 +801,12 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) { > VuDevRegion shadow_regions[VHOST_USER_MAX_RAM_SLOTS] = {}; > VhostUserMemoryRegion m = vmsg->payload.memreg.region, *msg_region = &m; > > + if (vmsg->fd_num != 1 || > + vmsg->size != sizeof(vmsg->payload.memreg)) { Is there a chance someone is sending larger messages and relying on libvhost-user to ignore padding? > + vu_panic(dev, "VHOST_USER_REM_MEM_REG received multiple regions"); Maybe print the parameters that caused the issue? > + return false; > + } > + > DPRINT("Removing region:\n"); > DPRINT(" guest_phys_addr: 0x%016"PRIx64"\n", > msg_region->guest_phys_addr); > -- > 2.20.1
On Mon, Jan 10, 2022 at 04:36:34AM -0500, Michael S. Tsirkin wrote: > On Thu, Jan 06, 2022 at 06:47:26AM +0000, Raphael Norwitz wrote: > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > > > Raphael any chance you can add a bit more to commit logs? > E.g. what happens right now if you pass more? > Sure - I'll add those details. > > --- > > subprojects/libvhost-user/libvhost-user.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c > > index 787f4d2d4f..a6dadeb637 100644 > > --- a/subprojects/libvhost-user/libvhost-user.c > > +++ b/subprojects/libvhost-user/libvhost-user.c > > @@ -801,6 +801,12 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) { > > VuDevRegion shadow_regions[VHOST_USER_MAX_RAM_SLOTS] = {}; > > VhostUserMemoryRegion m = vmsg->payload.memreg.region, *msg_region = &m; > > > > + if (vmsg->fd_num != 1 || > > + vmsg->size != sizeof(vmsg->payload.memreg)) { > > Is there a chance someone is sending larger messages and relying > on libvhost-user to ignore padding? > Great point - I didn't consider padding. I'll drop the vmsg->size check here. It looks like we are inconsistent with size checking. For example, in vu_set_log_base_exec() we also check the size. Should we make it consistent across the library or am I missing some details about why the padding is not an issue in that case? > > + vu_panic(dev, "VHOST_USER_REM_MEM_REG received multiple regions"); > > Maybe print the parameters that caused the issue? > Ack. > > + return false; > > + } > > + > > DPRINT("Removing region:\n"); > > DPRINT(" guest_phys_addr: 0x%016"PRIx64"\n", > > msg_region->guest_phys_addr); > > -- > > 2.20.1 >
On Mon, Jan 10, 2022 at 07:43:06PM +0000, Raphael Norwitz wrote: > On Mon, Jan 10, 2022 at 04:36:34AM -0500, Michael S. Tsirkin wrote: > > On Thu, Jan 06, 2022 at 06:47:26AM +0000, Raphael Norwitz wrote: > > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > > > > > > Raphael any chance you can add a bit more to commit logs? > > E.g. what happens right now if you pass more? > > > > Sure - I'll add those details. > > > > --- > > > subprojects/libvhost-user/libvhost-user.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c > > > index 787f4d2d4f..a6dadeb637 100644 > > > --- a/subprojects/libvhost-user/libvhost-user.c > > > +++ b/subprojects/libvhost-user/libvhost-user.c > > > @@ -801,6 +801,12 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) { > > > VuDevRegion shadow_regions[VHOST_USER_MAX_RAM_SLOTS] = {}; > > > VhostUserMemoryRegion m = vmsg->payload.memreg.region, *msg_region = &m; > > > > > > + if (vmsg->fd_num != 1 || > > > + vmsg->size != sizeof(vmsg->payload.memreg)) { > > > > Is there a chance someone is sending larger messages and relying > > on libvhost-user to ignore padding? > > > > Great point - I didn't consider padding. I'll drop the vmsg->size check > here. > > It looks like we are inconsistent with size checking. For example, in > vu_set_log_base_exec() we also check the size. > > Should we make it consistent across the library or am I missing some > details about why the padding is not an issue in that case? Not sure. We don't allow arbitrary padding do we? That would require extra processing to skip padding in case it's very large. Let's try to come up with a policy and document it as part of the spec? > > > + vu_panic(dev, "VHOST_USER_REM_MEM_REG received multiple regions"); > > > > Maybe print the parameters that caused the issue? > > > > Ack. > > > > + return false; > > > + } > > > + > > > DPRINT("Removing region:\n"); > > > DPRINT(" guest_phys_addr: 0x%016"PRIx64"\n", > > > msg_region->guest_phys_addr); > > > -- > > > 2.20.1 > >
On Mon, Jan 10, 2022 at 07:43:06PM +0000, Raphael Norwitz wrote: > On Mon, Jan 10, 2022 at 04:36:34AM -0500, Michael S. Tsirkin wrote: > > On Thu, Jan 06, 2022 at 06:47:26AM +0000, Raphael Norwitz wrote: > > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > > > > > > Raphael any chance you can add a bit more to commit logs? > > E.g. what happens right now if you pass more? > > > > Sure - I'll add those details. > > > > --- > > > subprojects/libvhost-user/libvhost-user.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c > > > index 787f4d2d4f..a6dadeb637 100644 > > > --- a/subprojects/libvhost-user/libvhost-user.c > > > +++ b/subprojects/libvhost-user/libvhost-user.c > > > @@ -801,6 +801,12 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) { > > > VuDevRegion shadow_regions[VHOST_USER_MAX_RAM_SLOTS] = {}; > > > VhostUserMemoryRegion m = vmsg->payload.memreg.region, *msg_region = &m; > > > > > > + if (vmsg->fd_num != 1 || > > > + vmsg->size != sizeof(vmsg->payload.memreg)) { > > > > Is there a chance someone is sending larger messages and relying > > on libvhost-user to ignore padding? > > > > Great point - I didn't consider padding. I'll drop the vmsg->size check > here. A vmsg->size > sizeof(vmsg->payload.memreg) check is still reasonable. Without a check we'll treat the 0-initialized vmsg bytes as input, which should be okay as long as there is input validation later on. In some cases 0s may really be valid input and we'll perform some operation. So in the end, I think checking vmsg->size is safest, just ignore extra bytes. Stefan
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c index 787f4d2d4f..a6dadeb637 100644 --- a/subprojects/libvhost-user/libvhost-user.c +++ b/subprojects/libvhost-user/libvhost-user.c @@ -801,6 +801,12 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) { VuDevRegion shadow_regions[VHOST_USER_MAX_RAM_SLOTS] = {}; VhostUserMemoryRegion m = vmsg->payload.memreg.region, *msg_region = &m; + if (vmsg->fd_num != 1 || + vmsg->size != sizeof(vmsg->payload.memreg)) { + vu_panic(dev, "VHOST_USER_REM_MEM_REG received multiple regions"); + return false; + } + DPRINT("Removing region:\n"); DPRINT(" guest_phys_addr: 0x%016"PRIx64"\n", msg_region->guest_phys_addr);
Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> --- subprojects/libvhost-user/libvhost-user.c | 6 ++++++ 1 file changed, 6 insertions(+)