[PATCH 00/10] Fix a couple of issues in TPM related code

[PATCH 00/10] Fix a couple of issues in TPM related code

[Stefan Berger]

This series of patches fixes various issues in TPM-related code, such as
memory leaks, potential integer underflows, clearing of buffers while they
are in use, and the proper handling of sizes of commands or responses.

Regards,
 Stefan

Stefan Berger (10):
  tpm_util: Pass TPMVersion to function writing fatal error response
  tpm_emulator: Reject TPM responses whose size is too small
  tpm_emulator: Reject sending commands smaller than the request header
  hw/tpm: tis: Only read from response buffer if rw_offset < len
  hw/tpm: tis: Use uint32_t for len to avoid truncating size of command
  hw/tpm: crb: Do not clear internal buffers when backend is busy
  hw/tpm: crb: Avoid potential integer underflow
  hw/tpm: crb: Free ppi.buf in tpm_crb_unrealize
  hw/tpm: crb: Consider response_buffer->len of received response
  tests: Add tests with commands with bad length

 backends/tpm/tpm_emulator.c             | 12 ++++--
 backends/tpm/tpm_passthrough.c          |  2 +-
 backends/tpm/tpm_util.c                 | 12 ++++--
 hw/tpm/tpm_crb.c                        | 23 +++++++---
 hw/tpm/tpm_tis_common.c                 |  8 ++--
 include/system/tpm_util.h               |  3 +-
 tests/qtest/tpm-crb-swtpm-test.c        | 10 +++++
 tests/qtest/tpm-tests.c                 | 56 +++++++++++++++++++++++++
 tests/qtest/tpm-tests.h                 |  8 ++++
 tests/qtest/tpm-tis-device-swtpm-test.c | 10 +++++
 tests/qtest/tpm-tis-swtpm-test.c        | 10 +++++
 tests/qtest/tpm-util.c                  | 55 ++++++++++++++++++++++++
 tests/qtest/tpm-util.h                  |  3 ++
 13 files changed, 196 insertions(+), 16 deletions(-)

-- 
2.55.0

[PATCH 01/10] tpm_util: Pass TPMVersion to function writing fatal error response

[Stefan Berger]

Have tpm_util_write_fatal_error_response write a TPM version specific
error response with error code TPM_FAIL for a TPM 1.2 and TPM_RC_FAILURE
for a TPM 2. Adjust the callers.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 backends/tpm/tpm_emulator.c    |  3 ++-
 backends/tpm/tpm_passthrough.c |  2 +-
 backends/tpm/tpm_util.c        | 12 +++++++++---
 include/system/tpm_util.h      |  3 ++-
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index 95d2de2f48..16ae44c8d7 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -270,7 +270,8 @@ static void tpm_emulator_handle_request(TPMBackend *tb, TPMBackendCmd *cmd,
         tpm_emulator_unix_tx_bufs(tpm_emu, cmd->in, cmd->in_len,
                                   cmd->out, cmd->out_len,
                                   &cmd->selftest_done, errp) < 0) {
-        tpm_util_write_fatal_error_response(cmd->out, cmd->out_len);
+        tpm_util_write_fatal_error_response(cmd->out, cmd->out_len,
+                                            TPM_EMULATOR(tb)->tpm_version);
     }
 }
 
