On Sat, Jun 27, 2020 at 10:09:36AM -0700, elena.ufimts...@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?
signature.asc
Description: PGP signature
