linux-kernel - RE: [PATCH] tpm: use get_unaligned

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 23 Nov 2016 20:56:25 +0000
From:   "Winkler, Tomas" <tomas.winkler@...el.com>
To:     Jason Gunthorpe <jgunthorpe@...idianresearch.com>
CC:     "tpmdd-devel@...ts.sourceforge.net" 
        <tpmdd-devel@...ts.sourceforge.net>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] tpm: use get_unaligned_be32 unaligned buffer access.

> 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