diff --git a/backends/tpm/tpm_passthrough.c b/backends/tpm/tpm_passthrough.c
index a9f35ab7d6..8a4f8183c7 100644
--- a/backends/tpm/tpm_passthrough.c
+++ b/backends/tpm/tpm_passthrough.c
@@ -118,7 +118,7 @@ static void tpm_passthrough_unix_tx_bufs(TPMPassthruState *tpm_pt,
 
 err_exit:
     if (ret < 0) {
-        tpm_util_write_fatal_error_response(out, out_len);
+        tpm_util_write_fatal_error_response(out, out_len, tpm_pt->tpm_version);
     }
 
     tpm_pt->tpm_executing = false;
diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
index 264bff22a9..cdb7a48861 100644
--- a/backends/tpm/tpm_util.c
+++ b/backends/tpm/tpm_util.c
@@ -87,12 +87,18 @@ const PropertyInfo qdev_prop_tpm = {
 /*
  * Write an error message in the given output buffer.
  */
-void tpm_util_write_fatal_error_response(uint8_t *out, uint32_t out_len)
+void tpm_util_write_fatal_error_response(uint8_t *out, uint32_t out_len,
+                                         TPMVersion tpm_version)
 {
     if (out_len >= sizeof(struct tpm_resp_hdr)) {
-        tpm_cmd_set_tag(out, TPM_TAG_RSP_COMMAND);
+        if (tpm_version == TPM_VERSION_1_2) {
+            tpm_cmd_set_tag(out, TPM_TAG_RSP_COMMAND);
+            tpm_cmd_set_error(out, TPM_FAIL);
+        } else {
+            tpm_cmd_set_tag(out, TPM2_ST_NO_SESSIONS);
+            tpm_cmd_set_error(out, TPM_RC_FAILURE);
+        }
         tpm_cmd_set_size(out, sizeof(struct tpm_resp_hdr));
-        tpm_cmd_set_error(out, TPM_FAIL);
     }
 }
 
diff --git a/include/system/tpm_util.h b/include/system/tpm_util.h
index 1858693225..3ab2fe693d 100644
--- a/include/system/tpm_util.h
+++ b/include/system/tpm_util.h
@@ -25,7 +25,8 @@
 #include "system/tpm.h"
 #include "qemu/bswap.h"
 
-void tpm_util_write_fatal_error_response(uint8_t *out, uint32_t out_len);
+void tpm_util_write_fatal_error_response(uint8_t *out, uint32_t out_len,
+                                         TPMVersion tpm_version);
 
 bool tpm_util_is_selftest(const uint8_t *in, uint32_t in_len);
 
-- 
2.55.0

[PATCH 02/10] tpm_emulator: Reject TPM responses whose size is too small

[Stefan Berger]

Reject a response from the TPM emulator whose size field indicates that the
response is smaller than the size of the header.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 backends/tpm/tpm_emulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index 16ae44c8d7..9c95b4d878 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -202,13 +202,13 @@ static int tpm_emulator_unix_tx_bufs(TPMEmulator *tpm_emu,
      * size)
      */
     to_read = tpm_cmd_get_size(out);
-    if (to_read > out_len) {
+    if (to_read > out_len || to_read < sizeof(struct tpm_resp_hdr)) {
         if (qio_channel_shutdown(tpm_emu->data_ioc, QIO_CHANNEL_SHUTDOWN_BOTH,
                                  &local_err) < 0) {
             error_report_err(local_err);
         }
         error_setg(errp, "tpm-emulator: Disconnected after receiving "
-                   "unacceptable large response (%u > %u)",
+                   "unacceptable response size (%u max: %u)",
                    to_read, out_len);
         return -1;
     }
-- 
2.55.0

[PATCH 03/10] tpm_emulator: Reject sending commands smaller than the request header

[Stefan Berger]

Reject any TPM request from being sent to the TPM emulator that is shorter
than the size of the request header.

Fixes: f4ede81eed29 ("tpm: Added support for TPM emulator")
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 backends/tpm/tpm_emulator.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index 9c95b4d878..0c9e32f36a 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -181,6 +181,11 @@ static int tpm_emulator_unix_tx_bufs(TPMEmulator *tpm_emu,
     uint32_t to_read;
     ssize_t ret;
 
+    if (in_len < sizeof(struct tpm_req_hdr)) {
+        error_setg(errp, "tpm-emulator: invalid request size %u", in_len);
+        return -1;
+    }
+
     if (selftest_done) {
         *selftest_done = false;
         is_selftest = tpm_util_is_selftest(in, in_len);
-- 
2.55.0

[PATCH 04/10] hw/tpm: tis: Only read from response buffer if rw_offset < len

[Stefan Berger]

Only read a byte from the response buffer, if the rw_offset < len.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 hw/tpm/tpm_tis_common.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c
index a134d5c205..4ba228116b 100644
--- a/hw/tpm/tpm_tis_common.c
+++ b/hw/tpm/tpm_tis_common.c
@@ -273,13 +273,15 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
         len = MIN(tpm_cmd_get_size(&s->buffer),
                   s->be_buffer_size);
 
-        ret = s->buffer[s->rw_offset++];
+        if (s->rw_offset < len) {
+            ret = s->buffer[s->rw_offset++];
+            trace_tpm_tis_data_read(ret, s->rw_offset - 1);
+        }
         if (s->rw_offset >= len) {
             /* got last byte */
             tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_VALID);
             tpm_tis_raise_irq(s, locty, TPM_TIS_INT_STS_VALID);
         }
-        trace_tpm_tis_data_read(ret, s->rw_offset - 1);
     }
 
     return ret;
-- 
2.55.0

[PATCH 05/10] hw/tpm: tis: Use uint32_t for len to avoid truncating size of command

[Stefan Berger]

To avoid truncating the uint32_t return value from tpm_cmd_get_size()
declare len as uint32_t rather than uint16_t.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 hw/tpm/tpm_tis_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c
index 4ba228116b..8fa1bee771 100644
--- a/hw/tpm/tpm_tis_common.c
+++ b/hw/tpm/tpm_tis_common.c
@@ -481,7 +481,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
     uint8_t locty = tpm_tis_locality_from_addr(addr);
     uint8_t active_locty, l;
     int c, set_new_locty = 1;
-    uint16_t len;
+    uint32_t len;
     uint32_t mask = (size == 1) ? 0xff : ((size == 2) ? 0xffff : ~0);
 
     trace_tpm_tis_mmio_write(size, addr, val);
-- 
2.55.0

[PATCH 06/10] hw/tpm: crb: Do not clear internal buffers when backend is busy

[Stefan Berger]

While the backend (thread) is accessing the request and response buffers,
do not clear those buffers upon a command cancellation. Still clear them
in all other cases, though.

Fixes: 29315d22b259 ("hw/tpm: Add internal buffer state for chunking")
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 hw/tpm/tpm_crb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 54fa2042b5..e5e33a3a2f 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -218,8 +218,9 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
         if (val == CRB_CANCEL_INVOKE) {
             if (s->regs[R_CRB_CTRL_START] & CRB_START_INVOKE) {
                 tpm_backend_cancel_cmd(s->tpmbe);
+            } else {
+                tpm_crb_clear_internal_buffers(s);
             }
-            tpm_crb_clear_internal_buffers(s);
         }
         break;
     case A_CRB_CTRL_START:
-- 
2.55.0

[PATCH 07/10] hw/tpm: crb: Avoid potential integer underflow

[Stefan Berger]

Avoid a potential integer underflow in tpm_crb_fill_command_response
that could occur if s->response_offset > s->response_buffer->len and
the function's call sites were to change.

tpm_crb_fill_command_response currently has 3 callers that either test
that 's->response_offset < s->response_buffer->len' is true or ensure that
s->response_offset = 0 and s->response_buffer->len >= 0. Therefore, the
integer underflow cannot currently occur. In the worst case the subtraction
would lead to a '0'.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 hw/tpm/tpm_crb.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index e5e33a3a2f..9a9677db6b 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -180,10 +180,17 @@ static void tpm_crb_fill_command_response(CRBState *s)
      * to the linux guest in chunks by writing it back to MMIO region.
      */
     void *mem = memory_region_get_ram_ptr(&s->cmdmem);
-    uint32_t remaining = s->response_buffer->len - s->response_offset;
-    uint32_t to_copy = MIN(CRB_CTRL_CMD_SIZE, remaining);
+    uint32_t remaining = 0;
+    uint32_t to_copy;
 
-    memcpy(mem, s->response_buffer->data + s->response_offset, to_copy);
+    if (s->response_offset < s->response_buffer->len) {
+        remaining = s->response_buffer->len - s->response_offset;
+    }
+    to_copy = MIN(CRB_CTRL_CMD_SIZE, remaining);
+
+    if (to_copy) {
+        memcpy(mem, s->response_buffer->data + s->response_offset, to_copy);
+    }
 
     if (to_copy < CRB_CTRL_CMD_SIZE) {
         memset((guint8 *)mem + to_copy, 0, CRB_CTRL_CMD_SIZE - to_copy);
-- 
2.55.0

[PATCH 08/10] hw/tpm: crb: Free ppi.buf in tpm_crb_unrealize

[Stefan Berger]

Fix a memory leak by freeing ppi.buf in tpm_crb_unrealize.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 hw/tpm/tpm_crb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 9a9677db6b..4bd1919952 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -16,6 +16,7 @@
 
 #include "qemu/osdep.h"
 
+#include "qemu/memalign.h"
 #include "qemu/module.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
@@ -529,6 +530,8 @@ static void tpm_crb_unrealize(DeviceState *dev)
     if (s->migration_blocker) {
         migrate_del_blocker(&s->migration_blocker);
     }
+
+    qemu_vfree(s->ppi.buf);
 }
 
 static void tpm_crb_class_init(ObjectClass *klass, const void *data)
-- 
2.55.0

[PATCH 09/10] hw/tpm: crb: Consider response_buffer->len of received response

[Stefan Berger]

When the CRB receives a TPM reponse from the backend, then reject responses
that are shorter than the TPM_HEADER_SIZE. When determining the size of the
reponse, also consider the length of the response_buffer as being possibly
shorter than the negotiated backend buffer size or the size indicated in
the response itself.

Fixes: 2a660ad67d15 ("hw/tpm: Implement TPM CRB chunking logic")
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 hw/tpm/tpm_crb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 4bd1919952..6ae2261b43 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -325,13 +325,15 @@ static void tpm_crb_request_completed(TPMIf *ti, int ret)
     CRBState *s = CRB(ti);
 
     ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, Start, 0);
-    if (ret != 0) {
+    if (ret != 0 || s->response_buffer->len < TPM_HEADER_SIZE) {
         ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
                          tpmSts, 1); /* fatal error */
         tpm_crb_clear_internal_buffers(s);
     } else {
         uint32_t actual_resp_size = tpm_cmd_get_size(s->response_buffer->data);
         uint32_t total_resp_size = MIN(actual_resp_size, s->be_buffer_size);
+
+        total_resp_size = MIN(total_resp_size, s->response_buffer->len);
         g_byte_array_set_size(s->response_buffer, total_resp_size);
         s->response_offset = 0;
     }
-- 
2.55.0

[PATCH 10/10] tests: Add tests with commands with bad length

[Stefan Berger]

Add two test cases with commands with bad length:

- Send a command that indicates a very large size (0x10000 bytes) to the
  TPM. This will only work with the CRB interface since the TIS wants
  to receive all the bytes indicated by the command header.

- Send a command that indicates that its size is only 9 bytes, which is
  too short to be valid since the command header alone is 10 bytes.
  This test only works with the TIS. The CRB silently rejects commands
  that are shorter than the command header.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 tests/qtest/tpm-crb-swtpm-test.c        | 10 +++++
 tests/qtest/tpm-tests.c                 | 56 +++++++++++++++++++++++++
 tests/qtest/tpm-tests.h                 |  8 ++++
 tests/qtest/tpm-tis-device-swtpm-test.c | 10 +++++
 tests/qtest/tpm-tis-swtpm-test.c        | 10 +++++
 tests/qtest/tpm-util.c                  | 55 ++++++++++++++++++++++++
 tests/qtest/tpm-util.h                  |  3 ++
 7 files changed, 152 insertions(+)

diff --git a/tests/qtest/tpm-crb-swtpm-test.c b/tests/qtest/tpm-crb-swtpm-test.c
index 050c7b0c1f..0e83a5325f 100644
--- a/tests/qtest/tpm-crb-swtpm-test.c
+++ b/tests/qtest/tpm-crb-swtpm-test.c
@@ -33,6 +33,14 @@ static void tpm_crb_swtpm_test(const void *data)
                         "tpm-crb", NULL);
 }
 
