| Message ID | 20220117041050.19718-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 17.01.22 05:12, Raphael Norwitz wrote: > Today if multiple FDs are sent from the VMM to the backend in a > VHOST_USER_REM_MEM_REG message, one FD will be unmapped and the remaining > FDs will be leaked. Therefore if multiple FDs are sent we report an > error and fail the operation, closing all FDs in the message. > > Likewise in case the VMM sends a message with a size less than that of a > memory region descriptor, we add a check to gracefully report an error > and fail the operation rather than crashing. > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > --- > subprojects/libvhost-user/libvhost-user.c | 15 +++++++++++++++ > subprojects/libvhost-user/libvhost-user.h | 2 ++ > 2 files changed, 17 insertions(+) > > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c > index 787f4d2d4f..b09b1c269e 100644 > --- a/subprojects/libvhost-user/libvhost-user.c > +++ b/subprojects/libvhost-user/libvhost-user.c > @@ -801,6 +801,21 @@ 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_close_fds(vmsg); > + vu_panic(dev, "VHOST_USER_REM_MEM_REG received %d fds - only 1 fd " > + "should be sent for this message type", vmsg->fd_num); > + return false; > + } > + > + if (vmsg->size < VHOST_USER_MEM_REG_SIZE) { > + close(vmsg->fds[0]); I wonder if using vmsg_close_fds(vmsg); makes the code easier to read. Reviewed-by: David Hildenbrand <david@redhat.com>
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c index 787f4d2d4f..b09b1c269e 100644 --- a/subprojects/libvhost-user/libvhost-user.c +++ b/subprojects/libvhost-user/libvhost-user.c @@ -801,6 +801,21 @@ 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_close_fds(vmsg); + vu_panic(dev, "VHOST_USER_REM_MEM_REG received %d fds - only 1 fd " + "should be sent for this message type", vmsg->fd_num); + return false; + } + + if (vmsg->size < VHOST_USER_MEM_REG_SIZE) { + close(vmsg->fds[0]); + vu_panic(dev, "VHOST_USER_REM_MEM_REG requires a message size of at " + "least %d bytes and only %d bytes were received", + VHOST_USER_MEM_REG_SIZE, vmsg->size); + return false; + } + DPRINT("Removing region:\n"); DPRINT(" guest_phys_addr: 0x%016"PRIx64"\n", msg_region->guest_phys_addr); diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h index 3d13dfadde..cde9f07bb3 100644 --- a/subprojects/libvhost-user/libvhost-user.h +++ b/subprojects/libvhost-user/libvhost-user.h @@ -129,6 +129,8 @@ typedef struct VhostUserMemoryRegion { uint64_t mmap_offset; } VhostUserMemoryRegion; +#define VHOST_USER_MEM_REG_SIZE (sizeof(VhostUserMemoryRegion)) + typedef struct VhostUserMemory { uint32_t nregions; uint32_t padding;
Today if multiple FDs are sent from the VMM to the backend in a VHOST_USER_REM_MEM_REG message, one FD will be unmapped and the remaining FDs will be leaked. Therefore if multiple FDs are sent we report an error and fail the operation, closing all FDs in the message. Likewise in case the VMM sends a message with a size less than that of a memory region descriptor, we add a check to gracefully report an error and fail the operation rather than crashing. Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> --- subprojects/libvhost-user/libvhost-user.c | 15 +++++++++++++++ subprojects/libvhost-user/libvhost-user.h | 2 ++ 2 files changed, 17 insertions(+)