Getting weird TPM error after rebasing my tree to security/next-general

DISCUSSION Jarkko Sakkinen 6 participants 30 messages Jan 18, 2019 – Feb 4, 2019

Subsystems (4)

x86 Security (LSM) Keys-trusted TPM device driver

lore

Share & tools

lore

Jarkko Sakkinen

author

2019-01-18 14:26 UTC #0 lore raw

I get this on a Geminilake NUC after rebasing my maintainer trees:

tpm tpm0: A TPM error (-1) occurred attempting the self test

I checked the latest commit ID from drivers/char/tpm to make sure that I did not put anything broken to my last PR [1]. It works without issues.

In addition [2] gives me an empty diff.

Something outside of the TPM driver must have happened that breaks the driver. Any ideas?

[1] commit 9488585b21bef0df1217e510c7134905d1d376a7
[2] git diff 9488585b21bef0df1217e510c7134905d1d376a7 master drivers/char/tpm/

/Jarkko

James Bottomley

maintainer

2019-01-18 22:09 UTC #1 lore raw

Quoted 8 lines · Jarkko Sakkinen

I get this on a Geminilake NUC after rebasing my maintainer trees:
tpm tpm0: A TPM error (-1) occurred attempting the self test
I checked the latest commit ID from drivers/char/tpm to make sure that I did not put anything broken to my last PR [1]. It works without issues.

Quoted 8 lines

In addition [2] gives me an empty diff.
Something outside of the TPM driver must have happened that breaks the driver. Any ideas?
[1] commit 9488585b21bef0df1217e510c7134905d1d376a7
[2] git diff 9488585b21bef0df1217e510c7134905d1d376a7 master drivers/char/tpm/

I'm afraid you're going to have to bisect to find the offending in- kernel commit, which is going to be painful since it seems to depend on physical hardware. My first instinct is that we're getting a zero length read somewhere, but I still can't see anything in the merge window that would cause that behaviour.

James

Jarkko Sakkinen

author

2019-01-20 16:04 UTC #2 lore raw

Quoted 18 lines · James Bottomley

Quoted 8 lines · Jarkko Sakkinen
I get this on a Geminilake NUC after rebasing my maintainer trees:
tpm tpm0: A TPM error (-1) occurred attempting the self test
I checked the latest commit ID from drivers/char/tpm to make sure that I did not put anything broken to my last PR [1]. It works without issues.
Quoted 8 lines
In addition [2] gives me an empty diff.
Something outside of the TPM driver must have happened that breaks the driver. Any ideas?
[1] commit 9488585b21bef0df1217e510c7134905d1d376a7
[2] git diff 9488585b21bef0df1217e510c7134905d1d376a7 master drivers/char/tpm/

Quoted 5 lines

I'm afraid you're going to have to bisect to find the offending in- kernel commit, which is going to be painful since it seems to depend on physical hardware. My first instinct is that we're getting a zero length read somewhere, but I still can't see anything in the merge window that would cause that behaviour.

Yeah, I've started to bisect it (still 9 rounds to go).

/Jarkko

Jarkko Sakkinen

author

2019-01-22 01:02 UTC #3 lore raw

Quoted 19 lines · Jarkko Sakkinen

Quoted 18 lines · James Bottomley
Quoted 16 lines · Jarkko Sakkinen
I get this on a Geminilake NUC after rebasing my maintainer trees:
tpm tpm0: A TPM error (-1) occurred attempting the self test
I checked the latest commit ID from drivers/char/tpm to make sure that I did not put anything broken to my last PR [1]. It works without issues.
In addition [2] gives me an empty diff.
Something outside of the TPM driver must have happened that breaks the driver. Any ideas?
[1] commit 9488585b21bef0df1217e510c7134905d1d376a7
[2] git diff 9488585b21bef0df1217e510c7134905d1d376a7 master drivers/char/tpm/

Quoted 7 lines

Quoted 5 lines
I'm afraid you're going to have to bisect to find the offending in- kernel commit, which is going to be painful since it seems to depend on physical hardware. My first instinct is that we're getting a zero length read somewhere, but I still can't see anything in the merge window that would cause that behaviour.
Yeah, I've started to bisect it (still 9 rounds to go).

Fails on commit 170d13ca3a2fdaaa0283399247631b76b441cca2. Still works on preceding commit a959dc88f9c8900296ccf13e2f3e1cbc555a8917.

Double-checked this after bisecting by compiling the kernel with these commits as tip.

/Jarkko

Jason Gunthorpe

reviewer

