Re: [PATCH RESEND v6 20/36] multi-process: Forward PCI config space acceses to the remote process

[Stefan Hajnoczi]

On Wed, Apr 22, 2020 at 09:13:55PM -0700, address@hidden 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