On Wed, 21 Aug 2019 at 08:26, David Gibson <da...@gibson.dropbear.id.au> wrote:
>
> From: Michael Roth <mdr...@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 <mdr...@linux.vnet.ibm.com
> Message-Id: <20190717205842.17827-3-mdr...@linux.vnet.ibm.com>
> [dwg: Corrected #include for upstream change]
> Signed-off-by: David Gibson <da...@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
