[PATCH v2] security/integrity: fix pointer to ESL data and its size on pseries

[PATCH v2] security/integrity: fix pointer to ESL data and its size on pseries

[Nayna Jain]

On PowerVM guest, variable data is prefixed with 8 bytes of timestamp.
Extract ESL by stripping off the timestamp before passing to ESL parser.

Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
Cc: stable@vger.kenrnel.org # v6.3
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
Changelog:
v2: Fixed feedback from Jarkko
      * added CC to stable
      * moved *data declaration to same line as *db,*dbx
    Renamed extract_data() macro to extract_esl() for clarity
 .../integrity/platform_certs/load_powerpc.c   | 40 ++++++++++++-------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
index b9de70b90826..170789dc63d2 100644
--- a/security/integrity/platform_certs/load_powerpc.c
+++ b/security/integrity/platform_certs/load_powerpc.c
@@ -15,6 +15,9 @@
 #include "keyring_handler.h"
 #include "../integrity.h"
 
+#define extract_esl(db, data, size, offset)	\
+	do { db = data + offset; size = size - offset; } while (0)
+
 /*
  * Get a certificate list blob from the named secure variable.
  *
@@ -55,8 +58,9 @@ static __init void *get_cert_list(u8 *key, unsigned long keylen, u64 *size)
  */
 static int __init load_powerpc_certs(void)
 {
-	void *db = NULL, *dbx = NULL;
-	u64 dbsize = 0, dbxsize = 0;
+	void *db = NULL, *dbx = NULL, *data = NULL;
+	u64 dsize = 0;
+	u64 offset = 0;
 	int rc = 0;
 	ssize_t len;
 	char buf[32];
@@ -74,38 +78,46 @@ static int __init load_powerpc_certs(void)
 		return -ENODEV;
 	}
 
+	if (strcmp("ibm,plpks-sb-v1", buf) == 0)
+		/* PLPKS authenticated variables ESL data is prefixed with 8 bytes of timestamp */
+		offset = 8;
+
 	/*
 	 * Get db, and dbx. They might not exist, so it isn't an error if we
 	 * can't get them.
 	 */