+static void tpm_crb_short_write_swtpm_test(const void *data)
+{
+    const TestState *ts = data;
+
+    tpm_test_short_write_swtpm_test(ts->src_tpm_path, tpm_util_crb_transfer,
+                                    "tpm-crb", NULL);
+}
+
 static void tpm_crb_chunk_swtpm_test(const void *data)
 {
     const TestState *ts = data;
@@ -62,6 +70,8 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_data_func("/tpm/crb-swtpm/test", &ts, tpm_crb_swtpm_test);
+    qtest_add_data_func("/tpm/crb-short-write-swtpm/test", &ts,
+                        tpm_crb_short_write_swtpm_test);
     qtest_add_data_func("/tpm/crb-chunk-swtpm/test", &ts,
                         tpm_crb_chunk_swtpm_test);
     qtest_add_data_func("/tpm/crb-swtpm-migration/test", &ts,
diff --git a/tests/qtest/tpm-tests.c b/tests/qtest/tpm-tests.c
index 1b4f932b0c..74b22c43aa 100644
--- a/tests/qtest/tpm-tests.c
+++ b/tests/qtest/tpm-tests.c
@@ -79,6 +79,62 @@ void tpm_test_swtpm_test(const char *src_tpm_path, tx_func *tx,
     qapi_free_SocketAddress(addr);
 }
 
+/* Setup the TPM and send a command by calling a passed function */
+static void
+tpm_test_simple_cmd_swtpm_test(const char *src_tpm_path, tx_func *tx,
+                               const char *ifmodel,
+                               const char *machine_options,
+                               void (*test_func)(QTestState *, tx_func))
+{
+    char *args = NULL;
+    QTestState *s;
+    SocketAddress *addr = NULL;
+    gboolean succ;
+    GPid swtpm_pid;
+    GError *error = NULL;
+
+    if (tpm_test_swtpm_skip()) {
+        return;
+    }
+
+    succ = tpm_util_swtpm_start(src_tpm_path, &swtpm_pid, &addr, &error);
+    g_assert_true(succ);
+
+    args = g_strdup_printf(
+        "%s "
+        "-chardev socket,id=chr,path=%s "
+        "-tpmdev emulator,id=tpm0,chardev=chr "
+        "-device %s,tpmdev=tpm0",
+        machine_options ? : "", addr->u.q_unix.path, ifmodel);
+
+    s = qtest_start(args);
+    g_free(args);
+
+    test_func(s, tx);
+
+    qtest_end();
+    tpm_util_swtpm_kill(swtpm_pid);
+
+    g_unlink(addr->u.q_unix.path);
+    qapi_free_SocketAddress(addr);
+}
+
+void tpm_test_short_write_swtpm_test(const char *src_tpm_path, tx_func *tx,
+                                     const char *ifmodel,
+                                     const char *machine_options)
+{
+    tpm_test_simple_cmd_swtpm_test(src_tpm_path, tx, ifmodel, machine_options,
+                                   tpm_util_short_write);
+}
+
+void tpm_test_too_short_cmd_swtpm_test(const char *src_tpm_path, tx_func *tx,
+                                       const char *ifmodel,
+                                       const char *machine_options)
+{
+    tpm_test_simple_cmd_swtpm_test(src_tpm_path, tx, ifmodel, machine_options,
+                                   tpm_util_too_short_cmd);
+}
+
 void tpm_test_swtpm_migration_test(const char *src_tpm_path,
                                    const char *dst_tpm_path,
                                    const char *uri, tx_func *tx,
diff --git a/tests/qtest/tpm-tests.h b/tests/qtest/tpm-tests.h
index 07ba60d26e..7684abf8f4 100644
--- a/tests/qtest/tpm-tests.h
+++ b/tests/qtest/tpm-tests.h
@@ -18,6 +18,14 @@
 void tpm_test_swtpm_test(const char *src_tpm_path, tx_func *tx,
                          const char *ifmodel, const char *machine_options);
 
+void tpm_test_short_write_swtpm_test(const char *src_tpm_path, tx_func *tx,
+                                     const char *ifmodel,
+                                     const char *machine_options);
+
+void tpm_test_too_short_cmd_swtpm_test(const char *src_tpm_path, tx_func *tx,
+                                       const char *ifmodel,
+                                       const char *machine_options);
+
 void tpm_test_swtpm_migration_test(const char *src_tpm_path,
                                    const char *dst_tpm_path,
                                    const char *uri, tx_func *tx,
diff --git a/tests/qtest/tpm-tis-device-swtpm-test.c b/tests/qtest/tpm-tis-device-swtpm-test.c
index 517a077005..48a7389306 100644
--- a/tests/qtest/tpm-tis-device-swtpm-test.c
+++ b/tests/qtest/tpm-tis-device-swtpm-test.c
@@ -38,6 +38,14 @@ static void tpm_tis_swtpm_test(const void *data)
                         "tpm-tis-device", MACHINE_OPTIONS);
 }
 
+static void tpm_tis_too_short_cmd_swtpm_test(const void *data)
+{
+    const TestState *ts = data;
+
+    tpm_test_too_short_cmd_swtpm_test(ts->src_tpm_path, tpm_tis_transfer,
+                                      "tpm-tis-device", MACHINE_OPTIONS);
+}
+
 static void tpm_tis_swtpm_migration_test(const void *data)
 {
     const TestState *ts = data;
@@ -62,6 +70,8 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_data_func("/tpm/tis-swtpm/test", &ts, tpm_tis_swtpm_test);
+    qtest_add_data_func("/tpm/tis-too-short-cmd-swtpm/test", &ts,
+                        tpm_tis_too_short_cmd_swtpm_test);
     qtest_add_data_func("/tpm/tis-swtpm-migration/test", &ts,
                         tpm_tis_swtpm_migration_test);
     ret = g_test_run();
diff --git a/tests/qtest/tpm-tis-swtpm-test.c b/tests/qtest/tpm-tis-swtpm-test.c
index 105e42e21d..14b4cbce21 100644
--- a/tests/qtest/tpm-tis-swtpm-test.c
+++ b/tests/qtest/tpm-tis-swtpm-test.c
@@ -36,6 +36,14 @@ static void tpm_tis_swtpm_test(const void *data)
                         "tpm-tis", NULL);
 }
 
+static void tpm_tis_too_short_cmd_swtpm_test(const void *data)
+{
+    const TestState *ts = data;
+
+    tpm_test_too_short_cmd_swtpm_test(ts->src_tpm_path, tpm_tis_transfer,
+                                      "tpm-tis", NULL);
+}
+
 static void tpm_tis_swtpm_migration_test(const void *data)
 {
     const TestState *ts = data;
@@ -57,6 +65,8 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_data_func("/tpm/tis-swtpm/test", &ts, tpm_tis_swtpm_test);
+    qtest_add_data_func("/tpm/tis-too-short-cmd-swtpm/test", &ts,
+                        tpm_tis_too_short_cmd_swtpm_test);
     qtest_add_data_func("/tpm/tis-swtpm-migration/test", &ts,
                         tpm_tis_swtpm_migration_test);
     ret = g_test_run();
diff --git a/tests/qtest/tpm-util.c b/tests/qtest/tpm-util.c
index 151002ea24..d2042ed5df 100644
--- a/tests/qtest/tpm-util.c
+++ b/tests/qtest/tpm-util.c
@@ -194,6 +194,61 @@ void tpm_util_pcrread(QTestState *s, tx_func *tx,
                     &exp_resp[14], exp_resp_size - 14);
 }
 
+/*
+ * Write a TPM command with too large length indicator
+ * The CRB will send the command to the TPM.
+ * The TIS will never send the command since it wants to receive the number of
+ * bytes indicate in the command.
+ */
+void tpm_util_short_write(QTestState *s, tx_func *tx)
+{
+    unsigned char buffer[1024] = { 0 };
+    static const unsigned char tpm_bad_command[] = {
+        0x80, 0x01,
+        0x00, 0x01, 0x00, 0x00, /* length = 0x10000 */
+        0x00, 0x00, 0x01, 0x7e,
+        0x00, 0x00, 0x00, 0x01, 0x00, 0x0b, 0x03, 0x00, 0x04, 0x00
+    };
+
+    tx(s, tpm_bad_command, sizeof(tpm_bad_command), buffer, sizeof(buffer));
+
+    static const unsigned char tpm_error_resp[] = {
+        0x80, 0x01,
+        0x00, 0x00, 0x00, 0x0a,
+        0x00, 0x00, 0x01, 0x42, /* TPM_RC_COMMAND_SIZE */
+    };
+
+    g_assert_cmpmem(buffer, sizeof(tpm_error_resp),
+                    tpm_error_resp, sizeof(tpm_error_resp));
+}
+
+/*
+ * Write a TPM command that is shorter than the header.
+ * The CRB will not send this command.
+ * The TIS will send the command to the backend (needs to be at least
+ * 6 bytes).
+ */
+void tpm_util_too_short_cmd(QTestState *s, tx_func *tx)
+{
+    unsigned char buffer[1024] = { 0 };
+    static const unsigned char tpm_bad_command[] = {
+        0x80, 0x01,
+        0x00, 0x00, 0x00, 0x09, /* 9 bytes command is too short */
+        0x00, 0x00, 0x01,
+    };
+
+    tx(s, tpm_bad_command, sizeof(tpm_bad_command), buffer, sizeof(buffer));
+
+    static const unsigned char tpm_error_resp[] = {
+        0x80, 0x01,
+        0x00, 0x00, 0x00, 0x0a,
+        0x00, 0x00, 0x01, 0x01, /* TPM_RC_FAILURE from tpm_emulator.c */
+    };
+
+    g_assert_cmpmem(buffer, sizeof(tpm_error_resp),
+                    tpm_error_resp, sizeof(tpm_error_resp));
+}
+
 bool tpm_util_swtpm_has_tpm2(void)
 {
     bool has_tpm2 = false;
diff --git a/tests/qtest/tpm-util.h b/tests/qtest/tpm-util.h
index 681544e7d8..70c2fc09c6 100644
--- a/tests/qtest/tpm-util.h
+++ b/tests/qtest/tpm-util.h
@@ -38,6 +38,9 @@ void tpm_util_pcrextend(QTestState *s, tx_func *tx);
 void tpm_util_pcrread(QTestState *s, tx_func *tx,
                       const unsigned char *exp_resp, size_t exp_resp_size);
 
+void tpm_util_short_write(QTestState *s, tx_func *tx);
+void tpm_util_too_short_cmd(QTestState *s, tx_func *tx);
+
 bool tpm_util_swtpm_has_tpm2(void);
 
 gboolean tpm_util_swtpm_start(const char *path, GPid *pid,
-- 
2.55.0

Re: [PATCH 04/10] hw/tpm: tis: Only read from response buffer if rw_offset < len

[marcandre.lureau]

> Only read a byte from the response buffer, if the rw_offset < len.

OOB read from malicious driver is CVE-level.

> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Message-ID: <20260924012527.858527-5-stefanb@linux.ibm.com>
>
> diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c
> index a134d5c20599..4ba228116b64 100644
> --- a/hw/tpm/tpm_tis_common.c
> +++ b/hw/tpm/tpm_tis_common.c
> @@ -273,13 +273,15 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
>          len = MIN(tpm_cmd_get_size(&s->buffer),
>                    s->be_buffer_size);
>  
> -        ret = s->buffer[s->rw_offset++];
> +        if (s->rw_offset < len) {
> +            ret = s->buffer[s->rw_offset++];
> +            trace_tpm_tis_data_read(ret, s->rw_offset - 1);
> +        }
>          if (s->rw_offset >= len) {
>              /* got last byte */
>              tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_VALID);
>              tpm_tis_raise_irq(s, locty, TPM_TIS_INT_STS_VALID);
>          }
> -        trace_tpm_tis_data_read(ret, s->rw_offset - 1);
>      }
>  
>      return ret;

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 01/10] tpm_util: Pass TPMVersion to function writing fatal error response

