Hi On Mon, Jan 29, 2018 at 6:57 PM, Stefan Berger <stef...@linux.vnet.ibm.com> wrote: > On 01/29/2018 12:51 PM, Marc-Andre Lureau wrote: >> >> Hi >> >> On Mon, Jan 29, 2018 at 6:41 PM, Stefan Berger >> <stef...@linux.vnet.ibm.com> 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 <peter.mayd...@linaro.org> >>>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> >>>> --- >>>> 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/20180129123227.15778-1-marcandre.lur...@redhat.com/) There was no conflicts after a rebase of my series. If it helps, I can resend my pending tpm queue.