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

[Raphael Norwitz]

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
>