| Message ID | 00737a41c9bc4993ad47e5d4387ac14f1892be40.1582576372.git.jag.raman@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initial support for multi-process qemu | expand |
On Mon, Feb 24, 2020 at 03:55:39PM -0500, Jagannathan Raman wrote: > From: Elena Ufimtseva <elena.ufimtseva@oracle.com> > > Validate the incoming commands to confirm that they would not cause any > errors in the remote process. > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> > Signed-off-by: John G Johnson <john.g.johnson@oracle.com> > --- > hw/proxy/qemu-proxy.c | 6 +++- > include/io/mpqemu-link.h | 2 ++ > io/mpqemu-link.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++- > remote/remote-main.c | 4 +++ > 4 files changed, 85 insertions(+), 2 deletions(-) Please squash this into the patch(es) that introduced the code. Reviewers want to see a logical sequence of patches. Introducing unsafe code in an earlier patch and adding checks in a later patch makes it impossible to review the patches in sequence (reviewers would waste time pointing out bugs that end up getting fixed later). > diff --git a/remote/remote-main.c b/remote/remote-main.c > index 20d160e..c4aa3e0 100644 > --- a/remote/remote-main.c > +++ b/remote/remote-main.c > @@ -435,6 +435,10 @@ static void process_msg(GIOCondition cond, MPQemuChannel *chan) > if (msg->id > MAX_REMOTE_DEVICES) { > error_setg(&err, "id of the device is larger than max number of "\ > "devices per remote process."); > + } Was goto finalize_loop accidentally dropped?
On Thu, Feb 27, 2020 at 05:18:30PM +0000, Stefan Hajnoczi wrote: > On Mon, Feb 24, 2020 at 03:55:39PM -0500, Jagannathan Raman wrote: > > From: Elena Ufimtseva <elena.ufimtseva@oracle.com> > > > > Validate the incoming commands to confirm that they would not cause any > > errors in the remote process. > > > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> > > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> > > Signed-off-by: John G Johnson <john.g.johnson@oracle.com> > > --- > > hw/proxy/qemu-proxy.c | 6 +++- > > include/io/mpqemu-link.h | 2 ++ > > io/mpqemu-link.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++- > > remote/remote-main.c | 4 +++ > > 4 files changed, 85 insertions(+), 2 deletions(-) > > Please squash this into the patch(es) that introduced the code. > > Reviewers want to see a logical sequence of patches. Introducing > unsafe code in an earlier patch and adding checks in a later patch makes > it impossible to review the patches in sequence (reviewers would waste > time pointing out bugs that end up getting fixed later). > Thanks Stefan, will merge that with appropriate patches. > > diff --git a/remote/remote-main.c b/remote/remote-main.c > > index 20d160e..c4aa3e0 100644 > > --- a/remote/remote-main.c > > +++ b/remote/remote-main.c > > @@ -435,6 +435,10 @@ static void process_msg(GIOCondition cond, MPQemuChannel *chan) > > if (msg->id > MAX_REMOTE_DEVICES) { > > error_setg(&err, "id of the device is larger than max number of "\ > > "devices per remote process."); > > + } > > Was goto finalize_loop accidentally dropped? Yes, thank you. Elena
diff --git a/hw/proxy/qemu-proxy.c b/hw/proxy/qemu-proxy.c index 4d4eff4..7330094 100644 --- a/hw/proxy/qemu-proxy.c +++ b/hw/proxy/qemu-proxy.c @@ -868,7 +868,11 @@ static void send_bar_access_msg(PCIProxyDev *dev, MemoryRegion *mr, mpqemu_msg_recv(&ret, mpqemu_link->mmio); - *val = ret.data1.mmio_ret.val; + if (!mpqemu_msg_valid(&ret)) { + *val = 0; + } else { + *val = ret.data1.mmio_ret.val; + } } void proxy_default_bar_write(void *opaque, hwaddr addr, uint64_t val, diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h index 722f08c..2f44f31 100644 --- a/include/io/mpqemu-link.h +++ b/include/io/mpqemu-link.h @@ -213,4 +213,6 @@ void mpqemu_start_coms(MPQemuLinkState *s); uint64_t wait_for_remote(int efd); void notify_proxy(int fd, uint64_t val); +bool mpqemu_msg_valid(MPQemuMsg *msg); + #endif diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c index 07a9be9..298bb58 100644 --- a/io/mpqemu-link.c +++ b/io/mpqemu-link.c @@ -65,7 +65,7 @@ void mpqemu_link_finalize(MPQemuLinkState *s) void mpqemu_msg_send(MPQemuMsg *msg, MPQemuChannel *chan) { int rc; - uint8_t *data; + uint8_t *data = NULL; union { char control[CMSG_SPACE(REMOTE_MAX_FDS * sizeof(int))]; struct cmsghdr align; @@ -335,3 +335,76 @@ void mpqemu_start_coms(MPQemuLinkState *s) g_main_loop_run(s->loop); } + +bool mpqemu_msg_valid(MPQemuMsg *msg) +{ + if (msg->cmd >= MAX) { + return false; + } + + if (msg->bytestream) { + if (!msg->data2) { + return false; + } + } else { + if (msg->data2) { + return false; + } + } + + /* Verify FDs. */ + if (msg->num_fds >= REMOTE_MAX_FDS) { + return false; + } + if (msg->num_fds > 0) { + for (int i = 0; i < msg->num_fds; i++) { + if (fcntl(msg->fds[i], F_GETFL) == -1) { + return false; + } + } + } + + /* Verify ID size. */ + if (msg->id >= UINT64_MAX) { + return false; + } + + /* Verify message specific fields. */ + switch (msg->cmd) { + case SYNC_SYSMEM: + if (msg->num_fds == 0 || msg->bytestream != 0) { + return false; + } + if (msg->size != sizeof(msg->data1)) { + return false; + } + break; + case PCI_CONFIG_WRITE: + case PCI_CONFIG_READ: + if (msg->size != sizeof(struct conf_data_msg)) { + return false; + } + break; + case BAR_WRITE: + case BAR_READ: + case SET_IRQFD: + case MMIO_RETURN: + case DEVICE_RESET: + case RUNSTATE_SET: + if (msg->size != sizeof(msg->data1)) { + return false; + } + break; + case PROXY_PING: + case START_MIG_OUT: + case START_MIG_IN: + if (msg->size != 0) { + return false; + } + break; + default: + break; + } + + return true; +} diff --git a/remote/remote-main.c b/remote/remote-main.c index 20d160e..c4aa3e0 100644 --- a/remote/remote-main.c +++ b/remote/remote-main.c @@ -435,6 +435,10 @@ static void process_msg(GIOCondition cond, MPQemuChannel *chan) if (msg->id > MAX_REMOTE_DEVICES) { error_setg(&err, "id of the device is larger than max number of "\ "devices per remote process."); + } + + if (!mpqemu_msg_valid(msg)) { + error_setg(&err, "Message is not valid"); goto finalize_loop; }