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

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 01/29/2018 01:02 PM, Marc-Andre Lureau wrote:
> Hi
>
> On Mon, Jan 29, 2018 at 6:57 PM, Stefan Berger
> <stefanb@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
>>> <stefanb@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.maydell@linaro.org>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@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.lureau@redhat.com/)
>
> 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