Refactor TPM event log code

PATCH Superseded Thiebaud Weksteen 6 participants 21 messages Apr 12, 2018 – May 11, 2018

Subsystems (3)

Keys-trusted TPM device driver Sparse checker

lore

Share & tools

lore

Thiebaud Weksteen

author

2018-04-12 10:13 UTC #0 lore raw

v2 cover Refactor TPM event log code

This patchset implements the proposal from Jarkko Sakkinen [1]. I have included the feedback from Nayna Jain about the function naming.

[1] https://lkml.kernel.org/r/[email protected]

Changes since v1:
- Add Suggested-by
- Fix kbuild report

Thiebaud Weksteen (4):
tpm: Add explicit endianness cast
tpm: Move eventlog files to a subdirectory
tpm: Move shared eventlog functions to common.c
tpm: Move eventlog declarations to its own header

 drivers/char/tpm/Makefile                     |  10 +-
 .../{tpm_eventlog_acpi.c => eventlog/acpi.c}  |   3 +-
 drivers/char/tpm/eventlog/common.c            | 195 +++++++++++++++++
 drivers/char/tpm/eventlog/common.h            |  35 +++
 .../{tpm_eventlog_efi.c => eventlog/efi.c}    |   3 +-
 .../tpm/{tpm_eventlog_of.c => eventlog/of.c}  |   7 +-
 .../tpm/{tpm1_eventlog.c => eventlog/tpm1.c}  | 200 ++----------------
 .../tpm/{tpm2_eventlog.c => eventlog/tpm2.c}  |   3 +-
 drivers/char/tpm/tpm.h                        |  27 ---
 9 files changed, 262 insertions(+), 221 deletions(-)
 rename drivers/char/tpm/{tpm_eventlog_acpi.c => eventlog/acpi.c} (98%)
 create mode 100644 drivers/char/tpm/eventlog/common.c
 create mode 100644 drivers/char/tpm/eventlog/common.h
 rename drivers/char/tpm/{tpm_eventlog_efi.c => eventlog/efi.c} (97%)
 rename drivers/char/tpm/{tpm_eventlog_of.c => eventlog/of.c} (94%)
 rename drivers/char/tpm/{tpm1_eventlog.c => eventlog/tpm1.c} (58%)
 rename drivers/char/tpm/{tpm2_eventlog.c => eventlog/tpm2.c} (99%)

Signature · 1 line

2.17.0.484.g0c8726318c-goog

Thiebaud Weksteen

author

2018-04-12 10:14 UTC #1 lore raw

PATCH v2 1/4 tpm: Add explicit endianness cast

Signed-off-by Thiebaud Weksteen <[email protected]>

---
 drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/char/tpm/tpm_eventlog_of.c +2 -2

index 96fd5646f866..d74568d58a66 100644
@@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	 * but physical tpm needs the conversion.
 	 */
 	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
