Re: [PATCH v7 14/21] multi-process: PCI BAR read/write handling for proxy & remote endpoints

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2566 bytes --]

On Sat, Jun 27, 2020 at 10:09:36AM -0700, elena.ufimtseva@oracle.com wrote:
> @@ -54,6 +57,12 @@ gboolean mpqemu_process_msg(QIOChannel *ioc, GIOCondition cond,
>      case PCI_CONFIG_READ:
>          process_config_read(ioc, pci_dev, &msg);
>          break;
> +    case BAR_WRITE:
> +        process_bar_write(ioc, &msg, &local_err);
> +        break;
> +    case BAR_READ:
> +        process_bar_read(ioc, &msg, &local_err);
> +        break;

These functions are more than BAR read/write functions, they are general
address space read/write functions. This is could be a security problem
in the future because the client has access to more than just the PCI
device they are supposed to communicate with.

I don't have a strong opinion against leaving it as-is, but wanted to
mention it because the current approach is risky as a long-term
solution. The protocol message could have a uint8_t bar_index field or
the remote device could validate that addr falls within the device BARs.

>      default:
>          error_setg(&local_err, "Unknown command (%d) received from proxy \
>                     in remote process pid=%d", msg.cmd, getpid());
> @@ -143,3 +152,89 @@ static void process_config_read(QIOChannel *ioc, PCIDevice *dev,
>  
>      mpqemu_msg_send(&ret, ioc);
>  }
> +
> +static void process_bar_write(QIOChannel *ioc, MPQemuMsg *msg, Error **errp)
> +{
> +    BarAccessMsg *bar_access = &msg->data1.bar_access;
> +    AddressSpace *as =
> +        bar_access->memory ? &address_space_memory : &address_space_io;
> +    MPQemuMsg ret = { 0 };
> +    MemTxResult res;
> +
> +    if (!is_power_of_2(bar_access->size) ||
> +       (bar_access->size > sizeof(uint64_t))) {
> +        ret.data1.u64 = UINT64_MAX;
> +        goto fail;
> +    }
> +
> +    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
> +                           (void *)&bar_access->val, bar_access->size,
> +                           true);

(void *)&bar_access->val doesn't work on big-endian hosts. A uint64_t ->
{uint32_t, uint16_t, uint8_t} conversion must be performed before the
address_space_rw() call analogous to what process_bar_read() does.

Although it's unlikely that this will be run on big-endian hosts anytime
soon, it's good practice to keep the code portable when possible.

> +    case BAR_WRITE:
> +    case BAR_READ:
> +        if (msg->size != sizeof(msg->data1)) {
> +            return false;
> +        }

Is there a check that the number of passed fds is zero somewhere?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]