[marcandre.lureau]

> Have tpm_util_write_fatal_error_response write a TPM version specific
> error response with error code TPM_FAIL for a TPM 1.2 and TPM_RC_FAILURE
> for a TPM 2. Adjust the callers.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Message-ID: <20260924012527.858527-2-stefanb@linux.ibm.com>
>
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index 95d2de2f48b8..16ae44c8d722 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -270,7 +270,8 @@ static void tpm_emulator_handle_request(TPMBackend *tb, TPMBackendCmd *cmd,
>          tpm_emulator_unix_tx_bufs(tpm_emu, cmd->in, cmd->in_len,
>                                    cmd->out, cmd->out_len,
>                                    &cmd->selftest_done, errp) < 0) {
> -        tpm_util_write_fatal_error_response(cmd->out, cmd->out_len);
> +        tpm_util_write_fatal_error_response(cmd->out, cmd->out_len,
> +                                            TPM_EMULATOR(tb)->tpm_version);
>      }
>  }
>  
> diff --git a/backends/tpm/tpm_passthrough.c b/backends/tpm/tpm_passthrough.c
> index a9f35ab7d606..8a4f8183c7c9 100644
> --- a/backends/tpm/tpm_passthrough.c
> +++ b/backends/tpm/tpm_passthrough.c
> @@ -118,7 +118,7 @@ static void tpm_passthrough_unix_tx_bufs(TPMPassthruState *tpm_pt,
>  
>  err_exit:
>      if (ret < 0) {
> -        tpm_util_write_fatal_error_response(out, out_len);
> +        tpm_util_write_fatal_error_response(out, out_len, tpm_pt->tpm_version);
>      }
>  
>      tpm_pt->tpm_executing = false;
> diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
> index 264bff22a91a..cdb7a4886104 100644
> --- a/backends/tpm/tpm_util.c
> +++ b/backends/tpm/tpm_util.c
> @@ -87,12 +87,18 @@ const PropertyInfo qdev_prop_tpm = {
>  /*
>   * Write an error message in the given output buffer.
>   */
> -void tpm_util_write_fatal_error_response(uint8_t *out, uint32_t out_len)
> +void tpm_util_write_fatal_error_response(uint8_t *out, uint32_t out_len,
> +                                         TPMVersion tpm_version)
>  {
>      if (out_len >= sizeof(struct tpm_resp_hdr)) {
> -        tpm_cmd_set_tag(out, TPM_TAG_RSP_COMMAND);
> +        if (tpm_version == TPM_VERSION_1_2) {
> +            tpm_cmd_set_tag(out, TPM_TAG_RSP_COMMAND);
> +            tpm_cmd_set_error(out, TPM_FAIL);
> +        } else {

I would rather use a switch/case than fallback

> +            tpm_cmd_set_tag(out, TPM2_ST_NO_SESSIONS);
> +            tpm_cmd_set_error(out, TPM_RC_FAILURE);
> +        }
>          tpm_cmd_set_size(out, sizeof(struct tpm_resp_hdr));
> -        tpm_cmd_set_error(out, TPM_FAIL);
>      }
>  }
>  
> diff --git a/include/system/tpm_util.h b/include/system/tpm_util.h
> index 185869322571..3ab2fe693df1 100644
> --- a/include/system/tpm_util.h
> +++ b/include/system/tpm_util.h
> @@ -25,7 +25,8 @@
>  #include "system/tpm.h"
>  #include "qemu/bswap.h"
>  
> -void tpm_util_write_fatal_error_response(uint8_t *out, uint32_t out_len);
> +void tpm_util_write_fatal_error_response(uint8_t *out, uint32_t out_len,
> +                                         TPMVersion tpm_version);
>  
>  bool tpm_util_is_selftest(const uint8_t *in, uint32_t in_len);
>  

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 10/10] tests: Add tests with commands with bad length

[marcandre.lureau]

On Wed, 23 Sep 2026 21:25:27 -0400, Stefan Berger <stefanb@linux.ibm.com> wrote:
> Add two test cases with commands with bad length:
> 
> - Send a command that indicates a very large size (0x10000 bytes) to the
>   TPM. This will only work with the CRB interface since the TIS wants
>   to receive all the bytes indicated by the command header.
> 
> - Send a command that indicates that its size is only 9 bytes, which is
>   too short to be valid since the command header alone is 10 bytes.
>   This test only works with the TIS. The CRB silently rejects commands
>   that are shorter than the command header.
> 
> [...]

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 07/10] hw/tpm: crb: Avoid potential integer underflow

