From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756596AbcKWU4g convert rfc822-to-8bit (ORCPT ); Wed, 23 Nov 2016 15:56:36 -0500 Received: from mga09.intel.com ([134.134.136.24]:49511 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756507AbcKWU4f (ORCPT ); Wed, 23 Nov 2016 15:56:35 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,540,1473145200"; d="scan'208";a="34874185" From: "Winkler, Tomas" To: Jason Gunthorpe CC: "tpmdd-devel@lists.sourceforge.net" , Jarkko Sakkinen , "linux-kernel@vger.kernel.org" Subject: RE: [PATCH] tpm: use get_unaligned_be32 unaligned buffer access. Thread-Topic: [PATCH] tpm: use get_unaligned_be32 unaligned buffer access. Thread-Index: AQHSRaq+f4L5Q1e9wUmDPoLyDQDjB6Dm77Yw Date: Wed, 23 Nov 2016 20:56:25 +0000 Message-ID: <5B8DA87D05A7694D9FA63FD143655C1B543308CB@hasmsx108.ger.corp.intel.com> References: <1479899094-9486-1-git-send-email-tomas.winkler@intel.com> <20161123165734.GA2763@obsidianresearch.com> In-Reply-To: <20161123165734.GA2763@obsidianresearch.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMWYxMmY3YTQtODUxNy00OTdkLTg3MTgtNDY2MGNlNmU3ZWQ2IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Ild5WE9rZ0U4ME9IUXBscDhpb1pTRXVzXC9weWFPd1ZHOHNKaHQ4QmJOaDNFPSJ9 x-originating-ip: [10.184.70.11] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > On Wed, Nov 23, 2016 at 01:04:54PM +0200, Tomas Winkler wrote: > > Use get_unaligned_be32 as b32_to_cpu doesn't work correctly on all > > platforms for unaligned access. > > > > The fix doesn't cover all the cases as also some cast structures have > > members on unaligned addresses. > > I think this is a good idea.. > > > @@ -353,8 +353,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 > *buf, size_t bufsiz, > > if (bufsiz > TPM_BUFSIZE) > > bufsiz = TPM_BUFSIZE; > > > > - count = be32_to_cpu(*((__be32 *) (buf + 2))); > > - ordinal = be32_to_cpu(*((__be32 *) (buf + 6))); > > + count = get_unaligned_be32(buf + 2); > > + ordinal = get_unaligned_be32(buf + 6); > > But lets fix this better and get rid of the constants too... > const tpm_input_header *hdr = buf; > count = be32_to_cpu(hdr->length); > ordinal = be32_to_cpu(hdr->ordinal); > > Compiler will take care of unaligned for __packed. Yes, compiler takes care at performance penalty but probably we don't care about that much, and readability is maybe more important here. What I've done is pretty much mechanical fix, I agree this is better approach in this particular case. > > @@ -178,7 +178,7 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, > size_t count) > > return -EIO; > > > > memcpy_fromio(buf, priv->rsp, 6); > > - expected = be32_to_cpup((__be32 *) &buf[2]); > > + expected = get_unaligned_be32(buf + 2); > > Here too, except tpm_output_header (and is tpm1 and 2 the same here?) > > > @@ -451,7 +452,7 @@ static int tpm_tis_i2c_recv(struct tpm_chip *chip, u8 > *buf, size_t count) > > goto out; > > } > > > > - expected = be32_to_cpu(*(__be32 *)(buf + 2)); > > + expected = get_unaligned_be32(buf + 2); > > Ditto > > > @@ -314,7 +315,7 @@ static int i2c_nuvoton_recv(struct tpm_chip *chip, u8 > *buf, size_t count) > > * convert number of expected bytes field from big endian 32 > bit > > * to machine native > > */ > > - expected = be32_to_cpu(*(__be32 *) (buf + 2)); > > + expected = get_unaligned_be32(buf + 2); > > Ditto > > > if (expected > count) { > > dev_err(dev, "%s() expected > count\n", __func__); > > size = -EIO; > > @@ -442,7 +443,7 @@ static int i2c_nuvoton_send(struct tpm_chip *chip, u8 > *buf, size_t len) > > i2c_nuvoton_ready(chip); > > return rc; > > } > > - ordinal = be32_to_cpu(*((__be32 *) (buf + 6))); > > + ordinal = get_unaligned_be32(buf + 6); > > Ditto > > > @@ -174,8 +173,7 @@ static int tpm_nsc_recv(struct tpm_chip *chip, u8 * > buf, size_t count) > > return -EIO; > > } > > > > - native_size = (__force __be32 *) (buf + 2); > > - size = be32_to_cpu(*native_size); > > + size = get_unaligned_be32(buf + 2); > > Ditto > > > diff --git a/drivers/char/tpm/tpm_tis_core.c > > b/drivers/char/tpm/tpm_tis_core.c index 7993678954a2..5323c54dc917 > > 100644 > > +++ b/drivers/char/tpm/tpm_tis_core.c > > @@ -222,7 +222,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 > *buf, size_t count) > > goto out; > > } > > > > - expected = be32_to_cpu(*(__be32 *) (buf + 2)); > > + expected = get_unaligned_be32(buf + 2); > > Ditto > > > @@ -371,7 +371,7 @@ static int tpm_tis_send_main(struct tpm_chip *chip, > u8 *buf, size_t len) > > goto out_err; > > > > if (chip->flags & TPM_CHIP_FLAG_IRQ) { > > - ordinal = be32_to_cpu(*((__be32 *) (buf + 6))); > > + ordinal = get_unaligned_be32(buf + 6); > > Ditto > > Jason From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Winkler, Tomas" Subject: Re: [PATCH] tpm: use get_unaligned_be32 unaligned buffer access. Date: Wed, 23 Nov 2016 20:56:25 +0000 Message-ID: <5B8DA87D05A7694D9FA63FD143655C1B543308CB@hasmsx108.ger.corp.intel.com> References: <1479899094-9486-1-git-send-email-tomas.winkler@intel.com> <20161123165734.GA2763@obsidianresearch.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20161123165734.GA2763-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: tpmdd-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Jason Gunthorpe Cc: "tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: tpmdd-devel@lists.sourceforge.net > On Wed, Nov 23, 2016 at 01:04:54PM +0200, Tomas Winkler wrote: > > Use get_unaligned_be32 as b32_to_cpu doesn't work correctly on all > > platforms for unaligned access. > > > > The fix doesn't cover all the cases as also some cast structures have > > members on unaligned addresses. > > I think this is a good idea.. > > > @@ -353,8 +353,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 > *buf, size_t bufsiz, > > if (bufsiz > TPM_BUFSIZE) > > bufsiz = TPM_BUFSIZE; > > > > - count = be32_to_cpu(*((__be32 *) (buf + 2))); > > - ordinal = be32_to_cpu(*((__be32 *) (buf + 6))); > > + count = get_unaligned_be32(buf + 2); > > + ordinal = get_unaligned_be32(buf + 6); > > But lets fix this better and get rid of the constants too... > const tpm_input_header *hdr = buf; > count = be32_to_cpu(hdr->length); > ordinal = be32_to_cpu(hdr->ordinal); > > Compiler will take care of unaligned for __packed. Yes, compiler takes care at performance penalty but probably we don't care about that much, and readability is maybe more important here. What I've done is pretty much mechanical fix, I agree this is better approach in this particular case. > > @@ -178,7 +178,7 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, > size_t count) > > return -EIO; > > > > memcpy_fromio(buf, priv->rsp, 6); > > - expected = be32_to_cpup((__be32 *) &buf[2]); > > + expected = get_unaligned_be32(buf + 2); > > Here too, except tpm_output_header (and is tpm1 and 2 the same here?) > > > @@ -451,7 +452,7 @@ static int tpm_tis_i2c_recv(struct tpm_chip *chip, u8 > *buf, size_t count) > > goto out; > > } > > > > - expected = be32_to_cpu(*(__be32 *)(buf + 2)); > > + expected = get_unaligned_be32(buf + 2); > > Ditto > > > @@ -314,7 +315,7 @@ static int i2c_nuvoton_recv(struct tpm_chip *chip, u8 > *buf, size_t count) > > * convert number of expected bytes field from big endian 32 > bit > > * to machine native > > */ > > - expected = be32_to_cpu(*(__be32 *) (buf + 2)); > > + expected = get_unaligned_be32(buf + 2); > > Ditto > > > if (expected > count) { > > dev_err(dev, "%s() expected > count\n", __func__); > > size = -EIO; > > @@ -442,7 +443,7 @@ static int i2c_nuvoton_send(struct tpm_chip *chip, u8 > *buf, size_t len) > > i2c_nuvoton_ready(chip); > > return rc; > > } > > - ordinal = be32_to_cpu(*((__be32 *) (buf + 6))); > > + ordinal = get_unaligned_be32(buf + 6); > > Ditto > > > @@ -174,8 +173,7 @@ static int tpm_nsc_recv(struct tpm_chip *chip, u8 * > buf, size_t count) > > return -EIO; > > } > > > > - native_size = (__force __be32 *) (buf + 2); > > - size = be32_to_cpu(*native_size); > > + size = get_unaligned_be32(buf + 2); > > Ditto > > > diff --git a/drivers/char/tpm/tpm_tis_core.c > > b/drivers/char/tpm/tpm_tis_core.c index 7993678954a2..5323c54dc917 > > 100644 > > +++ b/drivers/char/tpm/tpm_tis_core.c > > @@ -222,7 +222,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 > *buf, size_t count) > > goto out; > > } > > > > - expected = be32_to_cpu(*(__be32 *) (buf + 2)); > > + expected = get_unaligned_be32(buf + 2); > > Ditto > > > @@ -371,7 +371,7 @@ static int tpm_tis_send_main(struct tpm_chip *chip, > u8 *buf, size_t len) > > goto out_err; > > > > if (chip->flags & TPM_CHIP_FLAG_IRQ) { > > - ordinal = be32_to_cpu(*((__be32 *) (buf + 6))); > > + ordinal = get_unaligned_be32(buf + 6); > > Ditto > > Jason ------------------------------------------------------------------------------