[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] tpm: fix alignment issues
From: |
Marc-Andre Lureau |
Subject: |
Re: [Qemu-devel] [PATCH] tpm: fix alignment issues |
Date: |
Mon, 29 Jan 2018 19:02:04 +0100 |
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.