On Mon, 9 Sep 2019 18:23:20 +0100 Peter Maydell <peter.mayd...@linaro.org> wrote:
> 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. > And we have this in the device realize function: static void spapr_tpm_proxy_realize(DeviceState *d, Error **errp) { SpaprTpmProxy *tpm_proxy = SPAPR_TPM_PROXY(d); if (tpm_proxy->host_path == NULL) { error_setg(errp, "must specify 'host-path' option for device"); return; } [...] } so we can safely assume host_path is never NULL. I guess the fix is to stop checking tpm_proxy->host_path in the trace callsite above. > > + > > + 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