[marcandre.lureau]

On Wed, 23 Sep 2026 21:25:24 -0400, Stefan Berger <stefanb@linux.ibm.com> wrote:
> Avoid a potential integer underflow in tpm_crb_fill_command_response
> that could occur if s->response_offset > s->response_buffer->len and
> the function's call sites were to change.
> 
> tpm_crb_fill_command_response currently has 3 callers that either test
> that 's->response_offset < s->response_buffer->len' is true or ensure that
> s->response_offset = 0 and s->response_buffer->len >= 0. Therefore, the
> integer underflow cannot currently occur. In the worst case the subtraction
> would lead to a '0'.
> 
> [...]

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 02/10] tpm_emulator: Reject TPM responses whose size is too small

[marcandre.lureau]

On Wed, 23 Sep 2026 21:25:19 -0400, Stefan Berger <stefanb@linux.ibm.com> wrote:
> Reject a response from the TPM emulator whose size field indicates that the
> response is smaller than the size of the header.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 03/10] tpm_emulator: Reject sending commands smaller than the request header

[marcandre.lureau]

> Reject any TPM request from being sent to the TPM emulator that is shorter
> than the size of the request header.
> 
> Fixes: f4ede81eed29 ("tpm: Added support for TPM emulator")
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Message-ID: <20260924012527.858527-4-stefanb@linux.ibm.com>
>
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index 9c95b4d878b5..0c9e32f36a53 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -181,6 +181,11 @@ static int tpm_emulator_unix_tx_bufs(TPMEmulator *tpm_emu,
>      uint32_t to_read;
>      ssize_t ret;
>  
> +    if (in_len < sizeof(struct tpm_req_hdr)) {
> +        error_setg(errp, "tpm-emulator: invalid request size %u", in_len);
> +        return -1;
> +    }

It could probably be an assert() instead.

> +
>      if (selftest_done) {
>          *selftest_done = false;
>          is_selftest = tpm_util_is_selftest(in, in_len);

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 09/10] hw/tpm: crb: Consider response_buffer->len of received response

[marcandre.lureau]

> When the CRB receives a TPM reponse from the backend, then reject responses
> that are shorter than the TPM_HEADER_SIZE. When determining the size of the
> reponse, also consider the length of the response_buffer as being possibly
> shorter than the negotiated backend buffer size or the size indicated in
> the response itself.

Since the backend is considered trusted, this is not a security issue.

> 
> Fixes: 2a660ad67d15 ("hw/tpm: Implement TPM CRB chunking logic")
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Message-ID: <20260924012527.858527-10-stefanb@linux.ibm.com>
>
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index 4bd1919952c1..6ae2261b43e3 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -325,13 +325,15 @@ static void tpm_crb_request_completed(TPMIf *ti, int ret)
>      CRBState *s = CRB(ti);
>  
>      ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, Start, 0);
> -    if (ret != 0) {
> +    if (ret != 0 || s->response_buffer->len < TPM_HEADER_SIZE) {
>          ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
>                           tpmSts, 1); /* fatal error */
>          tpm_crb_clear_internal_buffers(s);
>      } else {
>          uint32_t actual_resp_size = tpm_cmd_get_size(s->response_buffer->data);
>          uint32_t total_resp_size = MIN(actual_resp_size, s->be_buffer_size);
> +
> +        total_resp_size = MIN(total_resp_size, s->response_buffer->len);
>          g_byte_array_set_size(s->response_buffer, total_resp_size);
>          s->response_offset = 0;
>      }

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 05/10] hw/tpm: tis: Use uint32_t for len to avoid truncating size of command