2019-01-22 02:58 UTC #4 lore raw

Quoted 26 lines · Jarkko Sakkinen

Quoted 25 lines · Jarkko Sakkinen
Quoted 23 lines · James Bottomley
Quoted 8 lines · Jarkko Sakkinen
I get this on a Geminilake NUC after rebasing my maintainer trees:
tpm tpm0: A TPM error (-1) occurred attempting the self test
I checked the latest commit ID from drivers/char/tpm to make sure that I did not put anything broken to my last PR [1]. It works without issues.
Quoted 8 lines
In addition [2] gives me an empty diff.
Something outside of the TPM driver must have happened that breaks the driver. Any ideas?
[1] commit 9488585b21bef0df1217e510c7134905d1d376a7
[2] git diff 9488585b21bef0df1217e510c7134905d1d376a7 master drivers/char/tpm/
I'm afraid you're going to have to bisect to find the offending in- kernel commit, which is going to be painful since it seems to depend on physical hardware. My first instinct is that we're getting a zero length read somewhere, but I still can't see anything in the merge window that would cause that behaviour.

Quoted 4 lines

Quoted 1 line
Yeah, I've started to bisect it (still 9 rounds to go).
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

Jason

Jarkko Sakkinen

author

2019-01-22 13:29 UTC #5 lore raw

Quoted 32 lines · Jason Gunthorpe

Quoted 26 lines · Jarkko Sakkinen
Quoted 25 lines · Jarkko Sakkinen
Quoted 23 lines · James Bottomley
Quoted 8 lines · Jarkko Sakkinen
I get this on a Geminilake NUC after rebasing my maintainer trees:
tpm tpm0: A TPM error (-1) occurred attempting the self test
I checked the latest commit ID from drivers/char/tpm to make sure that I did not put anything broken to my last PR [1]. It works without issues.
Quoted 8 lines
In addition [2] gives me an empty diff.
Something outside of the TPM driver must have happened that breaks the driver. Any ideas?
[1] commit 9488585b21bef0df1217e510c7134905d1d376a7
[2] git diff 9488585b21bef0df1217e510c7134905d1d376a7 master drivers/char/tpm/
I'm afraid you're going to have to bisect to find the offending in- kernel commit, which is going to be painful since it seems to depend on physical hardware. My first instinct is that we're getting a zero length read somewhere, but I still can't see anything in the merge window that would cause that behaviour.
Quoted 4 lines
Quoted 1 line
Yeah, I've started to bisect it (still 9 rounds to go).
Fails on commit 170d13ca3a2fdaaa0283399247631b76b441cca2. Still works on preceding commit a959dc88f9c8900296ccf13e2f3e1cbc555a8917.

Quoted 6 lines

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:

struct tpm_output_header {
	__be16	tag;
	__be32	length;
	__be32	return_code;
} __packed;

/Jarkko

Linus Torvalds

2019-01-22 18:27 UTC #6 lore raw

Quoted 12 lines · Jarkko Sakkinen

Quoted 11 lines
Quoted 4 lines
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

Quoted 6 lines

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")?

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

Jarkko Sakkinen

author

2019-01-23 15:36 UTC #7 lore raw

Quoted 34 lines · Linus Torvalds

Quoted 12 lines · Jarkko Sakkinen
Quoted 11 lines
Quoted 4 lines
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
Quoted 6 lines
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.

Quoted 2 lines

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.

Quoted 5 lines

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

Linus Torvalds

2019-01-23 18:43 UTC #8 lore raw

Quoted 10 lines · Jarkko Sakkinen

Quoted 3 lines
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;

Ok, strange.

So what *used* to happen is that the memcpy_fromio() would just expand as a "memcpy()", and in this case, gcc would then inline the memcpy(). In fact, gcc does it as a 4-byte access and a two-byte access from what I can tell.

Which is actually exactly the same as memcpy_fromio() should do, just using a different code sequence.

Quoted 1 line

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

This one gets turned into an out-of-line "memcpy()" in the old world order, which depending on size will do different things, but might be a "rep movsb". Or it might be the software expansion that does overlapping accesses and/or backwards copies.

In the new world order, it's the "memcpy_fromio()" that willdo first 4-byte accesses for the main bulk of the copy, and then end up with a two-byte and single-byte move to pad out the end.

Quoted 3 lines

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.

The first one seems to do the same thing now as it used to do, so I don't *think* it should have mattered.

The second one looks like it is unaligned (offset 6) and doing the 4-byte io reads would fail if that device needs aligned accesses. The old memcpy() *might* have done it with a "rep movsb" that would just work (?).

