Re: [PATCH v3 2/2] tpm_emulator: Read control channel response in 2 passes

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Oct 16, 2024 at 10:57:08AM -0400, Stefan Berger wrote:
> Error responses from swtpm are typically only 4 bytes long with the
> exception of a few commands that return more bytes. Therefore, read the
> entire response in 2 passes and stop if the first few bytes indicate an
> error response with no subsequent bytes readable. Read the rest in a 2nd
> pass, if needed. This avoids getting stuck while waiting for too many
> bytes. The 'getting stuck' condition has not been observed in practice so
> far, though.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2615
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  backends/tpm/tpm_emulator.c | 65 ++++++++++++++++++++++++++++---------
>  1 file changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index b0e2fb3fc7..dfb298a16d 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -123,12 +123,17 @@ static const char *tpm_emulator_strerror(uint32_t tpm_result)
>  }
>  
>  static int tpm_emulator_ctrlcmd(TPMEmulator *tpm, unsigned long cmd, void *msg,
> -                                size_t msg_len_in, size_t msg_len_out)
> +                                size_t msg_len_in, size_t msg_len_out_err,
> +                                size_t msg_len_out_total)
>  {
>      CharBackend *dev = &tpm->ctrl_chr;
>      uint32_t cmd_no = cpu_to_be32(cmd);
>      ssize_t n = sizeof(uint32_t) + msg_len_in;
> +    size_t left_to_read = msg_len_out_total;
>      uint8_t *buf = NULL;
> +    ptm_res res;
> +    off_t o = 0;
> +    int to_read;
>  
>      WITH_QEMU_LOCK_GUARD(&tpm->mutex) {
>          buf = g_alloca(n);
> @@ -140,11 +145,28 @@ static int tpm_emulator_ctrlcmd(TPMEmulator *tpm, unsigned long cmd, void *msg,
>              return -1;
>          }
>  
> -        if (msg_len_out != 0) {
> -            n = qemu_chr_fe_read_all(dev, msg, msg_len_out);
> +        if (msg_len_out_err > 0) {
> +            to_read = msg_len_out_err;
> +        } else {
> +            to_read = left_to_read;
> +        }
> +        while (to_read > 0) {
> +            n = qemu_chr_fe_read_all(dev, (uint8_t *)msg + o, to_read);
>              if (n <= 0) {
>                  return -1;
>              }
> +            left_to_read -= n;
> +            if (left_to_read == 0) {
> +                return 0;
> +            }
> +            /* result error code is always in the first 4 bytes */
> +            memcpy(&res, msg, sizeof(res));
> +            if (res) {
> +                return 0;
> +            }
> +
> +            o = to_read;
> +            to_read = left_to_read;
>          }

Using qemu_chr_fe_read_all in a loop feels like an anti-pattern, since
it will always read exactly the full amount you ask, or return an error.

IOW, there code here is a rather confusing way to do exactly 2 reads
of the required size.  I think it'd be much clearer to just read in
two steps, without a loop like this:

  n = qemu_chr_fe_read_all(dev, (uint8_t *)msg, msg_len_out_err);
  if (n != msg_len_out_err) {
    return -1;
  }
 
  /* result error code is always in the first 4 bytes */
  memcpy(&res, msg, sizeof(res));
  if (res) {
     return 0;
  }

  assert(msg_len_out_err <= msg_len_out_total);
  msg_len_out_total -= msg_len_out_err;
  n = qemu_chr_fe_read_all(dev, (uint8_t *)msg + msg_len_out_err,
                           msg_len_out_total);
  if (n != msg_len_out_total) {
    return -1;
  }

  return 0;

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|