qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] tpm: fix alignment issues

From: Stefan Berger
Subject: Re: [Qemu-devel] [PATCH] tpm: fix alignment issues
Date: Mon, 29 Jan 2018 12:57:31 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 
On 01/29/2018 12:51 PM, Marc-Andre Lureau wrote:

Hi

On Mon, Jan 29, 2018 at 6:41 PM, Stefan Berger
<address@hidden> wrote:

On 01/29/2018 07:32 AM, Marc-André Lureau wrote:

The new tpm-crb-test fails on sparc host:

TEST: tests/tpm-crb-test... (pid=230409)
    /i386/tpm-crb/test:
Broken pipe
FAIL
GTester: last random seed: R02S29cea50247fe1efa59ee885a26d51a85
(pid=230423)
FAIL: tests/tpm-crb-test

and generates a new clang sanitizer runtime warning:

/home/petmay01/linaro/qemu-for-merges/hw/tpm/tpm_util.h:36:24: runtime
error: load of misaligned address 0x7fdc24c00002 for type 'const
uint32_t' (aka 'const unsigned int'), which requires 4 byte alignment
0x7fdc24c00002: note: pointer points here
<memory cannot be printed>

The sparc architecture does not allow misaligned loads and will
segfault if you try them.  For example, this function:

static inline uint32_t tpm_cmd_get_size(const void *b)
{
      return be32_to_cpu(*(const uint32_t *)(b + 2));
}

Should read,
      return ldl_be_p(b + 2);

As a general rule you can't take an arbitrary pointer into a byte
buffer and try to interpret it as a structure or a pointer to a
larger-than-bytesize-data simply by casting the pointer.

Use this clean up as an opportunity to remove unnecessary temporary
buffers and casts.

Reported-by: Peter Maydell <address@hidden>
Signed-off-by: Marc-André Lureau <address@hidden>
---
   hw/tpm/tpm_util.h        | 17 ++++++++++-
   hw/tpm/tpm_emulator.c    | 14 ++++-----
   hw/tpm/tpm_passthrough.c |  6 ++--
   hw/tpm/tpm_util.c        | 75
++++++++++++++++++++++--------------------------
   4 files changed, 58 insertions(+), 54 deletions(-)

diff --git a/hw/tpm/tpm_util.h b/hw/tpm/tpm_util.h
index 19b28474ae..c562140e52 100644
--- a/hw/tpm/tpm_util.h
+++ b/hw/tpm/tpm_util.h
@@ -31,9 +31,24 @@ bool tpm_util_is_selftest(const uint8_t *in, uint32_t
in_len);

   int tpm_util_test_tpmdev(int tpm_fd, TPMVersion *tpm_version);

