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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]