Linus

Jarkko Sakkinen

author

2019-01-29 13:20 UTC #9 lore raw

Quoted 13 lines · Linus Torvalds

Quoted 10 lines · Jarkko Sakkinen
Quoted 3 lines
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;

Quoted 6 lines

Ok, strange.
So what *used* to happen is that the memcpy_fromio() would just expand as a "memcpy()", and in this case, gcc would then inline the memcpy(). In fact, gcc does it as a 4-byte access and a two-byte access from what I can tell.

I verified, and it is exactly as you stated:

0xffffffff814aaa33 <+51>: mov (%rax),%edx
0xffffffff814aaa35 <+53>: mov %edx,0x0(%rbp)
0xffffffff814aaa38 <+56>: movzwl 0x4(%rax),%eax
0xffffffff814aaa3c <+60>: mov %ax,0x4(%rbp)

And your new version does exactly the same thing to the first six bytes (with different opcode, but the same memory access pattern).

/Jarkko

Jarkko Sakkinen

author

2019-01-31 12:26 UTC #10 lore raw

Quoted 23 lines · Jarkko Sakkinen

Quoted 13 lines · Linus Torvalds
Quoted 10 lines · Jarkko Sakkinen
Quoted 3 lines
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;
Quoted 6 lines
Ok, strange.
So what *used* to happen is that the memcpy_fromio() would just expand as a "memcpy()", and in this case, gcc would then inline the memcpy(). In fact, gcc does it as a 4-byte access and a two-byte access from what I can tell.
I verified, and it is exactly as you stated:

Quoted 7 lines

   0xffffffff814aaa33 <+51>:	mov    (%rax),%edx
   0xffffffff814aaa35 <+53>:	mov    %edx,0x0(%rbp)
   0xffffffff814aaa38 <+56>:	movzwl 0x4(%rax),%eax
   0xffffffff814aaa3c <+60>:	mov    %ax,0x4(%rbp)
And your new version does exactly the same thing to the first six bytes (with different opcode, but the same memory access pattern).

I think I have found the root cause:

memcpy_fromio(&__rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);

This is from crb_map_io(). This should be read as quad word.

I'll change it to ioread64() and see what happens. I don't know why it even has used memcpy_fromio() in the first place. I guess, when I first implemented the driver, I used that for no logical reason, and it has worked since up until now.

/Jarkko

Jarkko Sakkinen

author

2019-01-31 16:04 UTC #11 lore raw

Quoted 34 lines · Jarkko Sakkinen

Quoted 23 lines · Jarkko Sakkinen
Quoted 13 lines · Linus Torvalds
Quoted 10 lines · Jarkko Sakkinen
Quoted 3 lines
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;
Quoted 6 lines
Ok, strange.
So what *used* to happen is that the memcpy_fromio() would just expand as a "memcpy()", and in this case, gcc would then inline the memcpy(). In fact, gcc does it as a 4-byte access and a two-byte access from what I can tell.
I verified, and it is exactly as you stated:
Quoted 7 lines
   0xffffffff814aaa33 <+51>:	mov    (%rax),%edx
   0xffffffff814aaa35 <+53>:	mov    %edx,0x0(%rbp)
   0xffffffff814aaa38 <+56>:	movzwl 0x4(%rax),%eax
   0xffffffff814aaa3c <+60>:	mov    %ax,0x4(%rbp)
And your new version does exactly the same thing to the first six bytes (with different opcode, but the same memory access pattern).
I think I have found the root cause:

Quoted 8 lines

