|
From: | Stefan Berger |
Subject: | Re: [Qemu-devel] [PATCH] tpm: fix alignment issues |
Date: | Mon, 29 Jan 2018 13:16:23 -0500 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 |
On 01/29/2018 01:02 PM, Marc-Andre Lureau wrote:
Hi On Mon, Jan 29, 2018 at 6:57 PM, Stefan Berger <address@hidden> wrote: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.The patch is meant to be applied first on top of master, to avoid intermediary regressions (it passes patchew: http://patchew.org/QEMU/address@hidden/) There was no conflicts after a rebase of my series. If it helps, I can resend my pending tpm queue.
Please resent the series. Stefan
[Prev in Thread] | Current Thread | [Next in Thread] |