Re: [Qemu-devel] [PULL 20/42] spapr: initial implementation for H_TPM_COMM/spapr-tpm-proxy

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

On Wed, 21 Aug 2019 at 08:26, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> From: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> This implements the H_TPM_COMM hypercall, which is used by an
> Ultravisor to pass TPM commands directly to the host's TPM device, or
> a TPM Resource Manager associated with the device.
>
> This also introduces a new virtual device, spapr-tpm-proxy, which
> is used to configure the host TPM path to be used to service
> requests sent by H_TPM_COMM hcalls, for example:
>
>   -device spapr-tpm-proxy,id=tpmp0,host-path=/dev/tpmrm0
>
> By default, no spapr-tpm-proxy will be created, and hcalls will return
> H_FUNCTION.
>
> The full specification for this hypercall can be found in
> docs/specs/ppc-spapr-uv-hcalls.txt
>
> Since SVM-related hcalls like H_TPM_COMM use a reserved range of
> 0xEF00-0xEF80, we introduce a separate hcall table here to handle
> them.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com
> Message-Id: <20190717205842.17827-3-mdroth@linux.vnet.ibm.com>
> [dwg: Corrected #include for upstream change]
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Hi; Coverity reports an issue in this change (CID 1405304):

> +static ssize_t tpm_execute(SpaprTpmProxy *tpm_proxy, target_ulong *args)
> +{
> +    uint64_t data_in = ppc64_phys_to_real(args[1]);
> +    target_ulong data_in_size = args[2];
> +    uint64_t data_out = ppc64_phys_to_real(args[3]);
> +    target_ulong data_out_size = args[4];
> +    uint8_t buf_in[TPM_SPAPR_BUFSIZE];
> +    uint8_t buf_out[TPM_SPAPR_BUFSIZE];
> +    ssize_t ret;
> +
> +    trace_spapr_tpm_execute(data_in, data_in_size, data_out, data_out_size);
> +
> +    if (data_in_size > TPM_SPAPR_BUFSIZE) {
> +        error_report("invalid TPM input buffer size: " TARGET_FMT_lu,
> +                     data_in_size);
> +        return H_P3;
> +    }
> +
> +    if (data_out_size < TPM_SPAPR_BUFSIZE) {
> +        error_report("invalid TPM output buffer size: " TARGET_FMT_lu,
> +                     data_out_size);
> +        return H_P5;
> +    }
> +
> +    if (tpm_proxy->host_fd == -1) {
> +        tpm_proxy->host_fd = open(tpm_proxy->host_path, O_RDWR);
> +        if (tpm_proxy->host_fd == -1) {
> +            error_report("failed to open TPM device %s: %d",
> +                         tpm_proxy->host_path, errno);
> +            return H_RESOURCE;
> +        }
> +    }
> +
> +    cpu_physical_memory_read(data_in, buf_in, data_in_size);
> +
> +    do {
> +        ret = write(tpm_proxy->host_fd, buf_in, data_in_size);
> +        if (ret > 0) {
> +            data_in_size -= ret;
> +        }
> +    } while ((ret >= 0 && data_in_size > 0) || (ret == -1 && errno == EINTR));
> +
> +    if (ret == -1) {
> +        error_report("failed to write to TPM device %s: %d",
> +                     tpm_proxy->host_path, errno);
> +        return H_RESOURCE;
> +    }
> +
> +    do {
> +        ret = read(tpm_proxy->host_fd, buf_out, data_out_size);
> +    } while (ret == 0 || (ret == -1 && errno == EINTR));
> +
> +    if (ret == -1) {
> +        error_report("failed to read from TPM device %s: %d",
> +                     tpm_proxy->host_path, errno);

The tpm_execute() function can unconditionally dereference
tpm_proxy->host_path, implying it can never be NULL...

> +        return H_RESOURCE;
> +    }
> +
> +    cpu_physical_memory_write(data_out, buf_out, ret);
> +    args[0] = ret;
> +
> +    return H_SUCCESS;
> +}
> +
> +static target_ulong h_tpm_comm(PowerPCCPU *cpu,
> +                               SpaprMachineState *spapr,
> +                               target_ulong opcode,
> +                               target_ulong *args)
> +{
> +    target_ulong op = args[0];
> +    SpaprTpmProxy *tpm_proxy = spapr->tpm_proxy;
> +
> +    if (!tpm_proxy) {
> +        error_report("TPM proxy not available");
> +        return H_FUNCTION;
> +    }
> +
> +    trace_spapr_h_tpm_comm(tpm_proxy->host_path ?: "null", op);

...but in this tracing at the callsite we check for whether
it is NULL or not, implying that it can be NULL.

> +
> +    switch (op) {
> +    case TPM_COMM_OP_EXECUTE:
> +        return tpm_execute(tpm_proxy, args);
> +    case TPM_COMM_OP_CLOSE_SESSION:
> +        spapr_tpm_proxy_reset(tpm_proxy);
> +        return H_SUCCESS;
> +    default:
> +        return H_PARAMETER;
> +    }
> +}

Coverity isn't happy about the possible use-after-NULL-check.

thanks
-- PMM