Re: [Qemu-devel] [RFC v3 PATCH 07/45] multi-process: define proxy-link object

[Jag Raman]

On 10/9/2019 1:58 PM, Elena Ufimtseva wrote:
> On Wed, Oct 09, 2019 at 02:37:24PM +0100, Stefan Hajnoczi wrote:
> > On Thu, Sep 12, 2019 at 05:34:35PM +0200, Stefan Hajnoczi wrote:
> > > On Tue, Sep 03, 2019 at 04:37:33PM -0400, Jagannathan Raman wrote:
> > > > +    msg->num_fds = 0;
> > > > +    for (chdr = CMSG_FIRSTHDR(&hdr); chdr != NULL;
> > > > +         chdr = CMSG_NXTHDR(&hdr, chdr)) {
> > > > +        if ((chdr->cmsg_level == SOL_SOCKET) &&
> > > > +            (chdr->cmsg_type == SCM_RIGHTS)) {
> > > > +            fdsize = chdr->cmsg_len - CMSG_LEN(0);
> > > > +            msg->num_fds = fdsize / sizeof(int);
> > > > +            memcpy(msg->fds, CMSG_DATA(chdr), fdsize);
> > >
> > > Please validate num_fds before memcpy to prevent the buffer overflow.
> > >
> > > > +            break;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    if (msg->size && msg->bytestream) {
> > > > +        msg->data2 = calloc(1, msg->size);
> > > > +        data = msg->data2;
> > > > +    } else {
> > > > +        data = (uint8_t *)&msg->data1;
> > > > +    }
> > > > +
> > > > +    if (msg->size) {
> > > > +        do {
> > > > +            rc = read(sock, data, msg->size);
> > > > +        } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> > > > +    }
> > >
> > > Please validate size to prevent the buffer overflow.
> >
> > I didn't see a reply so I want to highlight that the effort to introduce
> > isolation between devices is pointless if the communications link is not
> > coded securely.
> >
> > Multi-process QEMU adds no security if one process can corrupt the
> > memory of another process by sending invalid inputs.  Please audit the
> > code.
>
> Hi Stefan
>
> Sorry for not replyingi earlier. We have reviewed all the comments we received
> on the this patch series and working on the code improvements which are
> mostly done.
> We recognize the importance of the secure code and making efforts to
> eliminate as much as possible these mentioned unverified inputs along with 
> other changes.
>
> The changes will be in the next version of the patchset we are actively
> working on.
>
> The other your suggestion about reducing the number of syscalls by
> stuffing all of the parts of the message in the io_vec and using one
> sendmsg/recvmsg cannot be done at this point with the way we have
> organized the messages structure. But we are looking into the
> adoption of shared ring buffer for communication channel instead of the
> current mechanism to reduce the number of syscalls.
> Though this change will not be a part of the next patchset as we are
> tinkering with live migration.
> But all other recommendations and comments will be taken into account.

Thanks Elena!

Hi Stefan,

I'd like to confirm that we are targeting Live Migration for next
version and we are also discussing another use case for this project.

The other use case we are discussing is testing emulated devices (as 
separate processes) with libfuzzer. We could test corner cases and the 
handling of error conditions on emulated devices using this approach.

Thanks!
--
Jag

> Regards,
> Elena & Jag & JJ.
>
>
> > Stefan