-		size = be32_to_cpup(sizep);
-		base = be64_to_cpup(basep);
+		size = be32_to_cpup((__be32 *)sizep);
+		base = be64_to_cpup((__be64 *)basep);
 	} else {
 		size = *sizep;
 		base = *basep;

Signature · 1 line

2.17.0.484.g0c8726318c-goog

Jason Gunthorpe

reviewer

2018-04-17 03:02 UTC #2 lore raw

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

Quoted 8 lines · Thiebaud Weksteen

Signed-off-by Thiebaud Weksteen <[email protected]>
 drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/char/tpm/tpm_eventlog_of.c +1 -0
index 96fd5646f866..d74568d58a66 100644
+++ b/drivers/char/tpm/tpm_eventlog_of.c

drivers/char/tpm/tpm_eventlog_of.c int tpm_read_log_of(struct tpm_chip *chip) · line 60

Quoted 7 lines

drivers/char/tpm/tpm_eventlog_of.c +2 -2

@@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	 * but physical tpm needs the conversion.
 	 */
 	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
-		size = be32_to_cpup(sizep);
-		base = be64_to_cpup(basep);
+		size = be32_to_cpup((__be32 *)sizep);
+		base = be64_to_cpup((__be64 *)basep);

Er, no.. change the definitions of sizep and basep to be __be

Jason

Thiebaud Weksteen

author

2018-04-17 08:32 UTC #3 lore raw

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

Quoted 6 lines · Jason Gunthorpe

Review
Quoted 5 lines · Thiebaud Weksteen
Signed-off-by Thiebaud Weksteen <[email protected]>
 drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
(quoted diff) +0 -0

b/drivers/char/tpm/tpm_eventlog_of.c

Quoted 13 lines

drivers/char/tpm/tpm_eventlog_of.c int tpm_read_log_of(struct tpm_chip *chip) · line 60
Quoted 10 lines
index 96fd5646f866..d74568d58a66 100644
+++ b/drivers/char/tpm/tpm_eventlog_of.c
drivers/char/tpm/tpm_eventlog_of.c +2 -2
@@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
       * but physical tpm needs the conversion.
       */
      if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
-             size = be32_to_cpup(sizep);
-             base = be64_to_cpup(basep);
+             size = be32_to_cpup((__be32 *)sizep);
+             base = be64_to_cpup((__be64 *)basep);
Er, no.. change the definitions of sizep and basep to be __be

Quoted 1 line

Jason

Please read the comment before the condition. sizep and
basep may contain either little endian or big endian and this block is used to adjust that. Let me know if there is a better way for handling this.

Jason Gunthorpe

reviewer

2018-04-17 14:00 UTC #4 lore raw

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

Quoted 22 lines · Thiebaud Weksteen

Quoted 6 lines · Jason Gunthorpe
Review
Quoted 5 lines · Thiebaud Weksteen
Signed-off-by Thiebaud Weksteen <[email protected]>
 drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/char/tpm/tpm_eventlog_of.c
Quoted 12 lines
drivers/char/tpm/tpm_eventlog_of.c int tpm_read_log_of(struct tpm_chip *chip) · line 60
Quoted 10 lines
index 96fd5646f866..d74568d58a66 100644
+++ b/drivers/char/tpm/tpm_eventlog_of.c
drivers/char/tpm/tpm_eventlog_of.c +2 -2
@@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
       * but physical tpm needs the conversion.
       */
      if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
-             size = be32_to_cpup(sizep);
-             base = be64_to_cpup(basep);
+             size = be32_to_cpup((__be32 *)sizep);
+             base = be64_to_cpup((__be64 *)basep);
Er, no.. change the definitions of sizep and basep to be __be

Quoted 5 lines

Quoted 1 line
Jason
Please read the comment before the condition. sizep and
basep may contain either little endian or big endian and this block is used to adjust that. Let me know if there is a better way for handling this.

Well a cast like that will throw sparse warnings, you need __force at least

Jason

Thiebaud Weksteen

author

2018-04-19 13:09 UTC #5 lore raw

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

Quoted 8 lines · Jason Gunthorpe

Quoted 7 lines · Thiebaud Weksteen
Quoted 5 lines · Jason Gunthorpe
Quoted 4 lines · Thiebaud Weksteen
Signed-off-by Thiebaud Weksteen <[email protected]>
 drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Quoted 8 lines

Quoted 8 lines
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/char/tpm/tpm_eventlog_of.c
Quoted 6 lines
drivers/char/tpm/tpm_eventlog_of.c int tpm_read_log_of(struct tpm_chip *chip) · line 58
Quoted 6 lines
index 96fd5646f866..d74568d58a66 100644
+++ b/drivers/char/tpm/tpm_eventlog_of.c
drivers/char/tpm/tpm_eventlog_of.c +0 -0
@@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
       * but physical tpm needs the conversion.
       */
      if (of_property_match_string(np, "compatible", "IBM,vtpm") <

0) {

Quoted 11 lines

Quoted 11 lines
Quoted 8 lines
drivers/char/tpm/tpm_eventlog_of.c int tpm_read_log_of(struct tpm_chip *chip) · line 60
Quoted 4 lines
drivers/char/tpm/tpm_eventlog_of.c +2 -2
-             size = be32_to_cpup(sizep);
-             base = be64_to_cpup(basep);
+             size = be32_to_cpup((__be32 *)sizep);
+             base = be64_to_cpup((__be64 *)basep);
Er, no.. change the definitions of sizep and basep to be __be
Jason
Please read the comment before the condition. sizep and basep may contain either little endian or big endian and this block is

used

Quoted 6 lines

Quoted 1 line
to adjust that. Let me know if there is a better way for handling this.
Well a cast like that will throw sparse warnings, you need __force at least
Jason

I don't think so. Since the variable is only defined as u32*, no specific warning is generated. I've used make C=2 drivers/char/tpm/ with this patch applied and no new warning is being triggered.

Jason Gunthorpe

reviewer

2018-04-20 14:57 UTC #6 lore raw

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

Quoted 31 lines · Thiebaud Weksteen

Quoted 8 lines · Jason Gunthorpe
Quoted 7 lines · Thiebaud Weksteen
Quoted 5 lines · Jason Gunthorpe
Quoted 4 lines · Thiebaud Weksteen
Signed-off-by Thiebaud Weksteen <[email protected]>
 drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/char/tpm/tpm_eventlog_of.c
Quoted 6 lines
drivers/char/tpm/tpm_eventlog_of.c int tpm_read_log_of(struct tpm_chip *chip) · line 58
Quoted 6 lines
index 96fd5646f866..d74568d58a66 100644
+++ b/drivers/char/tpm/tpm_eventlog_of.c
drivers/char/tpm/tpm_eventlog_of.c +0 -0
@@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
       * but physical tpm needs the conversion.
       */
      if (of_property_match_string(np, "compatible", "IBM,vtpm") <
0) {
Quoted 11 lines
Quoted 11 lines
Quoted 8 lines
drivers/char/tpm/tpm_eventlog_of.c int tpm_read_log_of(struct tpm_chip *chip) · line 60
Quoted 4 lines
drivers/char/tpm/tpm_eventlog_of.c +2 -2
-             size = be32_to_cpup(sizep);
-             base = be64_to_cpup(basep);
+             size = be32_to_cpup((__be32 *)sizep);
+             base = be64_to_cpup((__be64 *)basep);
Er, no.. change the definitions of sizep and basep to be __be
Jason
Please read the comment before the condition. sizep and basep may contain either little endian or big endian and this block is
used

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
to adjust that. Let me know if there is a better way for handling this.
Well a cast like that will throw sparse warnings, you need __force at least
I don't think so. Since the variable is only defined as u32*, no specific warning is generated. I've used make C=2 drivers/char/tpm/ with this patch applied and no new warning is being triggered.

I'm surprised to hear you say that..

Sparse is supposed to require force on all cast that change the annotation, and there are many examples in the kernel that have force in that case.

Jason

Thiebaud Weksteen

author

2018-04-23 09:22 UTC #7 lore raw

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

Quoted 5 lines · Jason Gunthorpe

Quoted 4 lines · Thiebaud Weksteen
Quoted 2 lines · Jason Gunthorpe
Quoted 1 line · Thiebaud Weksteen
On Tue, Apr 17, 2018 at 5:02 AM Jason Gunthorpe <[email protected]>

Quoted 6 lines · wrote:

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines · Thiebaud Weksteen
Quoted 4 lines
Signed-off-by Thiebaud Weksteen <[email protected]>
 drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/char/tpm/tpm_eventlog_of.c
Quoted 6 lines
drivers/char/tpm/tpm_eventlog_of.c int tpm_read_log_of(struct tpm_chip *chip) · line 58
Quoted 6 lines
index 96fd5646f866..d74568d58a66 100644
+++ b/drivers/char/tpm/tpm_eventlog_of.c
drivers/char/tpm/tpm_eventlog_of.c +0 -0
@@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
       * but physical tpm needs the conversion.
       */
      if (of_property_match_string(np, "compatible",

"IBM,vtpm") <

Quoted 6 lines

Quoted 6 lines
0) {
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
drivers/char/tpm/tpm_eventlog_of.c +2 -2
-             size = be32_to_cpup(sizep);
-             base = be64_to_cpup(basep);
+             size = be32_to_cpup((__be32 *)sizep);
+             base = be64_to_cpup((__be64 *)basep);

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Er, no.. change the definitions of sizep and basep to be __be
Jason
Please read the comment before the condition. sizep and basep may contain either little endian or big endian and this block

Quoted 2 lines

Quoted 2 lines
used
Quoted 1 line
Quoted 1 line
to adjust that. Let me know if there is a better way for handling

this.

Quoted 5 lines

Quoted 5 lines
Quoted 3 lines
Well a cast like that will throw sparse warnings, you need __force at least
I don't think so. Since the variable is only defined as u32*, no

specific

Quoted 10 lines

Quoted 2 lines
warning is generated. I've used make C=2 drivers/char/tpm/ with this patch applied and no new warning is being triggered.
I'm surprised to hear you say that..
Sparse is supposed to require force on all cast that change the annotation, and there are many examples in the kernel that have force in that case.
Jason

[email protected] and [email protected] for a sanity check.

If you look at the man page of sparse, under the bitwise option, it states:
"Sparse will warn on [...] any conversion of one restricted type into another, except via a cast that includes __attribute__((force)).". In our case, it is a conversion from unrestricted to restricted which does not fall in this category.

Luc Van Oostenryck

2018-04-23 10:06 UTC #8 lore raw

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

Quoted 37 lines · Thiebaud Weksteen

Quoted 5 lines · Jason Gunthorpe
Quoted 4 lines · Thiebaud Weksteen
Quoted 2 lines · Jason Gunthorpe
Quoted 1 line · Thiebaud Weksteen
On Tue, Apr 17, 2018 at 5:02 AM Jason Gunthorpe <[email protected]>
Quoted 6 lines · wrote:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines · Thiebaud Weksteen
Quoted 4 lines
Signed-off-by Thiebaud Weksteen <[email protected]>
 drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/char/tpm/tpm_eventlog_of.c
Quoted 6 lines
Review int tpm_read_log_of(struct tpm_chip *chip) · line 58
Quoted 6 lines
index 96fd5646f866..d74568d58a66 100644
+++ b/drivers/char/tpm/tpm_eventlog_of.c
(quoted diff) +0 -0
@@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
       * but physical tpm needs the conversion.
       */
      if (of_property_match_string(np, "compatible",
"IBM,vtpm") <
Quoted 6 lines
Quoted 6 lines
0) {
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +2 -2
-             size = be32_to_cpup(sizep);
-             base = be64_to_cpup(basep);
+             size = be32_to_cpup((__be32 *)sizep);
+             base = be64_to_cpup((__be64 *)basep);
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Er, no.. change the definitions of sizep and basep to be __be
Jason
Please read the comment before the condition. sizep and basep may contain either little endian or big endian and this block
is
Quoted 1 line
Quoted 1 line
used

Quoted 10 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
to adjust that. Let me know if there is a better way for handling
this.
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Well a cast like that will throw sparse warnings, you need __force at least
I don't think so. Since the variable is only defined as u32*, no
specific
Quoted 2 lines
Quoted 2 lines
warning is generated. I've used make C=2 drivers/char/tpm/ with this patch applied and no new warning is being triggered.

Interesting.

Quoted 1 line

Quoted 1 line
I'm surprised to hear you say that..

Same for me.

Quoted 3 lines

Quoted 3 lines
Sparse is supposed to require force on all cast that change the annotation, and there are many examples in the kernel that have force in that case.

Yes, sparse is supposed to warn in such cases and the __force is there to quiets the warning when it is known that the cast is in fact correct.

Quoted 7 lines

[email protected] and [email protected] for a sanity check.
If you look at the man page of sparse, under the bitwise option, it states:
"Sparse will warn on [...] any conversion of one restricted type into another, except via a cast that includes __attribute__((force)).". In our case, it is a conversion from unrestricted to restricted which does not fall in this category.

The man page is not very clear here. It must be understood in the context where each use of '__bitwise' will create a new *distinct* type. Given this and the normal type checking, sparse should warn "on any conversion of one restricted type into another *or* between a restricted and the corresponding plain/unrestricted type" (or consider that an 'unrestricted type' is 'a restricted type with no restriction', which is, I think, what was meant here).

The fact that sparse doesn't warn in your case is clearly a bug in sparse's type checking.

Regards,
-- Luc Van Oostenryck

Thiebaud Weksteen

author

2018-04-24 09:57 UTC #9 lore raw

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

Quoted 5 lines · Luc Van Oostenryck

Quoted 4 lines · Thiebaud Weksteen
Quoted 2 lines · Jason Gunthorpe
Quoted 1 line · Thiebaud Weksteen
On Tue, Apr 17, 2018 at 4:00 PM Jason Gunthorpe <[email protected]>

Quoted 6 lines · wrote:

Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines · Thiebaud Weksteen
Quoted 1 line
On Tue, Apr 17, 2018 at 5:02 AM Jason Gunthorpe <[email protected]>
Quoted 2 lines · wrote:
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
On Thu, Apr 12, 2018 at 12:13:47PM +0200, Thiebaud Weksteen

Quoted 19 lines · wrote:

Quoted 19 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Signed-off-by Thiebaud Weksteen <[email protected]>
 drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/char/tpm/tpm_eventlog_of.c
Quoted 6 lines
Review int tpm_read_log_of(struct tpm_chip *chip) · line 58
Quoted 6 lines
index 96fd5646f866..d74568d58a66 100644
+++ b/drivers/char/tpm/tpm_eventlog_of.c
(quoted diff) +0 -0
@@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
       * but physical tpm needs the conversion.
       */
      if (of_property_match_string(np, "compatible",
"IBM,vtpm") <
Quoted 6 lines
Quoted 6 lines
0) {
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +2 -2
-             size = be32_to_cpup(sizep);
-             base = be64_to_cpup(basep);
+             size = be32_to_cpup((__be32 *)sizep);
+             base = be64_to_cpup((__be64 *)basep);

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Er, no.. change the definitions of sizep and basep to be __be
Jason
Please read the comment before the condition. sizep and basep may contain either little endian or big endian and this

block

Quoted 3 lines

Quoted 3 lines
is
Quoted 2 lines
Quoted 2 lines
used
Quoted 1 line
Quoted 1 line
to adjust that. Let me know if there is a better way for

handling

Quoted 3 lines

Quoted 3 lines
this.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Well a cast like that will throw sparse warnings, you need

__force at

Quoted 5 lines

Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
least
I don't think so. Since the variable is only defined as u32*, no
specific
Quoted 1 line
Quoted 1 line
warning is generated. I've used make C=2 drivers/char/tpm/ with

this

Quoted 8 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
patch applied and no new warning is being triggered.
Interesting.
Quoted 1 line
Quoted 1 line
I'm surprised to hear you say that..
Same for me.

Quoted 12 lines

Quoted 3 lines
Quoted 3 lines
Sparse is supposed to require force on all cast that change the annotation, and there are many examples in the kernel that have force in that case.
Yes, sparse is supposed to warn in such cases and the __force is there to quiets the warning when it is known that the cast is in fact correct.
Quoted 3 lines
[email protected] and [email protected] for a sanity check.
If you look at the man page of sparse, under the bitwise option, it

states:

Quoted 2 lines

Quoted 2 lines
"Sparse will warn on [...] any conversion of one restricted type into another, except via a cast that includes __attribute__((force)).". In

our

Quoted 10 lines

Quoted 2 lines
case, it is a conversion from unrestricted to restricted which does not fall in this category.
The man page is not very clear here. It must be understood in the context where each use of '__bitwise' will create a new *distinct* type. Given this and the normal type checking, sparse should warn "on any conversion of one restricted type into another *or* between a restricted and the corresponding plain/unrestricted type" (or consider that an 'unrestricted type' is 'a restricted type with no restriction', which is, I think, what was meant here).

Thanks for the explanation, that make sense. I believe the issue happens when dealing with restricted pointer types more than regular types. Also, this is not new and has probably been going on for the last 13 years. For instance, 81179bb6a54c2c626b4cbcc084ca974bb2d7f2a3 explicitly removed the __force attribute. I'll send a validation patch for sparse and update this patch.

Quoted 5 lines

The fact that sparse doesn't warn in your case is clearly a bug in sparse's type checking.
Regards,
-- Luc Van Oostenryck

Jarkko Sakkinen

Reviewed-by Tested-by on v2 1/4

2018-04-20 05:39 UTC #10 lore raw

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

In reply to Thiebaud Weksteen

Quoted 1 line · Thiebaud Weksteen

Signed-off-by Thiebaud Weksteen <[email protected]>

Reviewed-by Jarkko Sakkinen <[email protected]>

Tested-by Jarkko Sakkinen <[email protected]>

/Jarkko

Thiebaud Weksteen

author

2018-04-12 10:14 UTC #11 lore raw

PATCH v2 2/4 tpm: Move eventlog files to a subdirectory

In reply to Thiebaud Weksteen

Signed-off-by Thiebaud Weksteen <[email protected]>

---
 drivers/char/tpm/Makefile                                 | 8 ++++----
 drivers/char/tpm/{tpm_eventlog_acpi.c => eventlog/acpi.c} | 2 +-
 drivers/char/tpm/{tpm_eventlog_efi.c => eventlog/efi.c}   | 2 +-
 drivers/char/tpm/{tpm_eventlog_of.c => eventlog/of.c}     | 2 +-
 drivers/char/tpm/{tpm1_eventlog.c => eventlog/tpm1.c}     | 2 +-
 drivers/char/tpm/{tpm2_eventlog.c => eventlog/tpm2.c}     | 2 +-
 6 files changed, 9 insertions(+), 9 deletions(-)
 rename drivers/char/tpm/{tpm_eventlog_acpi.c => eventlog/acpi.c} (99%)
 rename drivers/char/tpm/{tpm_eventlog_efi.c => eventlog/efi.c} (98%)
 rename drivers/char/tpm/{tpm_eventlog_of.c => eventlog/of.c} (99%)
 rename drivers/char/tpm/{tpm1_eventlog.c => eventlog/tpm1.c} (99%)
 rename drivers/char/tpm/{tpm2_eventlog.c => eventlog/tpm2.c} (99%)

drivers/char/tpm/Makefile +4 -4

index acd758381c58..5dcf5bd35a3d 100644
@@ -4,11 +4,11 @@
 #
 obj-$(CONFIG_TCG_TPM) += tpm.o
 tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
-	 tpm-dev-common.o tpmrm-dev.o tpm1_eventlog.o tpm2_eventlog.o \
+	 tpm-dev-common.o tpmrm-dev.o eventlog/tpm1.o eventlog/tpm2.o \
          tpm2-space.o
-tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_eventlog_acpi.o
-tpm-$(CONFIG_EFI) += tpm_eventlog_efi.o
-tpm-$(CONFIG_OF) += tpm_eventlog_of.o
+tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o
+tpm-$(CONFIG_EFI) += eventlog/efi.o
+tpm-$(CONFIG_OF) += eventlog/of.o
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
 obj-$(CONFIG_TCG_TIS) += tpm_tis.o
 obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o

drivers/char/tpm/eventlog/acpi.c renamed from drivers/char/tpm/tpm_eventlog_acpi.c +1 -1

similarity index 99%
rename from drivers/char/tpm/tpm_eventlog_acpi.c
rename to drivers/char/tpm/eventlog/acpi.c
index 66f19e93c216..8476be2e9526 100644
@@ -27,7 +27,7 @@
 #include <linux/acpi.h>
 #include <linux/tpm_eventlog.h>
 
-#include "tpm.h"
+#include "../tpm.h"
 
 struct acpi_tcpa {
 	struct acpi_table_header hdr;

drivers/char/tpm/eventlog/efi.c renamed from drivers/char/tpm/tpm_eventlog_efi.c +1 -1

similarity index 98%
rename from drivers/char/tpm/tpm_eventlog_efi.c
rename to drivers/char/tpm/eventlog/efi.c
index e3f9ffd341d2..e1593c5271a4 100644
@@ -14,7 +14,7 @@
 #include <linux/efi.h>
 #include <linux/tpm_eventlog.h>
 
-#include "tpm.h"
+#include "../tpm.h"
 
 /* read binary bios log from EFI configuration table */
 int tpm_read_log_efi(struct tpm_chip *chip)

drivers/char/tpm/eventlog/of.c renamed from drivers/char/tpm/tpm_eventlog_of.c +1 -1

similarity index 99%
rename from drivers/char/tpm/tpm_eventlog_of.c
rename to drivers/char/tpm/eventlog/of.c
index d74568d58a66..d98c184d5c13 100644
@@ -19,7 +19,7 @@
 #include <linux/of.h>
 #include <linux/tpm_eventlog.h>
 
-#include "tpm.h"
+#include "../tpm.h"
 
 int tpm_read_log_of(struct tpm_chip *chip)
 {

drivers/char/tpm/eventlog/tpm1.c renamed from drivers/char/tpm/tpm1_eventlog.c +1 -1

similarity index 99%
rename from drivers/char/tpm/tpm1_eventlog.c
rename to drivers/char/tpm/eventlog/tpm1.c
index add798bd69d0..d6aea3ca950e 100644
@@ -28,7 +28,7 @@
 #include <linux/slab.h>
 #include <linux/tpm_eventlog.h>
 
-#include "tpm.h"
+#include "../tpm.h"
 
 
 static const char* tcpa_event_type_strings[] = {

drivers/char/tpm/eventlog/tpm2.c renamed from drivers/char/tpm/tpm2_eventlog.c +1 -1

similarity index 99%
rename from drivers/char/tpm/tpm2_eventlog.c
rename to drivers/char/tpm/eventlog/tpm2.c
index 1ce4411292ba..f0723fa9ae14 100644
@@ -23,7 +23,7 @@
 #include <linux/slab.h>
 #include <linux/tpm_eventlog.h>
 
-#include "tpm.h"
+#include "../tpm.h"
 
 /*
  * calc_tpm2_event_size() - calculate the event size, where event

Signature · 1 line

2.17.0.484.g0c8726318c-goog

Jarkko Sakkinen

Reviewed-by Tested-by on v2 2/4

2018-04-20 05:39 UTC #12 lore raw

Re: [PATCH v2 2/4] tpm: Move eventlog files to a subdirectory

Quoted 2 lines · Thiebaud Weksteen

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

Reviewed-by Jarkko Sakkinen <[email protected]>

Tested-by Jarkko Sakkinen <[email protected]>

/Jarkko

Mimi Zohar

2018-05-07 20:36 UTC #13 lore raw

Re: [PATCH v2 2/4] tpm: Move eventlog files to a subdirectory

[Cc'ing Petr Vorel and the ltp mailing list]

Hi Jarrko,

Quoted 6 lines · Jarkko Sakkinen

Quoted 2 lines · Thiebaud Weksteen
Signed-off-by Thiebaud Weksteen <[email protected]>
Suggested-by Jarkko Sakkinen <[email protected]>
Reviewed-by Jarkko Sakkinen <[email protected]>
Tested-by Jarkko Sakkinen <[email protected]>

I just noticed this is queued in your next branch.

Petr Vorel has been updating the IMA LTP tests. One of those IMA LTP tests includes walking the TPM binary_runtime_measurements in order to calculate the IMA boot-aggregate. The IMA boot-aggregate is the first measurement in the IMA measurement list.

Mimi

Petr Vorel

2018-05-09 14:51 UTC #14 lore raw

Re: [PATCH v2 2/4] tpm: Move eventlog files to a subdirectory

Hi Mimi,

Quoted 12 lines

[Cc'ing Petr Vorel and the ltp mailing list]
Hi Jarrko,
Quoted 6 lines · Jarkko Sakkinen
Quoted 2 lines · Thiebaud Weksteen
Signed-off-by Thiebaud Weksteen <[email protected]>
Suggested-by Jarkko Sakkinen <[email protected]>
Reviewed-by Jarkko Sakkinen <[email protected]>
Tested-by Jarkko Sakkinen <[email protected]>

Quoted 6 lines

I just noticed this is queued in your next branch.
Petr Vorel has been updating the IMA LTP tests. One of those IMA LTP tests includes walking the TPM binary_runtime_measurements in order to calculate the IMA boot-aggregate. The IMA boot-aggregate is the first measurement in the IMA measurement list.

Did you meant that this commit ([2]) in linux-tpmdd/next changed location of binary_runtime_measurements in sysfs?

IMHO these commits ([1], [2], [3]) just put source code into eventlog/ subdirectory:

Quoted 1 line

Mimi

Kind regards,
Petr

[1] 75d647f5de69 tpm: Move eventlog declarations to its own header
[2] 9b01b5356629 tpm: Move shared eventlog functions to common.c
[3] 0bfb23746052 tpm: Move eventlog files to a subdirectory

Mimi Zohar

2018-05-11 01:51 UTC #15 lore raw

Re: [PATCH v2 2/4] tpm: Move eventlog files to a subdirectory

Quoted 21 lines · Petr Vorel

Hi Mimi,
Quoted 12 lines
[Cc'ing Petr Vorel and the ltp mailing list]
Hi Jarrko,
Quoted 6 lines · Jarkko Sakkinen
Quoted 2 lines · Thiebaud Weksteen
Signed-off-by Thiebaud Weksteen <[email protected]>
Suggested-by Jarkko Sakkinen <[email protected]>
Reviewed-by Jarkko Sakkinen <[email protected]>
Tested-by Jarkko Sakkinen <[email protected]>
Quoted 6 lines
I just noticed this is queued in your next branch.
Petr Vorel has been updating the IMA LTP tests. One of those IMA LTP tests includes walking the TPM binary_runtime_measurements in order to calculate the IMA boot-aggregate. The IMA boot-aggregate is the first measurement in the IMA measurement list.

Quoted 4 lines

Did you meant that this commit ([2]) in linux-tpmdd/next changed location of binary_runtime_measurements in sysfs?
IMHO these commits ([1], [2], [3]) just put source code into eventlog/ subdirectory:

I mistakenly thought the eventlog itself was being moved, not the source code.

thanks,

Mimi

Quoted 8 lines

Kind regards,
Petr
[1] 75d647f5de69 tpm: Move eventlog declarations to its own header
[2] 9b01b5356629 tpm: Move shared eventlog functions to common.c
[3] 0bfb23746052 tpm: Move eventlog files to a subdirectory

Jarkko Sakkinen

2018-05-10 01:39 UTC #16 lore raw

Re: [PATCH v2 2/4] tpm: Move eventlog files to a subdirectory

In reply to Mimi Zohar

Quoted 12 lines · Mimi Zohar

[Cc'ing Petr Vorel and the ltp mailing list]
Hi Jarrko,
Quoted 6 lines · Jarkko Sakkinen
Quoted 2 lines · Thiebaud Weksteen
Signed-off-by Thiebaud Weksteen <[email protected]>
Suggested-by Jarkko Sakkinen <[email protected]>
Reviewed-by Jarkko Sakkinen <[email protected]>
Tested-by Jarkko Sakkinen <[email protected]>

Quoted 8 lines

I just noticed this is queued in your next branch.
Petr Vorel has been updating the IMA LTP tests. One of those IMA LTP tests includes walking the TPM binary_runtime_measurements in order to calculate the IMA boot-aggregate. The IMA boot-aggregate is the first measurement in the IMA measurement list.
Mimi

Sorry, not following how these connect directly?

/Jarkko

Thiebaud Weksteen

author

2018-04-12 10:14 UTC #17 lore raw

PATCH v2 3/4 tpm: Move shared eventlog functions to common.c

In reply to Thiebaud Weksteen

Functions and structures specific to TPM1 are renamed from tpm* to tpm1*.

Signed-off-by Thiebaud Weksteen <[email protected]>

---
 drivers/char/tpm/Makefile          |   4 +-
 drivers/char/tpm/eventlog/common.c | 195 ++++++++++++++++++++++++++++
 drivers/char/tpm/eventlog/tpm1.c   | 197 +++--------------------------
 drivers/char/tpm/tpm.h             |   2 +
 4 files changed, 214 insertions(+), 184 deletions(-)
 create mode 100644 drivers/char/tpm/eventlog/common.c

drivers/char/tpm/Makefile +2 -2

index 5dcf5bd35a3d..4e9c33ca1f8f 100644
@@ -4,8 +4,8 @@
 #
 obj-$(CONFIG_TCG_TPM) += tpm.o
 tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
-	 tpm-dev-common.o tpmrm-dev.o eventlog/tpm1.o eventlog/tpm2.o \
-         tpm2-space.o
+	 tpm-dev-common.o tpmrm-dev.o eventlog/common.o eventlog/tpm1.o \
+	 eventlog/tpm2.o tpm2-space.o
 tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o
 tpm-$(CONFIG_EFI) += eventlog/efi.o
 tpm-$(CONFIG_OF) += eventlog/of.o

drivers/char/tpm/eventlog/common.c new file +195 -0

195 lines not on this page · raw message

drivers/char/tpm/eventlog/tpm1.c +15 ~~-182~~

233 lines not on this page · raw message

drivers/char/tpm/tpm.h +2 -0

index f895fba4e20d..a583c5001904 100644
@@ -580,6 +580,8 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 		      u32 cc, u8 *buf, size_t *bufsiz);
 
+extern const struct seq_operations tpm1_ascii_b_measurements_seqops;
+extern const struct seq_operations tpm1_binary_b_measurements_seqops;
 extern const struct seq_operations tpm2_binary_b_measurements_seqops;
 
 #if defined(CONFIG_ACPI)

Signature · 1 line

2.17.0.484.g0c8726318c-goog

Jarkko Sakkinen

Reviewed-by Tested-by on v2 3/4

2018-04-20 05:40 UTC #18 lore raw

Re: [PATCH v2 3/4] tpm: Move shared eventlog functions to common.c

Quoted 4 lines · Thiebaud Weksteen

Functions and structures specific to TPM1 are renamed from tpm* to tpm1*.
Signed-off-by Thiebaud Weksteen <[email protected]>
Suggested-by Jarkko Sakkinen <[email protected]>

Reviewed-by Jarkko Sakkinen <[email protected]>

Tested-by Jarkko Sakkinen <[email protected]>

/Jarkko

Thiebaud Weksteen

author

2018-04-12 10:14 UTC #19 lore raw

PATCH v2 4/4 tpm: Move eventlog declarations to its own header

In reply to Thiebaud Weksteen

Reduce the size of tpm.h by moving eventlog declarations to a separate header.

Signed-off-by Thiebaud Weksteen <[email protected]>

---
 drivers/char/tpm/eventlog/acpi.c   |  1 +
 drivers/char/tpm/eventlog/common.c |  2 +-
 drivers/char/tpm/eventlog/common.h | 35 ++++++++++++++++++++++++++++++
 drivers/char/tpm/eventlog/efi.c    |  1 +
 drivers/char/tpm/eventlog/of.c     |  1 +
 drivers/char/tpm/eventlog/tpm1.c   |  1 +
 drivers/char/tpm/eventlog/tpm2.c   |  1 +
 drivers/char/tpm/tpm.h             | 29 -------------------------
 8 files changed, 41 insertions(+), 30 deletions(-)
 create mode 100644 drivers/char/tpm/eventlog/common.h

drivers/char/tpm/eventlog/acpi.c +1 -0

index 8476be2e9526..7c53b1973b62 100644
@@ -28,6 +28,7 @@
 #include <linux/tpm_eventlog.h>
 
 #include "../tpm.h"
+#include "common.h"
 
 struct acpi_tcpa {
 	struct acpi_table_header hdr;

drivers/char/tpm/eventlog/common.c +1 -1

index 54934b5a1566..5a8720df2b51 100644
@@ -25,7 +25,7 @@
 #include <linux/tpm_eventlog.h>
 
 #include "../tpm.h"
-
+#include "common.h"
 
 static int tpm_bios_measurements_open(struct inode *inode,
 					    struct file *file)

drivers/char/tpm/eventlog/common.h new file +35 -0

new file mode 100644
index 000000000000..47ff8136ceb5
@@ -0,0 +1,35 @@
+#ifndef __TPM_EVENTLOG_COMMON_H__
+#define __TPM_EVENTLOG_COMMON_H__
+
+#include "../tpm.h"
+
+extern const struct seq_operations tpm1_ascii_b_measurements_seqops;
+extern const struct seq_operations tpm1_binary_b_measurements_seqops;
+extern const struct seq_operations tpm2_binary_b_measurements_seqops;
+
+#if defined(CONFIG_ACPI)
+int tpm_read_log_acpi(struct tpm_chip *chip);
+#else
+static inline int tpm_read_log_acpi(struct tpm_chip *chip)
+{
+	return -ENODEV;
+}
+#endif
+#if defined(CONFIG_OF)
+int tpm_read_log_of(struct tpm_chip *chip);
+#else
+static inline int tpm_read_log_of(struct tpm_chip *chip)
+{
+	return -ENODEV;
+}
+#endif
+#if defined(CONFIG_EFI)
+int tpm_read_log_efi(struct tpm_chip *chip);
+#else
+static inline int tpm_read_log_efi(struct tpm_chip *chip)
+{
+	return -ENODEV;
+}
+#endif
+
+#endif

drivers/char/tpm/eventlog/efi.c +1 -0

index e1593c5271a4..68f1e7ee60ce 100644
@@ -15,6 +15,7 @@
 #include <linux/tpm_eventlog.h>
 
 #include "../tpm.h"
+#include "common.h"
 
 /* read binary bios log from EFI configuration table */
 int tpm_read_log_efi(struct tpm_chip *chip)

drivers/char/tpm/eventlog/of.c +1 -0

index d98c184d5c13..f8ad4c3ffe8e 100644
@@ -20,6 +20,7 @@
 #include <linux/tpm_eventlog.h>
 
 #include "../tpm.h"
+#include "common.h"
 
 int tpm_read_log_of(struct tpm_chip *chip)
 {

drivers/char/tpm/eventlog/tpm1.c +1 -0

index 8f30316e9bb6..58c84784ba25 100644
@@ -29,6 +29,7 @@
 #include <linux/tpm_eventlog.h>
 
 #include "../tpm.h"
+#include "common.h"
 
 
 static const char* tcpa_event_type_strings[] = {

drivers/char/tpm/eventlog/tpm2.c +1 -0

index f0723fa9ae14..1b8fa9de2cac 100644
@@ -24,6 +24,7 @@
 #include <linux/tpm_eventlog.h>
 
 #include "../tpm.h"
+#include "common.h"
 
 /*
  * calc_tpm2_event_size() - calculate the event size, where event

drivers/char/tpm/tpm.h +0 ~~-29~~

index a583c5001904..067f305e36f0 100644
@@ -580,35 +580,6 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 		      u32 cc, u8 *buf, size_t *bufsiz);
 
-extern const struct seq_operations tpm1_ascii_b_measurements_seqops;
-extern const struct seq_operations tpm1_binary_b_measurements_seqops;
-extern const struct seq_operations tpm2_binary_b_measurements_seqops;
-
-#if defined(CONFIG_ACPI)
-int tpm_read_log_acpi(struct tpm_chip *chip);
-#else
-static inline int tpm_read_log_acpi(struct tpm_chip *chip)
-{
-	return -ENODEV;
-}
-#endif
-#if defined(CONFIG_OF)
-int tpm_read_log_of(struct tpm_chip *chip);
-#else
-static inline int tpm_read_log_of(struct tpm_chip *chip)
-{
-	return -ENODEV;
-}
-#endif
-#if defined(CONFIG_EFI)
-int tpm_read_log_efi(struct tpm_chip *chip);
-#else
-static inline int tpm_read_log_efi(struct tpm_chip *chip)
-{
-	return -ENODEV;
-}
-#endif
-
 int tpm_bios_log_setup(struct tpm_chip *chip);
 void tpm_bios_log_teardown(struct tpm_chip *chip);
 #endif

Signature · 1 line

2.17.0.484.g0c8726318c-goog

Jarkko Sakkinen

Reviewed-by Tested-by on v2 4/4

2018-04-20 05:40 UTC #20 lore raw

Re: [PATCH v2 4/4] tpm: Move eventlog declarations to its own header

Quoted 5 lines · Thiebaud Weksteen

Reduce the size of tpm.h by moving eventlog declarations to a separate header.
Signed-off-by Thiebaud Weksteen <[email protected]>
Suggested-by Jarkko Sakkinen <[email protected]>

Reviewed-by Jarkko Sakkinen <[email protected]>

Tested-by Jarkko Sakkinen <[email protected]>

/Jarkko