[RFC PATCH v2 0/3] Preserve TPM log across kexec

[RFC PATCH v2 0/3] Preserve TPM log across kexec

[Stefan Berger]

This series resolves an issue on PowerVM and KVM on Power where the memory
the TPM log was held in may become inaccessible or corrupted after a kexec
soft reboot. The solution on these two platforms is to store the whole log
in the device tree because the device tree is preserved across a kexec with
either of the two kexec syscalls.

Regards,
   Stefan

v2:
 - Added DT bindings patch (2/3)
 - Reformulated commit messages and addded Fixes tags
 - Follow Michael's suggestion on prom_init patch (1/3)

Stefan Berger (3):
  powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log
  dt-bindings: tpm: Add linux,sml-log to ibm,vtpm.yaml
  tpm: of: If available use linux,sml-log to get the log and its size

 .../devicetree/bindings/tpm/ibm,vtpm.yaml     | 20 +++++++++--
 .../devicetree/bindings/tpm/tpm-common.yaml   | 14 +++++++-
 arch/powerpc/kernel/prom_init.c               | 27 +++++++++-----
 drivers/char/tpm/eventlog/of.c                | 36 ++++++-------------
 4 files changed, 61 insertions(+), 36 deletions(-)

-- 
2.43.0

[RFC PATCH v2 1/3] powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log

[Stefan Berger]

linux,sml-base holds the address of a buffer with the TPM log. This
buffer may become invalid after a kexec. To avoid accessing an invalid
address or corrupted buffer, embed the whole TPM log in the device tree
property linux,sml-log. This helps to protect the log since it is
properly carried across a kexec soft reboot with both of the kexec
syscalls.

Avoid having the firmware ingest the whole TPM log when calling
prom_setprop but only create the linux,sml-log property as a place holder.
Insert the actual TPM log during the tree flattening phase.

Fixes: 4a727429abec ("PPC64: Add support for instantiating SML from Open Firmware")
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 arch/powerpc/kernel/prom_init.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index e67effdba85c..6f7ca72013c2 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -211,6 +211,8 @@ static cell_t __prombss regbuf[1024];
 
 static bool  __prombss rtas_has_query_cpu_stopped;
 
+static u64 __prombss sml_base;
+static u32 __prombss sml_size;
 
 /*
  * Error results ... some OF calls will return "-1" on error, some
@@ -1954,17 +1956,15 @@ static void __init prom_instantiate_sml(void)
 	}
 	prom_printf(" done\n");
 
-	reserve_mem(base, size);
-
-	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
-		     &base, sizeof(base));
-	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
-		     &size, sizeof(size));
-
-	prom_debug("sml base     = 0x%llx\n", base);
+	/* Add property now, defer adding log to tree flattening phase */
+	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-log",
+		     NULL, 0);
 	prom_debug("sml size     = 0x%x\n", size);
 
 	prom_debug("prom_instantiate_sml: end...\n");
+
+	sml_base = base;
+	sml_size = size;
 }
 
 /*
@@ -2645,6 +2645,17 @@ static void __init scan_dt_build_struct(phandle node, unsigned long *mem_start,
 		}
 		prev_name = sstart + soff;
 
+		if (!prom_strcmp("linux,sml-log", pname)) {
+			/* push property head */
+			dt_push_token(OF_DT_PROP, mem_start, mem_end);
+			dt_push_token(sml_size, mem_start, mem_end);
+			dt_push_token(soff, mem_start, mem_end);
+			/* push property content */
+			valp = make_room(mem_start, mem_end, sml_size, 1);
+			memcpy(valp, (void *)sml_base, sml_size);
+			*mem_start = ALIGN(*mem_start, 4);
+			continue;
+		}
 		/* get length */
 		l = call_prom("getproplen", 2, 1, node, pname);
 
-- 
2.43.0

[RFC PATCH v2 2/3] dt-bindings: tpm: Add linux,sml-log to ibm,vtpm.yaml

[Stefan Berger]

Add linux,sml-log, which carries the firmware TPM log in a uint8-array, to
the properties. Either this property is required or both linux,sml-base and
linux,sml-size are required. Add a test case for verification.

