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

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

On Wed, Oct 16, 2024 at 01:51:29PM -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 steps and stop if the first few bytes indicate an
> error response with no subsequent bytes readable. Read the rest in a 2nd
> step, if needed. This avoids getting stuck while waiting for too many
> bytes in case of an error. 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 | 61 +++++++++++++++++++++++++++----------
>  1 file changed, 45 insertions(+), 16 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index b0e2fb3fc7..8ad54f49a5 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -123,12 +123,14 @@ 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;
>      uint8_t *buf = NULL;
> +    ptm_res res;
>  
>      WITH_QEMU_LOCK_GUARD(&tpm->mutex) {
>          buf = g_alloca(n);
> @@ -140,8 +142,24 @@ 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_total > 0) {
> +            assert(msg_len_out_total >= msg_len_out_err);
> +
> +            n = qemu_chr_fe_read_all(dev, (uint8_t *)msg, msg_len_out_err);
> +            if (n <= 0) {
> +                return -1;
> +            }
> +            if (msg_len_out_err == msg_len_out_total) {
> +                return 0;
> +            }
> +            /* result error code is always in the first 4 bytes */
> +            memcpy(&res, msg, sizeof(res));

Before this memcpy we need

   assert(sizeof(res) <= msg_len_out_err);

to sanity-check we're not reading un-initialized memory in 'msg',
or worse, reading out of bounds.


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