+static inline uint16_t tpm_cmd_get_tag(const void *b)
+{
+    return lduw_be_p(b);;
+}
+
   static inline uint32_t tpm_cmd_get_size(const void *b)
   {
-    return be32_to_cpu(*(const uint32_t *)(b + 2));
+    return ldl_be_p(b + 2);;


Why are there these two ';;' ?


obvious typo (repeated..)


+}
+
+static inline uint32_t tpm_cmd_get_ordinal(const void *b)
+{
+    return ldl_be_p(b + 6);;
+}
+
+static inline uint32_t tpm_cmd_get_errcode(const void *b)
+{
+    return ldl_be_p(b + 6);;
   }

   int tpm_util_get_buffer_size(int tpm_fd, TPMVersion tpm_version,
diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index 35c78de5a9..a34a18ac7a 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -120,7 +120,6 @@ static int tpm_emulator_unix_tx_bufs(TPMEmulator
*tpm_emu,
   {
       ssize_t ret;
       bool is_selftest = false;
-    const struct tpm_resp_hdr *hdr = NULL;

       if (selftest_done) {
           *selftest_done = false;
@@ -132,22 +131,21 @@ static int tpm_emulator_unix_tx_bufs(TPMEmulator
*tpm_emu,
           return -1;
       }

-    ret = qio_channel_read_all(tpm_emu->data_ioc, (char *)out,
sizeof(*hdr),
-                               err);
+    ret = qio_channel_read_all(tpm_emu->data_ioc, (char *)out,
+              sizeof(struct tpm_resp_hdr), err);
       if (ret != 0) {
           return -1;
       }

-    hdr = (struct tpm_resp_hdr *)out;
-    out += sizeof(*hdr);
-    ret = qio_channel_read_all(tpm_emu->data_ioc, (char *)out,
-                               be32_to_cpu(hdr->len) - sizeof(*hdr) ,
err);
+    ret = qio_channel_read_all(tpm_emu->data_ioc,
+              (char *)out + sizeof(struct tpm_resp_hdr),
+              tpm_cmd_get_size(out) - sizeof(struct tpm_resp_hdr), err);
       if (ret != 0) {
           return -1;
       }

       if (is_selftest) {
-        *selftest_done = (be32_to_cpu(hdr->errcode) == 0);
+        *selftest_done = tpm_cmd_get_errcode(out) == 0;
       }

       return 0;
diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index 29142f38bb..537e11a3f9 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -87,7 +87,6 @@ static int tpm_passthrough_unix_tx_bufs(TPMPassthruState
*tpm_pt,
   {
       ssize_t ret;
       bool is_selftest;
-    const struct tpm_resp_hdr *hdr;

       /* FIXME: protect shared variables or use other sync mechanism */
       tpm_pt->tpm_op_canceled = false;
@@ -116,15 +115,14 @@ static int
tpm_passthrough_unix_tx_bufs(TPMPassthruState *tpm_pt,
                            strerror(errno), errno);
           }
       } else if (ret < sizeof(struct tpm_resp_hdr) ||
-               be32_to_cpu(((struct tpm_resp_hdr *)out)->len) != ret) {
+               tpm_cmd_get_size(out) != ret) {
           ret = -1;
           error_report("tpm_passthrough: received invalid response "
                        "packet from TPM");
       }

       if (is_selftest && (ret >= sizeof(struct tpm_resp_hdr))) {
-        hdr = (struct tpm_resp_hdr *)out;
-        *selftest_done = (be32_to_cpu(hdr->errcode) == 0);
+        *selftest_done = tpm_cmd_get_errcode(out) == 0;
       }

   err_exit:
diff --git a/hw/tpm/tpm_util.c b/hw/tpm/tpm_util.c
index 747075e244..8abde59915 100644
--- a/hw/tpm/tpm_util.c
+++ b/hw/tpm/tpm_util.c
@@ -106,20 +106,16 @@ const PropertyInfo qdev_prop_tpm = {
   void tpm_util_write_fatal_error_response(uint8_t *out, uint32_t out_len)
   {
       if (out_len >= sizeof(struct tpm_resp_hdr)) {
-        struct tpm_resp_hdr *resp = (struct tpm_resp_hdr *)out;
-
-        resp->tag = cpu_to_be16(TPM_TAG_RSP_COMMAND);
-        resp->len = cpu_to_be32(sizeof(struct tpm_resp_hdr));
-        resp->errcode = cpu_to_be32(TPM_FAIL);
+        stw_be_p(out, TPM_TAG_RSP_COMMAND);
+        stl_be_p(out + 2, sizeof(struct tpm_resp_hdr));
+        stl_be_p(out + 6, TPM_FAIL);


Since we wrapped the getters we should wrap the setters as well.
tpm_cmd_set_len(), tpm_cmd_set_errcode.

They were not as widely used (there was only a getter so far), but
that makes sense too. Could be done on top.
Also please rebase on top of your series of patches. I had some problems with tpm_passthrough.c around line 115. Error reporting there has changed.
reply via email to
[Prev in Thread] Current Thread [Next in Thread]