Fixes: 82003e0487fb ("Documentation: tpm: add the IBM Virtual TPM device tree binding documentation")
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Nayna Jain <nayna@linux.ibm.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 .../devicetree/bindings/tpm/ibm,vtpm.yaml     | 20 +++++++++++++++++--
 .../devicetree/bindings/tpm/tpm-common.yaml   | 14 ++++++++++++-
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml b/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
index 50a3fd31241c..8885ef3b7638 100644
--- a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
+++ b/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
@@ -74,8 +74,6 @@ required:
   - ibm,my-dma-window
   - ibm,my-drc-index
   - ibm,loc-code
-  - linux,sml-base
-  - linux,sml-size
 
 allOf:
   - $ref: tpm-common.yaml#
@@ -102,3 +100,21 @@ examples:
             linux,sml-size = <0xbce10200>;
         };
     };
+  - |
+    soc {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        tpm@30000003 {
+            compatible = "IBM,vtpm20";
+            device_type = "IBM,vtpm";
+            reg = <0x30000003>;
+            interrupts = <0xa0003 0x0>;
+            ibm,#dma-address-cells = <0x2>;
+            ibm,#dma-size-cells = <0x2>;
+            ibm,my-dma-window = <0x10000003 0x0 0x0 0x0 0x10000000>;
+            ibm,my-drc-index = <0x30000003>;
+            ibm,loc-code = "U9080.HEX.134CA08-V7-C3";
+            linux,sml-log = <00 00 00 00 03 00 00>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/tpm/tpm-common.yaml b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
index 3c1241b2a43f..f6f0b551268c 100644
--- a/Documentation/devicetree/bindings/tpm/tpm-common.yaml
+++ b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
@@ -30,6 +30,11 @@ properties:
       size of reserved memory allocated for firmware event log
     $ref: /schemas/types.yaml#/definitions/uint32
 
+  linux,sml-log:
+    description:
+      Content of firmware event log
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+
   memory-region:
     description: reserved memory allocated for firmware event log
     maxItems: 1
@@ -53,15 +58,22 @@ dependentRequired:
   linux,sml-base: ['linux,sml-size']
   linux,sml-size: ['linux,sml-base']
 
-# must only have either memory-region or linux,sml-base
+# must only have either memory-region or linux,sml-base/size or linux,sml-log
 # as well as either resets or reset-gpios
 dependentSchemas:
   memory-region:
     properties:
       linux,sml-base: false
+      linux,sml-log: false
   linux,sml-base:
     properties:
       memory-region: false
+      linux,sml-log: false
+  linux,sml-log:
+    properties:
+      memory-region: false
+      linux,sml-base: false
+      linux,sml-size: false
   resets:
     properties:
       reset-gpios: false
-- 
2.43.0

[RFC PATCH v2 3/3] tpm: of: If available use linux,sml-log to get the log and its size

[Stefan Berger]

If linux,sml-log is available use it to get the TPM log rather than the
pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
on Power where after a kexec the memory pointed to by linux,sml-base may
have become inaccessible or corrupted. Also, linux,sml-log has replaced
linux,sml-base and linux,sml-size on these two platforms.

Keep the handling of linux,sml-base/sml-size for powernv platforms that
provide the two properties via skiboot.

Fixes: c5df39262dd5 ("drivers/char/tpm: Add securityfs support for event log")
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 drivers/char/tpm/eventlog/of.c | 36 +++++++++++-----------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
index 930fe43d5daf..dbd837d65264 100644
--- a/drivers/char/tpm/eventlog/of.c
+++ b/drivers/char/tpm/eventlog/of.c
@@ -54,8 +54,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	const u32 *sizep;
 	const u64 *basep;
 	struct tpm_bios_log *log;
+	const void *logp;
 	u32 size;
-	u64 base;
 
 	log = &chip->log;
 	if (chip->dev.parent && chip->dev.parent->of_node)
@@ -66,37 +66,23 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	if (of_property_read_bool(np, "powered-while-suspended"))
 		chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
 
-	sizep = of_get_property(np, "linux,sml-size", NULL);
-	basep = of_get_property(np, "linux,sml-base", NULL);
-	if (sizep == NULL && basep == NULL)
-		return tpm_read_log_memory_region(chip);
-	if (sizep == NULL || basep == NULL)
-		return -EIO;
-
-	/*
-	 * For both vtpm/tpm, firmware has log addr and log size in big
-	 * endian format. But in case of vtpm, there is a method called
-	 * sml-handover which is run during kernel init even before
-	 * device tree is setup. This sml-handover function takes care
-	 * of endianness and writes to sml-base and sml-size in little
-	 * endian format. For this reason, vtpm doesn't need conversion
-	 * but physical tpm needs the conversion.
-	 */
-	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
-	    of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
+	logp = of_get_property(np, "linux,sml-log", &size);
+	if (logp == NULL) {
+		sizep = of_get_property(np, "linux,sml-size", NULL);
+		basep = of_get_property(np, "linux,sml-base", NULL);
+		if (sizep == NULL && basep == NULL)
+			return tpm_read_log_memory_region(chip);
+		if (sizep == NULL || basep == NULL)
+			return -EIO;
 		size = be32_to_cpup((__force __be32 *)sizep);
-		base = be64_to_cpup((__force __be64 *)basep);
-	} else {
-		size = *sizep;
-		base = *basep;
+		logp = __va(be64_to_cpup((__force __be64 *)basep));
 	}
-
 	if (size == 0) {
 		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
 		return -EIO;
 	}
 
-	log->bios_event_log = devm_kmemdup(&chip->dev, __va(base), size, GFP_KERNEL);
+	log->bios_event_log = devm_kmemdup(&chip->dev, logp, size, GFP_KERNEL);
 	if (!log->bios_event_log)
 		return -ENOMEM;
 
-- 
2.43.0

Re: [RFC PATCH v2 1/3] powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log

[Christophe Leroy]

Le 11/03/2024 à 14:20, Stefan Berger a écrit :
> linux,sml-base holds the address of a buffer with the TPM log. This
> buffer may become invalid after a kexec. To avoid accessing an invalid
> address or corrupted buffer, embed the whole TPM log in the device tree
> property linux,sml-log. This helps to protect the log since it is
> properly carried across a kexec soft reboot with both of the kexec
> syscalls.
> 
> Avoid having the firmware ingest the whole TPM log when calling
> prom_setprop but only create the linux,sml-log property as a place holder.
> Insert the actual TPM log during the tree flattening phase.
> 
> Fixes: 4a727429abec ("PPC64: Add support for instantiating SML from Open Firmware")
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   arch/powerpc/kernel/prom_init.c | 27 +++++++++++++++++++--------
>   1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index e67effdba85c..6f7ca72013c2 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -211,6 +211,8 @@ static cell_t __prombss regbuf[1024];
>   
>   static bool  __prombss rtas_has_query_cpu_stopped;
>   
> +static u64 __prombss sml_base;
> +static u32 __prombss sml_size;
>   
>   /*
>    * Error results ... some OF calls will return "-1" on error, some
> @@ -1954,17 +1956,15 @@ static void __init prom_instantiate_sml(void)
>   	}
>   	prom_printf(" done\n");
>   
> -	reserve_mem(base, size);
> -
> -	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
> -		     &base, sizeof(base));
> -	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
> -		     &size, sizeof(size));
> -
> -	prom_debug("sml base     = 0x%llx\n", base);
> +	/* Add property now, defer adding log to tree flattening phase */
> +	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-log",
> +		     NULL, 0);
>   	prom_debug("sml size     = 0x%x\n", size);
>   
>   	prom_debug("prom_instantiate_sml: end...\n");
> +
> +	sml_base = base;
> +	sml_size = size;
>   }
>   
>   /*
> @@ -2645,6 +2645,17 @@ static void __init scan_dt_build_struct(phandle node, unsigned long *mem_start,
>   		}
>   		prev_name = sstart + soff;
>   
> +		if (!prom_strcmp("linux,sml-log", pname)) {
> +			/* push property head */
> +			dt_push_token(OF_DT_PROP, mem_start, mem_end);
> +			dt_push_token(sml_size, mem_start, mem_end);
> +			dt_push_token(soff, mem_start, mem_end);
> +			/* push property content */
> +			valp = make_room(mem_start, mem_end, sml_size, 1);
> +			memcpy(valp, (void *)sml_base, sml_size);

