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 <stef...@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 :|