memcpy_fromio(&__rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
This is from crb_map_io(). This should be read as quad word.
I'll change it to ioread64() and see what happens. I don't know why it even has used memcpy_fromio() in the first place. I guess, when I first implemented the driver, I used that for no logical reason, and it has worked since up until now.

No, cannot be it. If you couldn't read it in two dwords, then it would have been always broken with 32-bit build.

Anyway, just in case, I will check what address it prints out.

/Jarkko

Jarkko Sakkinen

author

2019-01-31 17:06 UTC #12 lore raw

Quoted 44 lines · Jarkko Sakkinen

Quoted 34 lines · Jarkko Sakkinen
Quoted 23 lines · Jarkko Sakkinen
Quoted 13 lines · Linus Torvalds
Quoted 10 lines · Jarkko Sakkinen
Quoted 3 lines
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;
Quoted 6 lines
Ok, strange.
So what *used* to happen is that the memcpy_fromio() would just expand as a "memcpy()", and in this case, gcc would then inline the memcpy(). In fact, gcc does it as a 4-byte access and a two-byte access from what I can tell.
I verified, and it is exactly as you stated:
Quoted 7 lines
   0xffffffff814aaa33 <+51>:	mov    (%rax),%edx
   0xffffffff814aaa35 <+53>:	mov    %edx,0x0(%rbp)
   0xffffffff814aaa38 <+56>:	movzwl 0x4(%rax),%eax
   0xffffffff814aaa3c <+60>:	mov    %ax,0x4(%rbp)
And your new version does exactly the same thing to the first six bytes (with different opcode, but the same memory access pattern).
I think I have found the root cause:
Quoted 8 lines
memcpy_fromio(&__rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
This is from crb_map_io(). This should be read as quad word.
I'll change it to ioread64() and see what happens. I don't know why it even has used memcpy_fromio() in the first place. I guess, when I first implemented the driver, I used that for no logical reason, and it has worked since up until now.

Quoted 4 lines

No, cannot be it. If you couldn't read it in two dwords, then it would have been always broken with 32-bit build.
Anyway, just in case, I will check what address it prints out.

Found something that *does* fix the issue. If I replace memcpy_*io() calls with regular memcpy(), the driver works and all my tests pass.

/Jarkko

Linus Torvalds

2019-01-31 17:44 UTC #13 lore raw

Quoted 3 lines · Jarkko Sakkinen

Found something that *does* fix the issue. If I replace memcpy_*io() calls with regular memcpy(), the driver works and all my tests pass.

That's not surprising, since that's what we used to do. And it's horribly wrong because "memcpy()" can do things that are horribly wrong on IO accesses. Like doing them twice, but alternatively also "copy one byte at a time" which generally works, but is horrendously slow for IO.

Can you check *which* memcpy_*io() triggers the issue? Maybe by "bisecting" them (first perhaps on a file-by-file basis, and then within a file).

Linus

Jarkko Sakkinen

author

2019-01-31 18:47 UTC #14 lore raw

Quoted 11 lines · Linus Torvalds

Quoted 3 lines · Jarkko Sakkinen
Found something that *does* fix the issue. If I replace memcpy_*io() calls with regular memcpy(), the driver works and all my tests pass.
That's not surprising, since that's what we used to do. And it's horribly wrong because "memcpy()" can do things that are horribly wrong on IO accesses. Like doing them twice, but alternatively also "copy one byte at a time" which generally works, but is horrendously slow for IO.

Yup, was just a sanity check.

Quoted 3 lines

Can you check *which* memcpy_*io() triggers the issue? Maybe by "bisecting" them (first perhaps on a file-by-file basis, and then within a file).

Already did that. See my follow-up.

/Jarkko

Jarkko Sakkinen

author

2019-01-31 18:35 UTC #15 lore raw

In reply to Jarkko Sakkinen

Quoted 2 lines · Jarkko Sakkinen

Found something that *does* fix the issue. If I replace memcpy_*io() calls with regular memcpy(), the driver works and all my tests pass.

OK, so the length of the response is not trashed, but only the error code. The attached patch fully fixes the issue.

Here's the header again:

struct tpm_output_header {
	__be16	tag;
	__be32	length;
	__be32	return_code;
} __packed;

The first to fields *are* read correctly and the last field get 1's (thus TPM error -1).

With the attached the patch things work properly, but still unsatisfactory fix (return to old behavior because it seems to work).

/Jarkko

From 8ff7eb66e1dc05daf1319bf8365e5a3434a90061 Mon Sep 17 00:00:00 2001
From: Jarkko Sakkinen <[email protected]>
Date: Thu, 31 Jan 2019 20:16:00 +0200
Subject: [PATCH] tpm/tpm_crb: Revert to memcpy() for copying tail of the
response

Revert the behavior before 170d13ca3a2f.

Signed-off-by Jarkko Sakkinen <[email protected]>

---
 drivers/char/tpm/tpm_crb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/char/tpm/tpm_crb.c +1 -1

index 36952ef98f90..003923ea50d2 100644
@@ -299,7 +299,7 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 	if (expected > count || expected < 6)
 		return -EIO;
 
-	memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);
+	memcpy(&buf[6], &priv->rsp[6], expected - 6);
 
 	return expected;
 }

Signature · 1 line

2.19.1

Linus Torvalds

2019-01-31 18:51 UTC #16 lore raw

Quoted 6 lines · Jarkko Sakkinen

OK, so the length of the response is not trashed, but only the error code. The attached patch fully fixes the issue.
Here's the header again:

Quoted 8 lines

struct tpm_output_header {
        __be16  tag;
        __be32  length;
        __be32  return_code;
} __packed;
The first to fields *are* read correctly and the last field get 1's (thus TPM error -1).

Ok, so this makes sense, even though that patch is (I think) completely wrong.

What happens is that the 32-bit fields are mis-aligned: the "tag" is obviously properly 16-bit aligned, but then both "length" and "return_code" are 32-bit fields that are only aligned on a 16-bit alignment.

What happens is that first you copy the two first fields:

        memcpy_fromio(buf, priv->rsp, 6);

which copies "tag" and "length", but it copies them by reading then as a 4-byte and then 2-byte value (in that order). So it actually reads 'tag' and 'first two bytes of 'length', and then the second access reads the last two bytes of 'length'

And it all works, because the accesses are aligned by address of access, even though they are *not* aligned in the 'struct tpm_output_header' fields.

But then later on, when you read 'return_code', and do

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

you now do a 4-byte memcpy at offset 6. So it does a 4-byte access, bit it's not 4-byte aligned.

Honestly, memcpy() itself shouldn't have worked *either*, but you lucked out. Gcc doesn't know that it's a 4-byte access, so it actually calls out to the memcpy() routine. And that one happened to be "rep movsb" on your machine. And that happened to work.

But it's really not supposed to work, and it really *wouldn't* have worked if somebody disabled the rep-string functions.

