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

[Marcel Apfelbaum <marcel@redhat.com>]

On 27/04/2018 18:01, Peter Maydell wrote:
> On 19 February 2018 at 11:43, 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.
> 
> Coverity CID 1390620: we call munmap() on a NULL pointer.
> 
>> +static int create_mr(PVRDMADev *dev, union pvrdma_cmd_req *req,
>> +                     union pvrdma_cmd_resp *rsp)
>> +{
>> +    struct pvrdma_cmd_create_mr *cmd = &req->create_mr;
>> +    struct pvrdma_cmd_create_mr_resp *resp = &rsp->create_mr_resp;
>> +    PCIDevice *pci_dev = PCI_DEVICE(dev);
>> +    void *host_virt = NULL;
> 
> Here we set host_virt to NULL...
> 
>> +
>> +    memset(resp, 0, sizeof(*resp));
>> +    resp->hdr.response = cmd->hdr.response;
>> +    resp->hdr.ack = PVRDMA_CMD_CREATE_MR_RESP;
>> +
>> +    pr_dbg("pd_handle=%d\n", cmd->pd_handle);
>> +    pr_dbg("access_flags=0x%x\n", cmd->access_flags);
>> +    pr_dbg("flags=0x%x\n", cmd->flags);
>> +
>> +    if (!(cmd->flags & PVRDMA_MR_FLAG_DMA)) {
> 
> ...and if we don't take this if() we won't set host_virt to anything...
> 
>> +        host_virt = pvrdma_map_to_pdir(pci_dev, cmd->pdir_dma, cmd->nchunks,
>> +                                       cmd->length);
>> +        if (!host_virt) {
>> +            pr_dbg("Failed to map to pdir\n");
>> +            resp->hdr.err = -EINVAL;
>> +            goto out;
>> +        }
>> +    }
>> +
>> +    resp->hdr.err = rdma_rm_alloc_mr(&dev->rdma_dev_res, cmd->pd_handle,
>> +                                     cmd->start, cmd->length, host_virt,
>> +                                     cmd->access_flags, &resp->mr_handle,
>> +                                     &resp->lkey, &resp->rkey);
>> +    if (!resp->hdr.err) {
>> +        munmap(host_virt, cmd->length);
> 
> ...but here we call munmap() on it without checking if it is NULL.
> Unlike g_free(), munmap() isn't specified to be "do nothing if
> passed a NULL pointer".

Will fix, thanks for finding it!
Marcel

> 
>> +    }
>> +
>> +out:
>> +    pr_dbg("ret=%d\n", resp->hdr.err);
>> +    return resp->hdr.err;
>> +}
> 
> thanks
> -- PMM
>