You can't cast a u64 into a pointer. If sml_base is an address, it must 
be declared as an unsigned long.

Build with pmac32_defconfig :

   CC      arch/powerpc/kernel/prom_init.o
arch/powerpc/kernel/prom_init.c: In function 'scan_dt_build_struct':
arch/powerpc/kernel/prom_init.c:2663:38: error: cast to pointer from 
integer of different size [-Werror=int-to-pointer-cast]
  2663 |                         memcpy(valp, (void *)sml_base, sml_size);
       |                                      ^
cc1: all warnings being treated as errors
make[4]: *** [scripts/Makefile.build:243: 
arch/powerpc/kernel/prom_init.o] Error 1


> +			*mem_start = ALIGN(*mem_start, 4);
> +			continue;
> +		}
>   		/* get length */
>   		l = call_prom("getproplen", 2, 1, node, pname);
>   

Re: [RFC PATCH v2 1/3] powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log

[Jerry Snitselaar]

On Mon, Mar 11, 2024 at 09:20:28AM -0400, Stefan Berger wrote:
> linux,sml-base holds the address of a buffer with the TPM log. This
> buffer may become invalid after a kexec. To avoid accessing an invalid
> address or corrupted buffer, embed the whole TPM log in the device tree
> property linux,sml-log. This helps to protect the log since it is
> properly carried across a kexec soft reboot with both of the kexec
> syscalls.
> 
> Avoid having the firmware ingest the whole TPM log when calling
> prom_setprop but only create the linux,sml-log property as a place holder.
> Insert the actual TPM log during the tree flattening phase.
> 
> Fixes: 4a727429abec ("PPC64: Add support for instantiating SML from Open Firmware")
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  arch/powerpc/kernel/prom_init.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index e67effdba85c..6f7ca72013c2 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -211,6 +211,8 @@ static cell_t __prombss regbuf[1024];
>  
>  static bool  __prombss rtas_has_query_cpu_stopped;
>  
> +static u64 __prombss sml_base;
> +static u32 __prombss sml_size;

