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