On Wed, Apr 22, 2020 at 09:13:55PM -0700, elena.ufimts...@oracle.com wrote:
> +static int config_op_send(PCIProxyDev *dev, uint32_t addr, uint32_t *val,
> int l,
> + unsigned int op)
> +{
> + MPQemuMsg msg;
> + struct conf_data_msg conf_data;
> + int wait;
> +
> + memset(&msg, 0, sizeof(MPQemuMsg));
> + conf_data.addr = addr;
> + conf_data.val = (op == PCI_CONFIG_WRITE) ? *val : 0;
> + conf_data.l = l;
> +
> + msg.data2 = (uint8_t *)&conf_data;
> + if (!msg.data2) {
> + return -ENOMEM;
This can never happen since conf_data is on the stack.
> + }
> +
> + msg.size = sizeof(conf_data);
> + msg.cmd = op;
> + msg.bytestream = 1;
> +
> + if (op == PCI_CONFIG_WRITE) {
> + msg.num_fds = 0;
> + } else {
> + /* TODO: Dont create fd each time for send. */
> + wait = GET_REMOTE_WAIT;
> + msg.num_fds = 1;
> + msg.fds[0] = wait;
> + }
> +
> + mpqemu_msg_send(&msg, dev->mpqemu_link->dev);
> +
> + if (op == PCI_CONFIG_READ) {
Are you sure it's correct for writes to be posted instead of waiting for
completion?
> + *val = (uint32_t)wait_for_remote(wait);
> + PUT_REMOTE_WAIT(wait);
> + }
> +
> + return 0;
> +}
> +
> +static uint32_t pci_proxy_read_config(PCIDevice *d, uint32_t addr, int len)
> +{
> + uint32_t val;
> +
> + (void)pci_default_read_config(d, addr, len);
Please add a comment explaining why this local read is necessary.
> +
> + config_op_send(PCI_PROXY_DEV(d), addr, &val, len, PCI_CONFIG_READ);
> +
> + return val;
> +}
> +
> +static void pci_proxy_write_config(PCIDevice *d, uint32_t addr, uint32_t val,
> + int l)
> +{
> + pci_default_write_config(d, addr, val, l);
Please add a comment explaining why this local write is necessary.
> +
> + config_op_send(PCI_PROXY_DEV(d), addr, &val, l, PCI_CONFIG_WRITE);
> +}
...
> diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
> index f780b65181..ef4a07b81a 100644
> --- a/io/mpqemu-link.c
> +++ b/io/mpqemu-link.c
> @@ -381,6 +381,12 @@ bool mpqemu_msg_valid(MPQemuMsg *msg)
> return false;
> }
> break;
> + case PCI_CONFIG_WRITE:
> + case PCI_CONFIG_READ:
> + if (msg->size != sizeof(struct conf_data_msg)) {
> + return false;
> + }
conf_data_msg.l is not validated.
> + break;
> default:
> break;
> }
> diff --git a/remote/remote-main.c b/remote/remote-main.c
> index f541baae6a..834574e172 100644
> --- a/remote/remote-main.c
> +++ b/remote/remote-main.c
> @@ -53,6 +53,32 @@ gchar *print_pid_exec(gchar *str)
>
> #define LINK_TO_DEV(link) ((PCIDevice *)link->opaque)
>
> +static void process_config_write(PCIDevice *dev, MPQemuMsg *msg)
> +{
> + struct conf_data_msg *conf = (struct conf_data_msg *)msg->data2;
> +
> + qemu_mutex_lock_iothread();
The qemu_mutex_lock_iothread() can be dropped once this is integrated in
to QEMU's event loop.
> + pci_default_write_config(dev, conf->addr, conf->val, conf->l);
It is not safe to call this function with arbitrary addr input values.
> + qemu_mutex_unlock_iothread();
> +}
> +
> +static void process_config_read(PCIDevice *dev, MPQemuMsg *msg)
> +{
> + struct conf_data_msg *conf = (struct conf_data_msg *)msg->data2;
> + uint32_t val;
> + int wait;
> +
> + wait = msg->fds[0];
> +
> + qemu_mutex_lock_iothread();
> + val = pci_default_read_config(dev, conf->addr, conf->l);
It is not safe to call this function with arbitrary addr input values.
signature.asc
Description: PGP signature
