linux-kernel - Re: Getting weird TPM error after rebasing my tree to security/next-general

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]

Message-ID: <20190123153638.GA8727@linux.intel.com>
Date:   Wed, 23 Jan 2019 17:36:38 +0200
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Jason Gunthorpe <jgg@...pe.ca>,
        James Bottomley <James.Bottomley@...senpartnership.com>,
        linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>
Subject: Re: Getting weird TPM error after rebasing my tree to
 security/next-general

On Wed, Jan 23, 2019 at 07:26:42AM +1300, Linus Torvalds wrote:
> On Wed, Jan 23, 2019 at 2:29 AM Jarkko Sakkinen
> <jarkko.sakkinen@...ux.intel.com> wrote:
> >
> > > >
> > > > Fails on commit 170d13ca3a2fdaaa0283399247631b76b441cca2. Still works on
> > > > preceding commit a959dc88f9c8900296ccf13e2f3e1cbc555a8917.
> > >
> > > This changes the IO access pattern in memcpy_to/fromio.. Presumably
> > > CRB HW doesn't like the new 4 byte move? Swap each one in crb to
> > > memcpy to confirm..
> > >
> > > If the HW requires particular access patterns you can't use
> > > memcpy_to/fromio
> >
> > Did not have time to look at the commit at all but your deduction
> > is correct. I know it without testing.
> >
> > Memory controller will feed 1's on unaligned read from IO memory,
> > and as we can see from the TPM header, this change causes two of
> > those:
> 
> Funky. But how did it work before then?
> 
> The new memcpy_fromio() is designed to have _predictable_ access
> patterns. Not necessarily the best, but at least consistent.
> 
> Prevously, we used whatever random "memcpy()" implementation we
> happened to pick, which *could* be aligned (particularly "rep movsb" -
> absolutely horrible performance for MMIO, but by doing IO one byte at
> a time it was certainly aligned ;), but most of our x86 memcpy
> implementations don't actually try all that hard to align the source.
> And the manual version will actually copy things *backwards* for some
> cases.
> 
> Is it just that this particular hardware always happened to trigger
> the ERMS case (ie "rep movsb")?

This is the particular snippet in question:

memcpy_fromio(buf, priv->rsp, 6);
expected = be32_to_cpup((__be32 *) &buf[2]);
if (expected > count || expected < 6)
	return -EIO;

memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);

I guess it did in the first memcpy_fromio operation since it is less
than a quad word, right? Not sure why the 2nd memcpy_fromio() operation
has worked, though.

> Anyway, Jason is correct that if a device has particular IO pattern
> requirements, you shouldn't use "memcpy_fromio()" and friends, but
> it's interesting how it apparently *happened* to work before.
> 
>              Linus

Sure, I'll prepare a fix ASAP.

/Jarkko