qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PULL 20/42] spapr: initial implementation for H_TPM_CO


From: Peter Maydell
Subject: Re: [Qemu-devel] [PULL 20/42] spapr: initial implementation for H_TPM_COMM/spapr-tpm-proxy
Date: Mon, 9 Sep 2019 18:23:20 +0100

On Wed, 21 Aug 2019 at 08:26, David Gibson <address@hidden> wrote:
>
> From: Michael Roth <address@hidden>
>
> 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 <address@hidden
> Message-Id: <address@hidden>
> [dwg: Corrected #include for upstream change]
> Signed-off-by: David Gibson <address@hidden>

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]