Hi Peter,
On 27/04/2018 17:31, Peter Maydell wrote:
> On 19 February 2018 at 11:43, Marcel Apfelbaum <mar...@redhat.com> wrote:
>> From: Yuval Shaia <yuval.sh...@oracle.com>
>>
>> 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 <dot...@mellanox.com>
>> Reviewed-by: Zhu Yanjun <yanjun....@oracle.com>
>> Signed-off-by: Yuval Shaia <yuval.sh...@oracle.com>
>> Signed-off-by: Marcel Apfelbaum <mar...@redhat.com>
>
> 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
>