[marcandre.lureau]

On Wed, 23 Sep 2026 21:25:22 -0400, Stefan Berger <stefanb@linux.ibm.com> wrote:
> To avoid truncating the uint32_t return value from tpm_cmd_get_size()
> declare len as uint32_t rather than uint16_t.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 08/10] hw/tpm: crb: Free ppi.buf in tpm_crb_unrealize

[marcandre.lureau]

> Fix a memory leak by freeing ppi.buf in tpm_crb_unrealize.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Message-ID: <20260924012527.858527-9-stefanb@linux.ibm.com>
>
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index 9a9677db6b19..4bd1919952c1 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -16,6 +16,7 @@
>  
>  #include "qemu/osdep.h"
>  
> +#include "qemu/memalign.h"
>  #include "qemu/module.h"
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
> @@ -529,6 +530,8 @@ static void tpm_crb_unrealize(DeviceState *dev)
>      if (s->migration_blocker) {
>          migrate_del_blocker(&s->migration_blocker);
>      }
> +
> +    qemu_vfree(s->ppi.buf);

We should have tpm_ppi_uninit(), there are other things that needs
to be taken care of, such add memory region and vmstate...

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 06/10] hw/tpm: crb: Do not clear internal buffers when backend is busy

[marcandre.lureau]

> While the backend (thread) is accessing the request and response buffers,
> do not clear those buffers upon a command cancellation. Still clear them
> in all other cases, though.
> 
> Fixes: 29315d22b259 ("hw/tpm: Add internal buffer state for chunking")