Should inside an #ifdef CONFIG_PPC64 since prom_instantiate_sml() is?

>  
>  /*
>   * Error results ... some OF calls will return "-1" on error, some
> @@ -1954,17 +1956,15 @@ static void __init prom_instantiate_sml(void)
>  	}
>  	prom_printf(" done\n");
>  
> -	reserve_mem(base, size);
> -
> -	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
> -		     &base, sizeof(base));
> -	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
> -		     &size, sizeof(size));
> -
> -	prom_debug("sml base     = 0x%llx\n", base);
> +	/* Add property now, defer adding log to tree flattening phase */
> +	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-log",
> +		     NULL, 0);
>  	prom_debug("sml size     = 0x%x\n", size);
>  
>  	prom_debug("prom_instantiate_sml: end...\n");
> +
> +	sml_base = base;
> +	sml_size = size;
>  }
>  
>  /*
> @@ -2645,6 +2645,17 @@ static void __init scan_dt_build_struct(phandle node, unsigned long *mem_start,
>  		}
>  		prev_name = sstart + soff;
>  
> +		if (!prom_strcmp("linux,sml-log", pname)) {
> +			/* push property head */
> +			dt_push_token(OF_DT_PROP, mem_start, mem_end);
> +			dt_push_token(sml_size, mem_start, mem_end);
> +			dt_push_token(soff, mem_start, mem_end);
> +			/* push property content */
> +			valp = make_room(mem_start, mem_end, sml_size, 1);
> +			memcpy(valp, (void *)sml_base, sml_size);
> +			*mem_start = ALIGN(*mem_start, 4);
> +			continue;
> +		}

Same question as above.

Regards,
Jerry

>  		/* get length */
>  		l = call_prom("getproplen", 2, 1, node, pname);
>  
> -- 
> 2.43.0
> 

Re: [RFC PATCH v2 1/3] powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log

[Stefan Berger]

On 3/11/24 13:24, Christophe Leroy wrote:
> 
> 
> Le 11/03/2024 à 14:20, Stefan Berger a écrit :
>> linux,sml-base holds the address of a buffer with the TPM log. This
>> buffer may become invalid after a kexec. To avoid accessing an invalid
>> address or corrupted buffer, embed the whole TPM log in the device tree
>> property linux,sml-log. This helps to protect the log since it is
>> properly carried across a kexec soft reboot with both of the kexec
>> syscalls.
>>
>> Avoid having the firmware ingest the whole TPM log when calling
>> prom_setprop but only create the linux,sml-log property as a place holder.
>> Insert the actual TPM log during the tree flattening phase.
>>
>> Fixes: 4a727429abec ("PPC64: Add support for instantiating SML from Open Firmware")
>> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---

