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

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 19 Feb 2018 at 11:44, Marcel Apfelbaum <marcel@redhat.com> wrote:
>
> From: Yuval Shaia <yuval.shaia@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 <dotanb@mellanox.com>
> Reviewed-by: Zhu Yanjun <yanjun.zhu@oracle.com>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>

Hi; I know this is a 5 year old commit, but Coverity has just
noticed something odd in this code (CID 1507146):

> +static int query_port(PVRDMADev *dev, union pvrdma_cmd_req *req,
> +                      union pvrdma_cmd_resp *rsp)
> +{
> +    struct pvrdma_cmd_query_port *cmd = &req->query_port;
> +    struct pvrdma_cmd_query_port_resp *resp = &rsp->query_port_resp;
> +    struct pvrdma_port_attr attrs = {0};
> +
> +    pr_dbg("port=%d\n", cmd->port_num);
> +
> +    if (rdma_backend_query_port(&dev->backend_dev,
> +                                (struct ibv_port_attr *)&attrs)) {

rdma_backend_query_port() wants a pointer to a struct ibv_port_attr.
But instead of passing it one, we have a local struct
pvrdma_port_attr and then explicitly cast it to the
other type.

Unfortunately, ibv_port_attr is larger than pvrdma_port_attr
(50 bytes vs 48 bytes), so this could overrun the local
variable.

What's going on here, and what should the code be doing instead?

Given that we are just copying fields out of the structure,
and (other than the extra field at the end of ibv_port_attr)
the struct layout is identical, it looks to me that the simple
fix would be to make the local variable have the correct type
'struct ibv_port_attr' and drop the cast.

> +        return -ENOMEM;
> +    }
> +
> +    memset(resp, 0, sizeof(*resp));
> +    resp->hdr.response = cmd->hdr.response;
> +    resp->hdr.ack = PVRDMA_CMD_QUERY_PORT_RESP;
> +    resp->hdr.err = 0;
> +
> +    resp->attrs.state = attrs.state;
> +    resp->attrs.max_mtu = attrs.max_mtu;
> +    resp->attrs.active_mtu = attrs.active_mtu;
> +    resp->attrs.phys_state = attrs.phys_state;
> +    resp->attrs.gid_tbl_len = MIN(MAX_PORT_GIDS, attrs.gid_tbl_len);
> +    resp->attrs.max_msg_sz = 1024;
> +    resp->attrs.pkey_tbl_len = MIN(MAX_PORT_PKEYS, attrs.pkey_tbl_len);
> +    resp->attrs.active_width = 1;
> +    resp->attrs.active_speed = 1;
> +
> +    return 0;
> +}

thanks
-- PMM