From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59695) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1egDyp-0000im-88 for qemu-devel@nongnu.org; Mon, 29 Jan 2018 13:16:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1egDym-0000la-0d for qemu-devel@nongnu.org; Mon, 29 Jan 2018 13:16:31 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:55356 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1egDyl-0000kJ-Pq for qemu-devel@nongnu.org; Mon, 29 Jan 2018 13:16:27 -0500 Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w0TIFEWG175675 for ; Mon, 29 Jan 2018 13:16:27 -0500 Received: from e17.ny.us.ibm.com (e17.ny.us.ibm.com [129.33.205.207]) by mx0b-001b2d01.pphosted.com with ESMTP id 2ft8339kgh-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Mon, 29 Jan 2018 13:16:26 -0500 Received: from localhost by e17.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 29 Jan 2018 13:16:25 -0500 References: <20180129123227.15778-1-marcandre.lureau@redhat.com> <3c1355ed-57af-d023-8da3-21f1600ee018@linux.vnet.ibm.com> From: Stefan Berger Date: Mon, 29 Jan 2018 13:16:23 -0500 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Message-Id: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] tpm: fix alignment issues List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marc-Andre Lureau Cc: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= , qemu-devel On 01/29/2018 01:02 PM, Marc-Andre Lureau wrote: > Hi > > On Mon, Jan 29, 2018 at 6:57 PM, Stefan Berger > wrote: >> On 01/29/2018 12:51 PM, Marc-Andre Lureau wrote: >>> Hi >>> >>> On Mon, Jan 29, 2018 at 6:41 PM, Stefan Berger >>> wrote: >>>> On 01/29/2018 07:32 AM, Marc-Andr=C3=A9 Lureau wrote: >>>>> The new tpm-crb-test fails on sparc host: >>>>> >>>>> TEST: tests/tpm-crb-test... (pid=3D230409) >>>>> /i386/tpm-crb/test: >>>>> Broken pipe >>>>> FAIL >>>>> GTester: last random seed: R02S29cea50247fe1efa59ee885a26d51a85 >>>>> (pid=3D230423) >>>>> 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: runt= ime >>>>> error: load of misaligned address 0x7fdc24c00002 for type 'const >>>>> uint32_t' (aka 'const unsigned int'), which requires 4 byte alignme= nt >>>>> 0x7fdc24c00002: note: pointer points here >>>>> >>>>> >>>>> 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 >>>>> Signed-off-by: Marc-Andr=C3=A9 Lureau >>>>> --- >>>>> 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, uin= t32_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(TPMEmulato= r >>>>> *tpm_emu, >>>>> { >>>>> ssize_t ret; >>>>> bool is_selftest =3D false; >>>>> - const struct tpm_resp_hdr *hdr =3D NULL; >>>>> >>>>> if (selftest_done) { >>>>> *selftest_done =3D false; >>>>> @@ -132,22 +131,21 @@ static int tpm_emulator_unix_tx_bufs(TPMEmula= tor >>>>> *tpm_emu, >>>>> return -1; >>>>> } >>>>> >>>>> - ret =3D qio_channel_read_all(tpm_emu->data_ioc, (char *)out, >>>>> sizeof(*hdr), >>>>> - err); >>>>> + ret =3D qio_channel_read_all(tpm_emu->data_ioc, (char *)out, >>>>> + sizeof(struct tpm_resp_hdr), err); >>>>> if (ret !=3D 0) { >>>>> return -1; >>>>> } >>>>> >>>>> - hdr =3D (struct tpm_resp_hdr *)out; >>>>> - out +=3D sizeof(*hdr); >>>>> - ret =3D qio_channel_read_all(tpm_emu->data_ioc, (char *)out, >>>>> - be32_to_cpu(hdr->len) - sizeof(*hdr= ) , >>>>> err); >>>>> + ret =3D 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 !=3D 0) { >>>>> return -1; >>>>> } >>>>> >>>>> if (is_selftest) { >>>>> - *selftest_done =3D (be32_to_cpu(hdr->errcode) =3D=3D 0); >>>>> + *selftest_done =3D tpm_cmd_get_errcode(out) =3D=3D 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 mechan= ism */ >>>>> tpm_pt->tpm_op_canceled =3D 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) !=3D= ret) { >>>>> + tpm_cmd_get_size(out) !=3D ret) { >>>>> ret =3D -1; >>>>> error_report("tpm_passthrough: received invalid respons= e " >>>>> "packet from TPM"); >>>>> } >>>>> >>>>> if (is_selftest && (ret >=3D sizeof(struct tpm_resp_hdr))) = { >>>>> - hdr =3D (struct tpm_resp_hdr *)out; >>>>> - *selftest_done =3D (be32_to_cpu(hdr->errcode) =3D=3D 0); >>>>> + *selftest_done =3D tpm_cmd_get_errcode(out) =3D=3D 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 =3D { >>>>> void tpm_util_write_fatal_error_response(uint8_t *out, uint32_t >>>>> out_len) >>>>> { >>>>> if (out_len >=3D sizeof(struct tpm_resp_hdr)) { >>>>> - struct tpm_resp_hdr *resp =3D (struct tpm_resp_hdr *)out; >>>>> - >>>>> - resp->tag =3D cpu_to_be16(TPM_TAG_RSP_COMMAND); >>>>> - resp->len =3D cpu_to_be32(sizeof(struct tpm_resp_hdr)); >>>>> - resp->errcode =3D 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 proble= ms >> with tpm_passthrough.c around line 115. Error reporting there has chan= ged. >> >> > 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