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

[Greg Kurz]

On Mon, 9 Sep 2019 18:23:20 +0100
Peter Maydell <address@hidden> wrote:

> 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.
> 

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