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

[Marc-Andre Lureau]

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.