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

[Marcel Apfelbaum]

Hi Peter,

On 27/04/2018 17:31, Peter Maydell wrote:
> 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 ">=".
> 

Right, thanks for finding it!

>> +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.
> 

The index is provided by the guest, so we should check it,
right Yuval?

I'll take care of it.
Thanks,
Marcel

>> +
>> +    return 0;
>> +}
> 
> thanks
> -- PMM
>