It's the following commit that makes use of it, but ok

This may also lead to OOB read/crashes from malicious guest driver.

> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Message-ID: <20260924012527.858527-7-stefanb@linux.ibm.com>
>
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index 54fa2042b5a8..e5e33a3a2ff4 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -218,8 +218,9 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
>          if (val == CRB_CANCEL_INVOKE) {
>              if (s->regs[R_CRB_CTRL_START] & CRB_START_INVOKE) {
>                  tpm_backend_cancel_cmd(s->tpmbe);
> +            } else {
> +                tpm_crb_clear_internal_buffers(s);
>              }
> -            tpm_crb_clear_internal_buffers(s);
>          }
>          break;
>      case A_CRB_CTRL_START:

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 01/10] tpm_util: Pass TPMVersion to function writing fatal error response

[Stefan Berger]

On 9/24/26 3:41 AM, marcandre.lureau@redhat.com wrote:
>> Have tpm_util_write_fatal_error_response write a TPM version specific
>> error response with error code TPM_FAIL for a TPM 1.2 and TPM_RC_FAILURE
>> for a TPM 2. Adjust the callers.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Message-ID: <20260924012527.858527-2-stefanb@linux.ibm.com>
>>
>> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
>> index 95d2de2f48b8..16ae44c8d722 100644
>> --- a/backends/tpm/tpm_emulator.c
>> +++ b/backends/tpm/tpm_emulator.c
>> @@ -270,7 +270,8 @@ static void tpm_emulator_handle_request(TPMBackend *tb, TPMBackendCmd *cmd,
>>           tpm_emulator_unix_tx_bufs(tpm_emu, cmd->in, cmd->in_len,
>>                                     cmd->out, cmd->out_len,
>>                                     &cmd->selftest_done, errp) < 0) {
>> -        tpm_util_write_fatal_error_response(cmd->out, cmd->out_len);
>> +        tpm_util_write_fatal_error_response(cmd->out, cmd->out_len,
>> +                                            TPM_EMULATOR(tb)->tpm_version);
>>       }
>>   }
>>   
>> diff --git a/backends/tpm/tpm_passthrough.c b/backends/tpm/tpm_passthrough.c
>> index a9f35ab7d606..8a4f8183c7c9 100644
>> --- a/backends/tpm/tpm_passthrough.c
>> +++ b/backends/tpm/tpm_passthrough.c
>> @@ -118,7 +118,7 @@ static void tpm_passthrough_unix_tx_bufs(TPMPassthruState *tpm_pt,
>>   
>>   err_exit:
>>       if (ret < 0) {
>> -        tpm_util_write_fatal_error_response(out, out_len);
>> +        tpm_util_write_fatal_error_response(out, out_len, tpm_pt->tpm_version);
>>       }
>>   
>>       tpm_pt->tpm_executing = false;
>> diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
>> index 264bff22a91a..cdb7a4886104 100644
>> --- a/backends/tpm/tpm_util.c
>> +++ b/backends/tpm/tpm_util.c
>> @@ -87,12 +87,18 @@ const PropertyInfo qdev_prop_tpm = {
>>   /*
>>    * Write an error message in the given output buffer.
>>    */
>> -void tpm_util_write_fatal_error_response(uint8_t *out, uint32_t out_len)
>> +void tpm_util_write_fatal_error_response(uint8_t *out, uint32_t out_len,
>> +                                         TPMVersion tpm_version)
>>   {
>>       if (out_len >= sizeof(struct tpm_resp_hdr)) {
>> -        tpm_cmd_set_tag(out, TPM_TAG_RSP_COMMAND);
>> +        if (tpm_version == TPM_VERSION_1_2) {
>> +            tpm_cmd_set_tag(out, TPM_TAG_RSP_COMMAND);
>> +            tpm_cmd_set_error(out, TPM_FAIL);
>> +        } else {
> 
> I would rather use a switch/case than fallback

Will change for v2.

Re: [PATCH 03/10] tpm_emulator: Reject sending commands smaller than the request header

[Stefan Berger]

On 9/24/26 3:41 AM, marcandre.lureau@redhat.com wrote:
>> Reject any TPM request from being sent to the TPM emulator that is shorter
>> than the size of the request header.
>>
>> Fixes: f4ede81eed29 ("tpm: Added support for TPM emulator")
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Message-ID: <20260924012527.858527-4-stefanb@linux.ibm.com>
>>
>> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
>> index 9c95b4d878b5..0c9e32f36a53 100644
>> --- a/backends/tpm/tpm_emulator.c
>> +++ b/backends/tpm/tpm_emulator.c
>> @@ -181,6 +181,11 @@ static int tpm_emulator_unix_tx_bufs(TPMEmulator *tpm_emu,
>>       uint32_t to_read;
>>       ssize_t ret;
>>   
>> +    if (in_len < sizeof(struct tpm_req_hdr)) {
>> +        error_setg(errp, "tpm-emulator: invalid request size %u", in_len);
>> +        return -1;
>> +    }
> 
> It could probably be an assert() instead.

The TIS may actually send a command with only 6 bytes.

> 
>> +
>>       if (selftest_done) {
>>           *selftest_done = false;
>>           is_selftest = tpm_util_is_selftest(in, in_len);
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 

Re: [PATCH 04/10] hw/tpm: tis: Only read from response buffer if rw_offset < len

[Stefan Berger]

On 9/24/26 3:41 AM, marcandre.lureau@redhat.com wrote:
>> Only read a byte from the response buffer, if the rw_offset < len.
> 
> OOB read from malicious driver is CVE-level.

It would not read outside the allocated bounds of the s->buffer but 
outside of the bounds of the returned TPM response. But this is only 
possible if 0 bytes were returned, so it would only ever read s->buffer[0].
The foollowing line then resets the TPM_TIS_STS_DATA_AVAILABLE flag so 
that it would not read another byte. IMO, this is not a CVE.

tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_VALID);


> 
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Message-ID: <20260924012527.858527-5-stefanb@linux.ibm.com>
>>
>> diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c
>> index a134d5c20599..4ba228116b64 100644
>> --- a/hw/tpm/tpm_tis_common.c
>> +++ b/hw/tpm/tpm_tis_common.c
>> @@ -273,13 +273,15 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
>>           len = MIN(tpm_cmd_get_size(&s->buffer),
>>                     s->be_buffer_size);
>>   
>> -        ret = s->buffer[s->rw_offset++];
>> +        if (s->rw_offset < len) {
>> +            ret = s->buffer[s->rw_offset++];
>> +            trace_tpm_tis_data_read(ret, s->rw_offset - 1);
>> +        }
>>           if (s->rw_offset >= len) {
>>               /* got last byte */
>>               tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_VALID);
>>               tpm_tis_raise_irq(s, locty, TPM_TIS_INT_STS_VALID);
>>           }
>> -        trace_tpm_tis_data_read(ret, s->rw_offset - 1);
>>       }
>>   
>>       return ret;
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 

Re: [PATCH 06/10] hw/tpm: crb: Do not clear internal buffers when backend is busy

[Stefan Berger]

On 9/24/26 3:41 AM, marcandre.lureau@redhat.com wrote:
>> While the backend (thread) is accessing the request and response buffers,
>> do not clear those buffers upon a command cancellation. Still clear them
>> in all other cases, though.
>>
>> Fixes: 29315d22b259 ("hw/tpm: Add internal buffer state for chunking")
> 
> It's the following commit that makes use of it, but ok
> 
> This may also lead to OOB read/crashes from malicious guest driver.

For example g_byte_array_set_size(s->response_buffer, 0) does not seem 
to deallocate the memory in s->response_buffer just because it was set 
to size 0.

Running the tests/qtest/tpm-crb-swtpm-test with this modification here 
requires me to disable asserts for expected responses but no crash 
actually occurs.

@@ -254,6 +254,7 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr 
addr,
                  .out_len = s->response_buffer->len,
              };
              tpm_backend_deliver_request(s->tpmbe, &s->cmd);
+            tpm_crb_clear_internal_buffers(s);
          } else if (val & CRB_START_NEXT_CHUNK) {
              if (!s->cap_chunk) {
                  break;


> 
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Message-ID: <20260924012527.858527-7-stefanb@linux.ibm.com>
>>
>> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
>> index 54fa2042b5a8..e5e33a3a2ff4 100644
>> --- a/hw/tpm/tpm_crb.c
>> +++ b/hw/tpm/tpm_crb.c
>> @@ -218,8 +218,9 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
>>           if (val == CRB_CANCEL_INVOKE) {
>>               if (s->regs[R_CRB_CTRL_START] & CRB_START_INVOKE) {
>>                   tpm_backend_cancel_cmd(s->tpmbe);
>> +            } else {
>> +                tpm_crb_clear_internal_buffers(s);
>>               }
>> -            tpm_crb_clear_internal_buffers(s);
>>           }
>>           break;
>>       case A_CRB_CTRL_START:
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>