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

[Yuval Shaia]

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 <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?

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