On Fri, Apr 27, 2018 at 09:20:44PM +0300, Marcel Apfelbaum wrote:
> 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?
Right,
Guest driver is considered trusted but we don't want a faulty driver to
crash the entire VM.
>
> I'll take care of it.
> Thanks,
> Marcel
>
> >> +
> >> + return 0;
> >> +}
> >
> > thanks
> > -- PMM
> >
>