In fact, we have another patch (that isn't applied) that makes even the memcpy_erms() just call the sw version of memcpy() for short copies (because "rep movsb" is slow for those cases). That would also have broken your driver.

Linus

Linus Torvalds

2019-01-31 18:52 UTC #17 lore raw

Quoted 5 lines · Linus Torvalds

In fact, we have another patch (that isn't applied) that makes even the memcpy_erms() just call the sw version of memcpy() for short copies (because "rep movsb" is slow for those cases). That would also have broken your driver.

I think what I should do is to just make "memcpy_*io()" do the "align naturally" thing.

Let me cook up a patch for you to test.

Linus

Linus Torvalds

2019-01-31 19:10 UTC #18 lore raw

Quoted 5 lines · Linus Torvalds

I think what I should do is to just make "memcpy_*io()" do the "align naturally" thing.
Let me cook up a patch for you to test.

Does this work for you?

I haven't tested it at all, but I verified that the generated code seems to make at least some amount of sense.

Linus

 arch/x86/lib/iomem.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

arch/x86/lib/iomem.c +24 -3

index 66894675f3c8..7b5b07f3d370 100644
@@ -3,7 +3,7 @@
 #include <linux/io.h>
 
 /* Originally from i386/string.h */
-static __always_inline void __iomem_memcpy(void *to, const void *from, size_t n)
+static __always_inline void rep_movs(void *to, const void *from, size_t n)
 {
 	unsigned long d0, d1, d2;
 	asm volatile("rep ; movsl\n\t"

@@ -19,15 +19,36 @@ static __always_inline void __iomem_memcpy(void *to, const void *from, size_t n)
 		     : "memory");
 }
 
+#define movs(type,to,from) \
+	asm volatile("movs" type:"=&D" (to), "=&S" (from):"0" (to), "1" (from):"memory")
+
 void memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
 {
-	__iomem_memcpy(to, (const void *)from, n);
+	/* Unaligned IO? */
+	if (n && unlikely(1 & (unsigned long)from)) {
+		movs("b", to, from);
+		n--;
+	}
+	if (n > 1 && unlikely(2 & (unsigned long)from)) {
+		movs("w", to, from);
+		n-=2;
+	}
+	rep_movs(to, (const void *)from, n);
 }
 EXPORT_SYMBOL(memcpy_fromio);
 
 void memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
 {
-	__iomem_memcpy((void *)to, (const void *) from, n);
+	/* Unaligned IO? */
+	if (n && unlikely(1 & (unsigned long)to)) {
+		movs("b", to, from);
+		n--;
+	}
+	if (n > 1 && unlikely(2 & (unsigned long)to)) {
+		movs("w", to, from);
+		n-=2;
+	}
+	rep_movs((void *)to, (const void *) from, n);
 }
 EXPORT_SYMBOL(memcpy_toio);

Winkler, Tomas

2019-01-31 19:47 UTC #19 lore raw

Quoted 8 lines

Quoted 4 lines · Linus Torvalds
I think what I should do is to just make "memcpy_*io()" do the "align naturally" thing.

Quoted 8 lines

Quoted 1 line
Let me cook up a patch for you to test.
Does this work for you?
I haven't tested it at all, but I verified that the generated code seems to make at least some amount of sense.
Linus

So dig into the spec and I think this is a bit relevant.

TPM TCG according the spec requires that all buffer access is done sequentially from the start to end of the payload, Simply In case of skipping or going back the transaction is aborted.
The write transactions should be 1 or power of 2. So in general 6 byte read should not work. But I'm sure our hw really obey this restriction.

Thanks
Tomas

Jarkko Sakkinen

author

2019-02-01 08:12 UTC #20 lore raw

Quoted 17 lines · Tomas

Quoted 8 lines
Quoted 4 lines · Linus Torvalds
I think what I should do is to just make "memcpy_*io()" do the "align naturally" thing.
Quoted 8 lines
Quoted 1 line
Let me cook up a patch for you to test.
Does this work for you?
I haven't tested it at all, but I verified that the generated code seems to make at least some amount of sense.
Linus

Quoted 7 lines

So dig into the spec and I think this is a bit relevant.
TPM TCG according the spec requires that all buffer access is done sequentially from the start to end of the payload, Simply In case of skipping or going back the transaction is aborted. The write transactions should be 1 or power of 2. So in general 6 byte read should not work. But I'm sure our hw really obey this restriction.

We could easily read the first 8 bytes instead of 6, and then check the length. The benefit in doing that would be to be able to backport the patch to stable kernels.

I'm fine with either solution.

Applying Linus' fix might anyway a good idea since given that tpm_crb broke down, there is a finite probability that there might be some other drivers that break down for similar reasons, but no one has noticed so far.

Thus, I might even propose doing both...

/Jarkko

Winkler, Tomas

2019-01-31 20:07 UTC #21 lore raw

In reply to Linus Torvalds

Quoted 31 lines

-----Original Message-----
From: Winkler, Tomas
Sent: Thursday, January 31, 2019 21:47
To: 'Linus Torvalds' <[email protected]>; Jarkko Sakkinen <[email protected]>
Cc: Jason Gunthorpe <[email protected]>; James Bottomley <[email protected]>; [email protected]; [email protected]; Linux List Kernel Mailing <linux- [email protected]>
Subject: RE: Getting weird TPM error after rebasing my tree to security/next- general
Quoted 7 lines
Quoted 4 lines · Linus Torvalds
I think what I should do is to just make "memcpy_*io()" do the "align naturally" thing.
Quoted 8 lines
Quoted 1 line
Let me cook up a patch for you to test.
Does this work for you?
I haven't tested it at all, but I verified that the generated code seems to make at least some amount of sense.
Linus

Quoted 7 lines

So dig into the spec and I think this is a bit relevant.
TPM TCG according the spec requires that all buffer access is done sequentially from the start to end of the payload, Simply In case of skipping or going back the transaction is aborted.
The write transactions should be 1 or power of 2. So in general 6 byte read should not work. But I'm sure our hw really obey this restriction.

For alignment of register access the spec specifies:

Some instantiating hardware may have size and alignment restrictions when accessing any of the fields in this interface. Some hardware may require access to any of data within a field or buffer be performed by reads or writes which are naturally aligned. For this reason, software should access the fields and buffers defined in this interface using instructions which do not cause an access to cross an alignment boundary and should not use string move instructions

This is for register address, I'm not sure this spans also for request response buffer, but from the behavior looks like it applies there two.

Thanks
Tomas

Jarkko Sakkinen

author

2019-01-31 20:47 UTC #22 lore raw

In reply to Linus Torvalds

Quoted 6 lines · Linus Torvalds

Quoted 4 lines · Linus Torvalds
I think what I should do is to just make "memcpy_*io()" do the "align naturally" thing.

Quoted 6 lines

Quoted 1 line
Let me cook up a patch for you to test.
Does this work for you?
I haven't tested it at all, but I verified that the generated code seems to make at least some amount of sense.

I'll try it first thing when I wake up tomorrow (11PM in Finland ATM). Appreciate for taking time on this. Thank you.

/Jarkko

Linus Torvalds

2019-01-31 21:58 UTC #23 lore raw

Quoted 2 lines · Jarkko Sakkinen

I'll try it first thing when I wake up tomorrow (11PM in Finland ATM).

Thanks.

Quoted 1 line

Appreciate for taking time on this.

Hey, it was my commit that broke it for you. Even if it happened to work before, and only did so by pure luck, it was a functional regression.

I get very upset when other developers don't step up when *their* changes break something, and I don't consider "it shouldn't have worked in the first place" to be a valid excuse. You broke it, you'd better fix it.

So I had better fix my own mess too, in order to not look too hypocritical.

And I was very aware that hardcoding the memcpy_*io() access patterns might break something. I just _hoped_ it wouldn't, because we actually ended up going back to the very original access patterns (but it was from a long long time ago).

In fact, while it's slightly annoying, in many ways it's actually good that we found breakage, and could pinpoint exactly *why* it broke. That does validate the whole "we shouldn't just depend on the random implementation detail of 'memcpy()'" argument.

So I'll wait to hear back whether that patch fixes things for you, but I _think_ it will, and we'll be better off in the long range with this whole thing.

Linus

Jerry Snitselaar

2019-01-31 23:31 UTC #24 lore raw

Quoted 30 lines · Linus Torvalds

Quoted 2 lines · Jarkko Sakkinen
I'll try it first thing when I wake up tomorrow (11PM in Finland ATM).
Thanks.
Quoted 1 line
Appreciate for taking time on this.
Hey, it was my commit that broke it for you. Even if it happened to work before, and only did so by pure luck, it was a functional regression.
I get very upset when other developers don't step up when *their* changes break something, and I don't consider "it shouldn't have worked in the first place" to be a valid excuse. You broke it, you'd better fix it.
So I had better fix my own mess too, in order to not look too hypocritical.
And I was very aware that hardcoding the memcpy_*io() access patterns might break something. I just _hoped_ it wouldn't, because we actually ended up going back to the very original access patterns (but it was from a long long time ago).
In fact, while it's slightly annoying, in many ways it's actually good that we found breakage, and could pinpoint exactly *why* it broke. That does validate the whole "we shouldn't just depend on the random implementation detail of 'memcpy()'" argument.

Quoted 5 lines

So I'll wait to hear back whether that patch fixes things for you, but I _think_ it will, and we'll be better off in the long range with this whole thing.
Linus

I just did a quick test here of booting and running a couple commands (tpm2_getcap, tpm2_pcrlist), and the patch seems to work for me. I was seeing the error during tpm_crb initialization without the patch.

Jarkko Sakkinen

author Reviewed-by Tested-by

2019-02-01 11:40 UTC #25 lore raw

Quoted 35 lines · Jerry Snitselaar

Quoted 34 lines · Linus Torvalds
Quoted 2 lines · Jarkko Sakkinen
I'll try it first thing when I wake up tomorrow (11PM in Finland ATM).
Thanks.
Quoted 1 line
Appreciate for taking time on this.
Hey, it was my commit that broke it for you. Even if it happened to work before, and only did so by pure luck, it was a functional regression.
I get very upset when other developers don't step up when *their* changes break something, and I don't consider "it shouldn't have worked in the first place" to be a valid excuse. You broke it, you'd better fix it.
So I had better fix my own mess too, in order to not look too hypocritical.
And I was very aware that hardcoding the memcpy_*io() access patterns might break something. I just _hoped_ it wouldn't, because we actually ended up going back to the very original access patterns (but it was from a long long time ago).
In fact, while it's slightly annoying, in many ways it's actually good that we found breakage, and could pinpoint exactly *why* it broke. That does validate the whole "we shouldn't just depend on the random implementation detail of 'memcpy()'" argument.
So I'll wait to hear back whether that patch fixes things for you, but I _think_ it will, and we'll be better off in the long range with this whole thing.

Quoted 5 lines

Quoted 1 line
Linus
I just did a quick test here of booting and running a couple commands (tpm2_getcap, tpm2_pcrlist), and the patch seems to work for me. I was seeing the error during tpm_crb initialization without the patch.

Works for me too. Just submitted tpm_crb level fix too for the reasons explained in accompanied commit message.

Reviewed-by Jarkko Sakkinen <[email protected]>

Tested-by Jarkko Sakkinen <[email protected]>

/Jarkko

Jarkko Sakkinen

author

2019-01-31 20:45 UTC #26 lore raw

In reply to Linus Torvalds

Quoted 14 lines · Linus Torvalds

Quoted 12 lines · Jarkko Sakkinen
OK, so the length of the response is not trashed, but only the error code. The attached patch fully fixes the issue.
Here's the header again:
struct tpm_output_header {
        __be16  tag;
        __be32  length;
        __be32  return_code;
} __packed;

Quoted 4 lines

Quoted 2 lines
The first to fields *are* read correctly and the last field get 1's (thus TPM error -1).
Ok, so this makes sense, even though that patch is (I think) completely wrong.

I don't disagree on that :-) Just pinpointed the location where it fails.

Quoted 9 lines

What happens is that the 32-bit fields are mis-aligned: the "tag" is obviously properly 16-bit aligned, but then both "length" and "return_code" are 32-bit fields that are only aligned on a 16-bit alignment.
What happens is that first you copy the two first fields:
        memcpy_fromio(buf, priv->rsp, 6);

Quoted 8 lines

which copies "tag" and "length", but it copies them by reading then as a 4-byte and then 2-byte value (in that order). So it actually reads 'tag' and 'first two bytes of 'length', and then the second access reads the last two bytes of 'length'
And it all works, because the accesses are aligned by address of access, even though they are *not* aligned in the 'struct tpm_output_header' fields.

Right, they are still naturally aligned accesses.

Quoted 11 lines

But then later on, when you read 'return_code', and do
        memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);
you now do a 4-byte memcpy at offset 6. So it does a 4-byte access, bit it's not 4-byte aligned.
Honestly, memcpy() itself shouldn't have worked *either*, but you lucked out. Gcc doesn't know that it's a 4-byte access, so it actually calls out to the memcpy() routine. And that one happened to be "rep movsb" on your machine. And that happened to work.

I understand what you mean. Just surprised that this hasn't failed before to anyone (the same driver has been even successfully used on ARM64 with TrustZone based fTPM implementation). It has been in for three years now.

Quoted 9 lines

But it's really not supposed to work, and it really *wouldn't* have worked if somebody disabled the rep-string functions.
In fact, we have another patch (that isn't applied) that makes even the memcpy_erms() just call the sw version of memcpy() for short copies (because "rep movsb" is slow for those cases). That would also have broken your driver.
Linus

/Jarkko

Linus Torvalds

2019-02-01 18:04 UTC #27 lore raw

Quoted 5 lines · Jarkko Sakkinen

I understand what you mean. Just surprised that this hasn't failed before to anyone (the same driver has been even successfully used on ARM64 with TrustZone based fTPM implementation). It has been in for three years now.

Just to finish this thread off: it turns out that both ARM and ARM64 worked fine, because neither did a memcpy(), but had explicit IO copy routines.

And in those explicit routines, 32-bit ARM did only byte accesses, and 64-bit ARM did 8-byte accesses for the bulk transfer part, but byte accesses for the unaligned head and tail of the IO area.

So I think it's all good. x86 used to work by luck (either because all machines that used that TPM chip always had ERMS, or because the people who didn't have it never cared), and ARM just worked because it would never do unaligned IO accesses anyway (well, I guess you can force them with "readl()" on an unaligned address, but then you just have yourself to blame).

Linus

Jarkko Sakkinen

author

2019-02-04 11:58 UTC #28 lore raw

Quoted 16 lines · Linus Torvalds

Quoted 5 lines · Jarkko Sakkinen
I understand what you mean. Just surprised that this hasn't failed before to anyone (the same driver has been even successfully used on ARM64 with TrustZone based fTPM implementation). It has been in for three years now.
Just to finish this thread off: it turns out that both ARM and ARM64 worked fine, because neither did a memcpy(), but had explicit IO copy routines.
And in those explicit routines, 32-bit ARM did only byte accesses, and 64-bit ARM did 8-byte accesses for the bulk transfer part, but byte accesses for the unaligned head and tail of the IO area.

Quoted 8 lines

So I think it's all good. x86 used to work by luck (either because all machines that used that TPM chip always had ERMS, or because the people who didn't have it never cared), and ARM just worked because it would never do unaligned IO accesses anyway (well, I guess you can force them with "readl()" on an unaligned address, but then you just have yourself to blame).
Linus

OK, thanks for the summary. This kind of answered to my question. Should be sufficient to include the tpm_crb fix to the 5.1 PR.

/Jarkko

Jarkko Sakkinen

author

2019-01-23 20:11 UTC #29 lore raw

In reply to Jarkko Sakkinen

Quoted 43 lines · Jarkko Sakkinen

Quoted 34 lines · Linus Torvalds
Quoted 12 lines · Jarkko Sakkinen
Quoted 11 lines
Quoted 4 lines
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
Quoted 6 lines
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.
Quoted 2 lines
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)

Quoted 7 lines

	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.

And I wonder why 32-bit has worked before.

Tomas, you've been more involved with ME and fTPM runs there. Do you have any clues where this could be rooted?

/Jarkko