Re: [Qemu-devel] [PATCH PULL v2 08/10] hw/rdma: PVRDMA commands and data-path ops

[Peter Maydell]

On 19 February 2018 at 11:43, Marcel Apfelbaum <address@hidden> wrote:
> From: Yuval Shaia <address@hidden>
>
> First PVRDMA sub-module - implementation of the PVRDMA device.
> - PVRDMA commands such as create CQ and create MR.
> - Data path QP operations - post_send and post_recv.
> - Completion handler.
>
> Reviewed-by: Dotan Barak <address@hidden>
> Reviewed-by: Zhu Yanjun <address@hidden>
> Signed-off-by: Yuval Shaia <address@hidden>
> Signed-off-by: Marcel Apfelbaum <address@hidden>

Hi; Coverity points out an array bounds overrun in this code:


> +static int create_bind(PVRDMADev *dev, union pvrdma_cmd_req *req,
> +                       union pvrdma_cmd_resp *rsp)
> +{
> +    struct pvrdma_cmd_create_bind *cmd = &req->create_bind;
> +#ifdef PVRDMA_DEBUG
> +    __be64 *subnet = (__be64 *)&cmd->new_gid[0];
> +    __be64 *if_id = (__be64 *)&cmd->new_gid[8];
> +#endif
> +
> +    pr_dbg("index=%d\n", cmd->index);
> +
> +    if (cmd->index > MAX_PORT_GIDS) {
> +        return -EINVAL;
> +    }

This bounds check allows cmd->index == MAX_PORT_GIDS...

> +
> +    pr_dbg("gid[%d]=0x%llx,0x%llx\n", cmd->index,
> +           (long long unsigned int)be64_to_cpu(*subnet),
> +           (long long unsigned int)be64_to_cpu(*if_id));
> +
> +    /* Driver forces to one port only */
> +    memcpy(dev->rdma_dev_res.ports[0].gid_tbl[cmd->index].raw, &cmd->new_gid,
> +           sizeof(cmd->new_gid));

...but the gid_tbl[] array we index into is declared with

    union ibv_gid gid_tbl[MAX_PORT_GIDS];

so using MAX_PORT_GIDS as an index is off the end of it.

Presumably the check should be ">=".

> +static int destroy_bind(PVRDMADev *dev, union pvrdma_cmd_req *req,
> +                        union pvrdma_cmd_resp *rsp)
> +{
> +    struct pvrdma_cmd_destroy_bind *cmd = &req->destroy_bind;
> +
> +    pr_dbg("clear index %d\n", cmd->index);
> +
> +    memset(dev->rdma_dev_res.ports[0].gid_tbl[cmd->index].raw, 0,
> +           sizeof(dev->rdma_dev_res.ports[0].gid_tbl[cmd->index].raw));

I'm assuming this function can't be called unless create_bind()
has previously succeeded and so it doesn't need its own
bounds check.

> +
> +    return 0;
> +}

thanks
-- PMM