-	db = get_cert_list("db", 3, &dbsize);
-	if (!db) {
+	data = get_cert_list("db", 3, &dsize);
+	if (!data) {
 		pr_info("Couldn't get db list from firmware\n");
-	} else if (IS_ERR(db)) {
-		rc = PTR_ERR(db);
+	} else if (IS_ERR(data)) {
+		rc = PTR_ERR(data);
 		pr_err("Error reading db from firmware: %d\n", rc);
 		return rc;
 	} else {
-		rc = parse_efi_signature_list("powerpc:db", db, dbsize,
+		extract_esl(db, data, dsize, offset);
+
+		rc = parse_efi_signature_list("powerpc:db", db, dsize,
 					      get_handler_for_db);
 		if (rc)
 			pr_err("Couldn't parse db signatures: %d\n", rc);
-		kfree(db);
+		kfree(data);
 	}
 
-	dbx = get_cert_list("dbx", 4,  &dbxsize);
-	if (!dbx) {
+	data = get_cert_list("dbx", 4,  &dsize);
+	if (!data) {
 		pr_info("Couldn't get dbx list from firmware\n");
-	} else if (IS_ERR(dbx)) {
-		rc = PTR_ERR(dbx);
+	} else if (IS_ERR(data)) {
+		rc = PTR_ERR(data);
 		pr_err("Error reading dbx from firmware: %d\n", rc);
 		return rc;
 	} else {
-		rc = parse_efi_signature_list("powerpc:dbx", dbx, dbxsize,
+		extract_esl(dbx, data, dsize, offset);
+
+		rc = parse_efi_signature_list("powerpc:dbx", dbx, dsize,
 					      get_handler_for_dbx);
 		if (rc)
 			pr_err("Couldn't parse dbx signatures: %d\n", rc);
-		kfree(dbx);
+		kfree(data);
 	}
 
 	return rc;
-- 
2.31.1

Re: [PATCH v2] security/integrity: fix pointer to ESL data and its size on pseries

[Jarkko Sakkinen]

On Thu Jun 8, 2023 at 3:04 PM EEST, Nayna Jain wrote:
> On PowerVM guest, variable data is prefixed with 8 bytes of timestamp.
> Extract ESL by stripping off the timestamp before passing to ESL parser.
>
> Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
> Cc: stable@vger.kenrnel.org # v6.3
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> ---
> Changelog:
> v2: Fixed feedback from Jarkko
>       * added CC to stable
>       * moved *data declaration to same line as *db,*dbx
>     Renamed extract_data() macro to extract_esl() for clarity
>  .../integrity/platform_certs/load_powerpc.c   | 40 ++++++++++++-------
>  1 file changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
> index b9de70b90826..170789dc63d2 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -15,6 +15,9 @@
>  #include "keyring_handler.h"
>  #include "../integrity.h"
>  
> +#define extract_esl(db, data, size, offset)	\
> +	do { db = data + offset; size = size - offset; } while (0)
> +
>  /*
>   * Get a certificate list blob from the named secure variable.
>   *
> @@ -55,8 +58,9 @@ static __init void *get_cert_list(u8 *key, unsigned long keylen, u64 *size)
>   */
>  static int __init load_powerpc_certs(void)
>  {
> -	void *db = NULL, *dbx = NULL;
> -	u64 dbsize = 0, dbxsize = 0;
> +	void *db = NULL, *dbx = NULL, *data = NULL;
> +	u64 dsize = 0;
> +	u64 offset = 0;
>  	int rc = 0;
>  	ssize_t len;
>  	char buf[32];
> @@ -74,38 +78,46 @@ static int __init load_powerpc_certs(void)
>  		return -ENODEV;
>  	}
>  
> +	if (strcmp("ibm,plpks-sb-v1", buf) == 0)
> +		/* PLPKS authenticated variables ESL data is prefixed with 8 bytes of timestamp */
> +		offset = 8;
> +
>  	/*
>  	 * Get db, and dbx. They might not exist, so it isn't an error if we
>  	 * can't get them.
>  	 */
> -	db = get_cert_list("db", 3, &dbsize);
> -	if (!db) {
> +	data = get_cert_list("db", 3, &dsize);
> +	if (!data) {
>  		pr_info("Couldn't get db list from firmware\n");
> -	} else if (IS_ERR(db)) {
> -		rc = PTR_ERR(db);
> +	} else if (IS_ERR(data)) {
> +		rc = PTR_ERR(data);
>  		pr_err("Error reading db from firmware: %d\n", rc);
>  		return rc;
>  	} else {
> -		rc = parse_efi_signature_list("powerpc:db", db, dbsize,
> +		extract_esl(db, data, dsize, offset);
> +
> +		rc = parse_efi_signature_list("powerpc:db", db, dsize,
>  					      get_handler_for_db);
>  		if (rc)
>  			pr_err("Couldn't parse db signatures: %d\n", rc);
> -		kfree(db);
> +		kfree(data);
>  	}
>  
> -	dbx = get_cert_list("dbx", 4,  &dbxsize);
> -	if (!dbx) {
> +	data = get_cert_list("dbx", 4,  &dsize);
> +	if (!data) {
>  		pr_info("Couldn't get dbx list from firmware\n");
> -	} else if (IS_ERR(dbx)) {
> -		rc = PTR_ERR(dbx);
> +	} else if (IS_ERR(data)) {
> +		rc = PTR_ERR(data);
>  		pr_err("Error reading dbx from firmware: %d\n", rc);
>  		return rc;
>  	} else {
> -		rc = parse_efi_signature_list("powerpc:dbx", dbx, dbxsize,
> +		extract_esl(dbx, data, dsize, offset);
> +
> +		rc = parse_efi_signature_list("powerpc:dbx", dbx, dsize,
>  					      get_handler_for_dbx);
>  		if (rc)
>  			pr_err("Couldn't parse dbx signatures: %d\n", rc);
> -		kfree(dbx);
> +		kfree(data);
>  	}
>  
>  	return rc;
> -- 
> 2.31.1

Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [PATCH v2] security/integrity: fix pointer to ESL data and its size on pseries

[R Nageswara Sastry]

On 08/06/23 5:34 pm, Nayna Jain wrote:
> On PowerVM guest, variable data is prefixed with 8 bytes of timestamp.
> Extract ESL by stripping off the timestamp before passing to ESL parser.
> 
> Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
> Cc: stable@vger.kenrnel.org # v6.3
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> ---

Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>

Seeing the keyring difference with and with out patch:
With out patch:
[root@ltcrain80-lp2 ~]# grep keyring /proc/keys | grep -v _ses
0131ebb4 I--Q---     1 perm 0c030000     0 65534 keyring   .user_reg: 2
039dfd93 I------     1 perm 1f0f0000     0     0 keyring   .ima: 1
03a160b5 I------     1 perm 1f0b0000     0     0 keyring 
.builtin_trusted_keys: 2
05377b73 I------     1 perm 1f0b0000     0     0 keyring   .platform: empty
0d7ea730 I------     1 perm 0f0b0000     0     0 keyring   .blacklist: empty
16235f2d I--Q---     6 perm 1f3f0000     0 65534 keyring   _uid.0: empty
1721f130 I------     1 perm 1f0f0000     0     0 keyring   .evm: empty

With patch:
[root@ltcrain80-lp2 ~]# grep keyring /proc/keys | grep -v _ses
04820159 I------     1 perm 0f0b0000     0     0 keyring   .blacklist: 1
16d05827 I--Q---     1 perm 0c030000     0 65534 keyring   .user_reg: 2
17648d6a I------     1 perm 1f0b0000     0     0 keyring 
.builtin_trusted_keys: 2
2158b34f I--Q---     6 perm 1f3f0000     0 65534 keyring   _uid.0: empty
2237eff6 I------     1 perm 1f0f0000     0     0 keyring   .evm: empty
26d0330c I------     1 perm 1f0b0000     0     0 keyring   .platform: 1
2daa48ab I------     1 perm 1f0f0000     0     0 keyring   .ima: 1

Thank you.
> Changelog:
> v2: Fixed feedback from Jarkko
>        * added CC to stable
>        * moved *data declaration to same line as *db,*dbx
>      Renamed extract_data() macro to extract_esl() for clarity
>   .../integrity/platform_certs/load_powerpc.c   | 40 ++++++++++++-------
>   1 file changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
> index b9de70b90826..170789dc63d2 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -15,6 +15,9 @@
>   #include "keyring_handler.h"
>   #include "../integrity.h"
>   
> +#define extract_esl(db, data, size, offset)	\
> +	do { db = data + offset; size = size - offset; } while (0)
> +
>   /*
>    * Get a certificate list blob from the named secure variable.
>    *
> @@ -55,8 +58,9 @@ static __init void *get_cert_list(u8 *key, unsigned long keylen, u64 *size)
>    */
>   static int __init load_powerpc_certs(void)
>   {
> -	void *db = NULL, *dbx = NULL;
> -	u64 dbsize = 0, dbxsize = 0;
> +	void *db = NULL, *dbx = NULL, *data = NULL;
> +	u64 dsize = 0;
> +	u64 offset = 0;
>   	int rc = 0;
>   	ssize_t len;
>   	char buf[32];
> @@ -74,38 +78,46 @@ static int __init load_powerpc_certs(void)
>   		return -ENODEV;
>   	}
>   
> +	if (strcmp("ibm,plpks-sb-v1", buf) == 0)
> +		/* PLPKS authenticated variables ESL data is prefixed with 8 bytes of timestamp */
> +		offset = 8;
> +
>   	/*
>   	 * Get db, and dbx. They might not exist, so it isn't an error if we
>   	 * can't get them.
>   	 */
> -	db = get_cert_list("db", 3, &dbsize);
> -	if (!db) {
> +	data = get_cert_list("db", 3, &dsize);
> +	if (!data) {
>   		pr_info("Couldn't get db list from firmware\n");
> -	} else if (IS_ERR(db)) {
> -		rc = PTR_ERR(db);
> +	} else if (IS_ERR(data)) {
> +		rc = PTR_ERR(data);
>   		pr_err("Error reading db from firmware: %d\n", rc);
>   		return rc;
>   	} else {
> -		rc = parse_efi_signature_list("powerpc:db", db, dbsize,
> +		extract_esl(db, data, dsize, offset);
> +
> +		rc = parse_efi_signature_list("powerpc:db", db, dsize,
>   					      get_handler_for_db);
>   		if (rc)
>   			pr_err("Couldn't parse db signatures: %d\n", rc);
> -		kfree(db);
> +		kfree(data);
>   	}
>   
> -	dbx = get_cert_list("dbx", 4,  &dbxsize);
> -	if (!dbx) {
> +	data = get_cert_list("dbx", 4,  &dsize);
> +	if (!data) {
>   		pr_info("Couldn't get dbx list from firmware\n");
> -	} else if (IS_ERR(dbx)) {
> -		rc = PTR_ERR(dbx);
> +	} else if (IS_ERR(data)) {
> +		rc = PTR_ERR(data);
>   		pr_err("Error reading dbx from firmware: %d\n", rc);
>   		return rc;
>   	} else {
> -		rc = parse_efi_signature_list("powerpc:dbx", dbx, dbxsize,
> +		extract_esl(dbx, data, dsize, offset);
> +
> +		rc = parse_efi_signature_list("powerpc:dbx", dbx, dsize,
>   					      get_handler_for_dbx);
>   		if (rc)
>   			pr_err("Couldn't parse dbx signatures: %d\n", rc);
> -		kfree(dbx);
> +		kfree(data);
>   	}
>   
>   	return rc;

-- 
Thanks and Regards
R.Nageswara Sastry

Re: [PATCH v2] security/integrity: fix pointer to ESL data and its size on pseries

[Michael Ellerman]

On Thu, 08 Jun 2023 08:04:44 -0400, Nayna Jain wrote:
> On PowerVM guest, variable data is prefixed with 8 bytes of timestamp.
> Extract ESL by stripping off the timestamp before passing to ESL parser.
> 
> 

Applied to powerpc/next.

[1/1] security/integrity: fix pointer to ESL data and its size on pseries
      https://git.kernel.org/powerpc/c/e66effaf61ffb1dc6088492ca3a0e98dcbf1c10d

cheers