>> @@ -2645,6 +2645,17 @@ static void __init scan_dt_build_struct(phandle node, unsigned long *mem_start,
>>    		}
>>    		prev_name = sstart + soff;
>>    
>> +		if (!prom_strcmp("linux,sml-log", pname)) {
>> +			/* push property head */
>> +			dt_push_token(OF_DT_PROP, mem_start, mem_end);
>> +			dt_push_token(sml_size, mem_start, mem_end);
>> +			dt_push_token(soff, mem_start, mem_end);
>> +			/* push property content */
>> +			valp = make_room(mem_start, mem_end, sml_size, 1);
>> +			memcpy(valp, (void *)sml_base, sml_size);
> 
> You can't cast a u64 into a pointer. If sml_base is an address, it must
> be declared as an unsigned long.
> 
> Build with pmac32_defconfig :
> 
>     CC      arch/powerpc/kernel/prom_init.o
> arch/powerpc/kernel/prom_init.c: In function 'scan_dt_build_struct':
> arch/powerpc/kernel/prom_init.c:2663:38: error: cast to pointer from
> integer of different size [-Werror=int-to-pointer-cast]
>    2663 |                         memcpy(valp, (void *)sml_base, sml_size);
>         |                                      ^
> cc1: all warnings being treated as errors
> make[4]: *** [scripts/Makefile.build:243:
> arch/powerpc/kernel/prom_init.o] Error 1

Next round will have this block under #ifdef CONFIG_PPC64.

Thanks.

Re: [RFC PATCH v2 1/3] powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log

[Jarkko Sakkinen]

On Mon Mar 11, 2024 at 3:20 PM EET, Stefan Berger wrote:
> linux,sml-base holds the address of a buffer with the TPM log. This
> buffer may become invalid after a kexec. To avoid accessing an invalid
> address or corrupted buffer, embed the whole TPM log in the device tree
> property linux,sml-log. This helps to protect the log since it is
> properly carried across a kexec soft reboot with both of the kexec
> syscalls.

- Describe the environment where TPM log gets corrupted.
- Describe why TPM log gets corrupted on kexec.

>
> Avoid having the firmware ingest the whole TPM log when calling
> prom_setprop but only create the linux,sml-log property as a place holder.
> Insert the actual TPM log during the tree flattening phase.

This commit message should shed some light about reasons of the
corruption in order to conclude that it should be fixed up like
this. I.e. why the "post-state" is a legit state where can be
continued despite a log being corrupted. Especially in security
features this is pretty essential information.

BR, Jarkko

Re: [RFC PATCH v2 3/3] tpm: of: If available use linux,sml-log to get the log and its size

[Jarkko Sakkinen]

On Mon Mar 11, 2024 at 3:20 PM EET, Stefan Berger wrote:
> If linux,sml-log is available use it to get the TPM log rather than the
> pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
> on Power where after a kexec the memory pointed to by linux,sml-base may
> have become inaccessible or corrupted. Also, linux,sml-log has replaced
> linux,sml-base and linux,sml-size on these two platforms.
>
> Keep the handling of linux,sml-base/sml-size for powernv platforms that
> provide the two properties via skiboot.
>
> Fixes: c5df39262dd5 ("drivers/char/tpm: Add securityfs support for event log")
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

I'm worried about not being up to date and instead using "cached" values
when verifying anything from a security chip. Does this guarantee that
TPM log is corrupted and will not get updated somehow?

BR, Jarkko

Re: [RFC PATCH v2 3/3] tpm: of: If available use linux,sml-log to get the log and its size

[Stefan Berger]

On 3/11/24 16:25, Jarkko Sakkinen wrote:
> On Mon Mar 11, 2024 at 3:20 PM EET, Stefan Berger wrote:
>> If linux,sml-log is available use it to get the TPM log rather than the
>> pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
>> on Power where after a kexec the memory pointed to by linux,sml-base may
>> have become inaccessible or corrupted. Also, linux,sml-log has replaced
>> linux,sml-base and linux,sml-size on these two platforms.
>>
>> Keep the handling of linux,sml-base/sml-size for powernv platforms that
>> provide the two properties via skiboot.
>>
>> Fixes: c5df39262dd5 ("drivers/char/tpm: Add securityfs support for event log")
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> 
> I'm worried about not being up to date and instead using "cached" values
> when verifying anything from a security chip. Does this guarantee that
> TPM log is corrupted and will not get updated somehow?


What do you mean 'guarantee that TPM log is corrupted'?

The TPM was handed over from the firmware to Linux and the firmware then 
freed all memory associated with the log and will then not create a new 
log or touch the TPM or do anything that would require an update to the 
handed-over log. Linux also does not append to the firmware log. So 
whatever we now find in linux,sml-log would be the latest firmware log 
and the state of the 'firmware PCRs' computed from it should correspond 
to the current state of the 'firmware PCRs'.

> 
> BR, Jarkko
> 

Re: [RFC PATCH v2 2/3] dt-bindings: tpm: Add linux,sml-log to ibm,vtpm.yaml

[Lukas Wunner]

On Mon, Mar 11, 2024 at 09:20:29AM -0400, Stefan Berger wrote:
> Add linux,sml-log, which carries the firmware TPM log in a uint8-array, to
> the properties. Either this property is required or both linux,sml-base and
> linux,sml-size are required. Add a test case for verification.
> 
> Fixes: 82003e0487fb ("Documentation: tpm: add the IBM Virtual TPM device tree binding documentation")

The Fixes tag is confusing.  The patch won't even apply cleanly to the
v4.10 commit referenced here as the conversion to yaml happened only
recently with v6.8.

Why is the Fixes tag necessary in the first place?  Same question for
the other patches in the series.  This looks like feature work rather
than a fix.  Not sure whether it satisfies the "obviously correct"
rule per Documentation/process/stable-kernel-rules.rst.


> --- a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
> +++ b/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
> @@ -74,8 +74,6 @@ required:
>    - ibm,my-dma-window
>    - ibm,my-drc-index
>    - ibm,loc-code
> -  - linux,sml-base
> -  - linux,sml-size

I assume that either these two or the new "linux,sml-log" property
are (still) required?  If so, a quick grep through the bindings
(e.g. auxdisplay/img,ascii-lcd.yaml) shows that the following
might work:

required:
  - ...

oneOf:
  - required:
      - linux,sml-base
  - required:
      - linux,sml-log


> --- a/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> +++ b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> @@ -30,6 +30,11 @@ properties:
>        size of reserved memory allocated for firmware event log
>      $ref: /schemas/types.yaml#/definitions/uint32
>  
> +  linux,sml-log:
> +    description:
> +      Content of firmware event log

Please add one or two sentences of context so that readers don't
need to use git blame + git log to find out what this is for.
(Mention at least that the property may be used to pass the log
to a kexec kernel.)


> -# must only have either memory-region or linux,sml-base
> +# must only have either memory-region or linux,sml-base/size or linux,sml-log
>  # as well as either resets or reset-gpios
>  dependentSchemas:
>    memory-region:
>      properties:
>        linux,sml-base: false
> +      linux,sml-log: false
>    linux,sml-base:
>      properties:
>        memory-region: false
> +      linux,sml-log: false
> +  linux,sml-log:
> +    properties:
> +      memory-region: false
> +      linux,sml-base: false
> +      linux,sml-size: false

Could you add "linux,sml-size: false" to "memory-region" as well
while at it for consistency?

Thanks,

Lukas

Re: [RFC PATCH v2 2/3] dt-bindings: tpm: Add linux,sml-log to ibm,vtpm.yaml

[Stefan Berger]

On 3/12/24 07:11, Lukas Wunner wrote:
> On Mon, Mar 11, 2024 at 09:20:29AM -0400, Stefan Berger wrote:
>> Add linux,sml-log, which carries the firmware TPM log in a uint8-array, to
>> the properties. Either this property is required or both linux,sml-base and
>> linux,sml-size are required. Add a test case for verification.
>>
>> Fixes: 82003e0487fb ("Documentation: tpm: add the IBM Virtual TPM device tree binding documentation")
> 
> The Fixes tag is confusing.  The patch won't even apply cleanly to the
> v4.10 commit referenced here as the conversion to yaml happened only
> recently with v6.8.

Then that's as far back (6.8) as the series may be applied. I put the 
Fixes tag on the first appearance of sml-base/sml-size since for kexec 
this was never correct.

> 
> Why is the Fixes tag necessary in the first place?  Same question for
> the other patches in the series.  This looks like feature work rather
> than a fix.  Not sure whether it satisfies the "obviously correct"
> rule per Documentation/process/stable-kernel-rules.rst.


It is a fix for the interaction of the TPM firmware log with kexec. The 
sml-base buffer pointer was never protected across a kexec.

> 
> 
>> --- a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
>> +++ b/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
>> @@ -74,8 +74,6 @@ required:
>>     - ibm,my-dma-window
>>     - ibm,my-drc-index
>>     - ibm,loc-code
>> -  - linux,sml-base
>> -  - linux,sml-size
> 
> I assume that either these two or the new "linux,sml-log" property
> are (still) required?  If so, a quick grep through the bindings
> (e.g. auxdisplay/img,ascii-lcd.yaml) shows that the following
> might work:
> 
> required:
>    - ...
> 
> oneOf:
>    - required:
>        - linux,sml-base
>    - required:
>        - linux,sml-log
> 
You're right, they need to be here since examples could now omit 
sml-base or sml-log. I added them. Thanks.

> 
>> --- a/Documentation/devicetree/bindings/tpm/tpm-common.yaml
>> +++ b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
>> @@ -30,6 +30,11 @@ properties:
>>         size of reserved memory allocated for firmware event log
>>       $ref: /schemas/types.yaml#/definitions/uint32
>>   
>> +  linux,sml-log:
>> +    description:
>> +      Content of firmware event log
> 
> Please add one or two sentences of context so that readers don't
> need to use git blame + git log to find out what this is for.
> (Mention at least that the property may be used to pass the log
> to a kexec kernel.)

Ok, will do:

Content of firmware event log embedded in device tree to be safely 
carried across a kexec soft reboot.



> 
> 
>> -# must only have either memory-region or linux,sml-base
>> +# must only have either memory-region or linux,sml-base/size or linux,sml-log
>>   # as well as either resets or reset-gpios
>>   dependentSchemas:
>>     memory-region:
>>       properties:
>>         linux,sml-base: false
>> +      linux,sml-log: false
>>     linux,sml-base:
>>       properties:
>>         memory-region: false
>> +      linux,sml-log: false
>> +  linux,sml-log:
>> +    properties:
>> +      memory-region: false
>> +      linux,sml-base: false
>> +      linux,sml-size: false
> 
> Could you add "linux,sml-size: false" to "memory-region" as well
> while at it for consistency?

Done. Thanks.

    Stefan
> 
> Thanks,
> 
> Lukas

Re: [RFC PATCH v2 3/3] tpm: of: If available use linux,sml-log to get the log and its size

[Jarkko Sakkinen]

On Mon Mar 11, 2024 at 10:33 PM EET, Stefan Berger wrote:
>
>
> On 3/11/24 16:25, Jarkko Sakkinen wrote:
> > On Mon Mar 11, 2024 at 3:20 PM EET, Stefan Berger wrote:
> >> If linux,sml-log is available use it to get the TPM log rather than the
> >> pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
> >> on Power where after a kexec the memory pointed to by linux,sml-base may
> >> have become inaccessible or corrupted. Also, linux,sml-log has replaced
> >> linux,sml-base and linux,sml-size on these two platforms.
> >>
> >> Keep the handling of linux,sml-base/sml-size for powernv platforms that
> >> provide the two properties via skiboot.
> >>
> >> Fixes: c5df39262dd5 ("drivers/char/tpm: Add securityfs support for event log")
> >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > 
> > I'm worried about not being up to date and instead using "cached" values
> > when verifying anything from a security chip. Does this guarantee that
> > TPM log is corrupted and will not get updated somehow?
>
>
> What do you mean 'guarantee that TPM log is corrupted'?

I presume that this is for power architecture but I have no idea what
leads log being corrupted, and is the scope all of that that arch or
some subset of CPUs.

The commit message is not very detailed on kexec scenario. It more like
assumes that reader knows all the detail beforehand. So probably this
will start to make sense once the backing story is improved, that's
all.

> The TPM was handed over from the firmware to Linux and the firmware then 
> freed all memory associated with the log and will then not create a new 
> log or touch the TPM or do anything that would require an update to the 
> handed-over log. Linux also does not append to the firmware log. So 
> whatever we now find in linux,sml-log would be the latest firmware log 
> and the state of the 'firmware PCRs' computed from it should correspond 
> to the current state of the 'firmware PCRs'.

So on what CPU this happens and is there any bigger picture for that
design choice in the firmware?

If it is a firmware bug, this should emit FW_BUG log message. If not,
then this commit message should provide the necessary context.

BR, Jarkko

Re: [RFC PATCH v2 2/3] dt-bindings: tpm: Add linux,sml-log to ibm,vtpm.yaml

[Jarkko Sakkinen]

On Tue Mar 12, 2024 at 1:11 PM EET, Lukas Wunner wrote:
> On Mon, Mar 11, 2024 at 09:20:29AM -0400, Stefan Berger wrote:
> > Add linux,sml-log, which carries the firmware TPM log in a uint8-array, to
> > the properties. Either this property is required or both linux,sml-base and
> > linux,sml-size are required. Add a test case for verification.
> > 
> > Fixes: 82003e0487fb ("Documentation: tpm: add the IBM Virtual TPM device tree binding documentation")
>
> The Fixes tag is confusing.  The patch won't even apply cleanly to the
> v4.10 commit referenced here as the conversion to yaml happened only
> recently with v6.8.
>
> Why is the Fixes tag necessary in the first place?  Same question for
> the other patches in the series.  This looks like feature work rather
> than a fix.  Not sure whether it satisfies the "obviously correct"
> rule per Documentation/process/stable-kernel-rules.rst.

I'm not yet sure whether these are bug fixes and or improvements because
I did not fully understand the scenario where TPM corrupts the event log
so that part reminds to be seen.

Probably once I fully understand what is going on, it is possible to
argue on that.

BR, Jarkko

Re: [RFC PATCH v2 3/3] tpm: of: If available use linux,sml-log to get the log and its size

[Stefan Berger]

On 3/12/24 11:43, Jarkko Sakkinen wrote:
> On Mon Mar 11, 2024 at 10:33 PM EET, Stefan Berger wrote:
>>
>>
>> On 3/11/24 16:25, Jarkko Sakkinen wrote:
>>> On Mon Mar 11, 2024 at 3:20 PM EET, Stefan Berger wrote:
>>>> If linux,sml-log is available use it to get the TPM log rather than the
>>>> pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
>>>> on Power where after a kexec the memory pointed to by linux,sml-base may
>>>> have become inaccessible or corrupted. Also, linux,sml-log has replaced
>>>> linux,sml-base and linux,sml-size on these two platforms.
>>>>
>>>> Keep the handling of linux,sml-base/sml-size for powernv platforms that
>>>> provide the two properties via skiboot.
>>>>
>>>> Fixes: c5df39262dd5 ("drivers/char/tpm: Add securityfs support for event log")
>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>
>>> I'm worried about not being up to date and instead using "cached" values
>>> when verifying anything from a security chip. Does this guarantee that
>>> TPM log is corrupted and will not get updated somehow?
>>
>>
>> What do you mean 'guarantee that TPM log is corrupted'?
> 
> I presume that this is for power architecture but I have no idea what

Yes it is for Power. From commit message above: "This resolves an issue 
on PowerVM and KVM on Power where after a kexec the memory pointed to by 
linux,sml-base may have become inaccessible or corrupted."

> leads log being corrupted, and is the scope all of that that arch or
> some subset of CPUs.

Every CPU will see a corrupted log.

> 
> The commit message is not very detailed on kexec scenario. It more like

I guess what is missing in the message that the buffer was not properly 
protected during the kexec and may have been overwritten for example 
since it was mistakenly assumed to be free memory?

> assumes that reader knows all the detail beforehand. So probably this
> will start to make sense once the backing story is improved, that's
> all.
> 
>> The TPM was handed over from the firmware to Linux and the firmware then
>> freed all memory associated with the log and will then not create a new
>> log or touch the TPM or do anything that would require an update to the
>> handed-over log. Linux also does not append to the firmware log. So
>> whatever we now find in linux,sml-log would be the latest firmware log
>> and the state of the 'firmware PCRs' computed from it should correspond
>> to the current state of the 'firmware PCRs'.
> 
> So on what CPU this happens and is there any bigger picture for that
> design choice in the firmware?

The firmware provides a call sml-handover, which hands over the TPM log 
to the caller and at the same time frees the log. You cannot call the 
firmware a 2nd time for the log.

> 
> If it is a firmware bug, this should emit FW_BUG log message. If not,
> then this commit message should provide the necessary context.

It's not a firmware bug. The issue is that the buffer holding the TPM 
log is not properly carried across a kexec soft reboot and may for 
example have been overwritten since it was assumed to be free memory.

> 
> BR, Jarkko
>