Re: [Xen-devel] [PATCH v2] xen-hvm: stop faking I/O to access PCI config space

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.05.18 at 15:00, <paul.durrant@xxxxxxxxxx> wrote:
> @@ -903,6 +926,80 @@ static void cpu_ioreq_move(ioreq_t *req)
>      }
>  }
>  
> +static void rw_config_req_item(XenPciDevice *xendev, ioreq_t *req,

It looks to me as if both parameters could be constified.

> +                               uint32_t i, uint32_t *val)
> +{
> +    int32_t reg = req->addr;
> +    uint32_t offset = req->size * i;
> +
> +    reg += (req->df ? -1 : 1) * offset;
> +    if (reg < 0 || reg > PCI_CONFIG_SPACE_SIZE) {

Having fought a number of issues in this area in the hypervisor a couple
of years back I wonder
- why reg is of signed type,
- whether overflow of the first multiplication really doesn't matter,
- whether wrapping when adding in the offset is not an issue.

I take it that the rather lax upper bound check (should imo really be
reg + size > PCI_CONFIG_SPACE_SIZE [implying reg + size doesn't
itself wrap], or at least reg >= PCI_CONFIG_SPACE_SIZE) is not a
problem because ...

> +        if (req->dir == IOREQ_READ) {
> +            *val = ~0u;
> +        }
> +        return;
> +    }
> +
> +    if (req->dir == IOREQ_READ) {
> +        *val = pci_host_config_read_common(xendev->pci_dev, reg,
> +                                           PCI_CONFIG_SPACE_SIZE,
> +                                           req->size);
> +        trace_cpu_ioreq_config_read(req, xendev->sbdf, reg,
> +                                    req->size, *val);
> +    } else {
> +        trace_cpu_ioreq_config_write(req, xendev->sbdf, reg, req->size,
> +                                     *val);
> +        pci_host_config_write_common(xendev->pci_dev, reg,
> +                                     PCI_CONFIG_SPACE_SIZE, *val,
> +                                     req->size);
> +    }

... these called functions do full checking anyway?

> +static void cpu_ioreq_config(XenIOState *state, ioreq_t *req)
> +{
> +    uint32_t sbdf = req->addr >> 32;
> +    XenPciDevice *xendev;
> +
> +    if (req->size > sizeof(uint32_t)) {
> +        hw_error("PCI config access: bad size (%u)", req->size);

What about size 0 or 3?

> +    }
> +
> +    QLIST_FOREACH(xendev, &state->dev_list, entry) {
> +        unsigned int i;
> +        uint32_t tmp;
> +
> +        if (xendev->sbdf != sbdf) {
> +            continue;
> +        }
> +
> +        if (!req->data_is_ptr) {
> +            if (req->dir == IOREQ_READ) {
> +                for (i = 0; i < req->count; i++) {
> +                    rw_config_req_item(xendev, req, i, &tmp);
> +                    req->data = tmp;
> +                }
> +            } else if (req->dir == IOREQ_WRITE) {
> +                for (i = 0; i < req->count; i++) {
> +                    tmp = req->data;
> +                    rw_config_req_item(xendev, req, i, &tmp);
> +                }
> +            }

Wouldn't it be more sensible to fail req->count != 1 requests here?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel