Re: [PATCH v2 1/5] libvhost-user: Add vu_rem_mem_reg input validation

["Michael S. Tsirkin" <mst@redhat.com>]

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
> >