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.
Regards,
Elena & Jag & JJ.
> Stefan
