Re: [PATCH] security/integrity: fix pointer to ESL data and its size on pseries
On Wed Jun 7, 2023 at 3:28 PM EEST, Nayna wrote:
>
> On 6/6/23 16:51, Jarkko Sakkinen wrote:
> > On Tue Jun 6, 2023 at 8:26 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.
> >>
> > Cc: sta...@vger.kenrnel.org # v6.3
> >
> > ?
>
> Aah yes. Missed that.. Thanks..
>
>
> >
> >> Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
> >> Signed-off-by: Nayna Jain
> >> ---
> >> .../integrity/platform_certs/load_powerpc.c | 39 ---
> >> 1 file changed, 26 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/security/integrity/platform_certs/load_powerpc.c
> >> b/security/integrity/platform_certs/load_powerpc.c
> >> index b9de70b90826..57768cbf1fd3 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_data(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,10 @@ static __init void *get_cert_list(u8 *key, unsigned
> >> long keylen, u64 *size)
> >>*/
> >> static int __init load_powerpc_certs(void)
> >> {
> >> + void *data = NULL;
> >> + u64 dsize = 0;
> >> + u64 offset = 0;
> >>void *db = NULL, *dbx = NULL;
> > So... what do you need db still for?
> >
> > If you meant to rename 'db' to 'data', then you should not do it, since
> > this is
> > a bug fix. It is zero gain, and a factor harder backport.
>
> In case of PowerVM guest, data points to timestamp + ESL. And then with
> offset of 8 bytes, db points to ESL.
>
> While db is used for parsing ESL, data is then later used to free
> (timestamp + ESL) memory.
>
> Hope it answers the question.
OK, cool. Only thing I have to add that it would be more consistent if
data was declared in the same line as db and dbx, given that they are
declared in the same line.
BR, Jarkko
Re: [PATCH] security/integrity: fix pointer to ESL data and its size on pseries
On 6/6/23 16:51, Jarkko Sakkinen wrote:
On Tue Jun 6, 2023 at 8:26 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.
Cc: sta...@vger.kenrnel.org # v6.3
?
Aah yes. Missed that.. Thanks..
Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
Signed-off-by: Nayna Jain
---
.../integrity/platform_certs/load_powerpc.c | 39 ---
1 file changed, 26 insertions(+), 13 deletions(-)
diff --git a/security/integrity/platform_certs/load_powerpc.c
b/security/integrity/platform_certs/load_powerpc.c
index b9de70b90826..57768cbf1fd3 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_data(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,10 @@ static __init void *get_cert_list(u8 *key, unsigned long
keylen, u64 *size)
*/
static int __init load_powerpc_certs(void)
{
+ void *data = NULL;
+ u64 dsize = 0;
+ u64 offset = 0;
void *db = NULL, *dbx = NULL;
So... what do you need db still for?
If you meant to rename 'db' to 'data', then you should not do it, since this is
a bug fix. It is zero gain, and a factor harder backport.
In case of PowerVM guest, data points to timestamp + ESL. And then with
offset of 8 bytes, db points to ESL.
While db is used for parsing ESL, data is then later used to free
(timestamp + ESL) memory.
Hope it answers the question.
Thanks & Regards,
- Nayna
- u64 dbsize = 0, dbxsize = 0;
int rc = 0;
ssize_t len;
char buf[32];
@@ -74,38 +79,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, );
- if (!db) {
+ data = get_cert_list("db", 3, );
+ 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_data(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, );
- if (!dbx) {
+ data = get_cert_list("dbx", 4, );
+ 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_data(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
BR, Jarkko
Re: [PATCH] security/integrity: fix pointer to ESL data and its size on pseries
On Tue Jun 6, 2023 at 8:26 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.
>
Cc: sta...@vger.kenrnel.org # v6.3
?
> Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
> Signed-off-by: Nayna Jain
> ---
> .../integrity/platform_certs/load_powerpc.c | 39 ---
> 1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> index b9de70b90826..57768cbf1fd3 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_data(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,10 @@ static __init void *get_cert_list(u8 *key, unsigned long
> keylen, u64 *size)
> */
> static int __init load_powerpc_certs(void)
> {
> + void *data = NULL;
> + u64 dsize = 0;
> + u64 offset = 0;
> void *db = NULL, *dbx = NULL;
So... what do you need db still for?
If you meant to rename 'db' to 'data', then you should not do it, since this is
a bug fix. It is zero gain, and a factor harder backport.
> - u64 dbsize = 0, dbxsize = 0;
> int rc = 0;
> ssize_t len;
> char buf[32];
> @@ -74,38 +79,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, );
> - if (!db) {
> + data = get_cert_list("db", 3, );
> + 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_data(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, );
> - if (!dbx) {
> + data = get_cert_list("dbx", 4, );
> + 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_data(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
BR, Jarkko
