* [PATCH] security/integrity: fix pointer to ESL data and its size on pseries
@ 2023-06-06 17:26 Nayna Jain
2023-06-06 20:51 ` Jarkko Sakkinen
0 siblings, 1 reply; 4+ messages in thread
From: Nayna Jain @ 2023-06-06 17:26 UTC (permalink / raw)
To: linuxppc-dev, linux-integrity
Cc: Andrew Donnellan, Michael Ellerman, Mimi Zohar, Jarkko Sakkinen,
Russell Currey, Nageswara R Sastry, George Wilson, 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")
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
.../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;
- 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, &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_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, &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_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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] security/integrity: fix pointer to ESL data and its size on pseries
2023-06-06 17:26 [PATCH] security/integrity: fix pointer to ESL data and its size on pseries Nayna Jain
@ 2023-06-06 20:51 ` Jarkko Sakkinen
2023-06-07 12:28 ` Nayna
0 siblings, 1 reply; 4+ messages in thread
From: Jarkko Sakkinen @ 2023-06-06 20:51 UTC (permalink / raw)
To: Nayna Jain, linuxppc-dev, linux-integrity
Cc: Andrew Donnellan, Michael Ellerman, Mimi Zohar, Russell Currey,
Nageswara R Sastry, George Wilson
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: stable@vger.kenrnel.org # v6.3
?
> Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> ---
> .../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, &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_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, &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_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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] security/integrity: fix pointer to ESL data and its size on pseries
2023-06-06 20:51 ` Jarkko Sakkinen
@ 2023-06-07 12:28 ` Nayna
2023-06-07 16:03 ` Jarkko Sakkinen
0 siblings, 1 reply; 4+ messages in thread
From: Nayna @ 2023-06-07 12:28 UTC (permalink / raw)
To: Jarkko Sakkinen, Nayna Jain, linuxppc-dev, linux-integrity
Cc: Andrew Donnellan, Michael Ellerman, Mimi Zohar, Russell Currey,
Nageswara R Sastry, George Wilson
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: stable@vger.kenrnel.org # v6.3
>
> ?
Aah yes. Missed that.. Thanks..
>
>> Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
>> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
>> ---
>> .../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, &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_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, &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_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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] security/integrity: fix pointer to ESL data and its size on pseries
2023-06-07 12:28 ` Nayna
@ 2023-06-07 16:03 ` Jarkko Sakkinen
0 siblings, 0 replies; 4+ messages in thread
From: Jarkko Sakkinen @ 2023-06-07 16:03 UTC (permalink / raw)
To: Nayna, Nayna Jain, linuxppc-dev, linux-integrity
Cc: Andrew Donnellan, Michael Ellerman, Mimi Zohar, Russell Currey,
Nageswara R Sastry, George Wilson
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: stable@vger.kenrnel.org # v6.3
> >
> > ?
>
> Aah yes. Missed that.. Thanks..
>
>
> >
> >> Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
> >> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> >> ---
> >> .../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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-07 16:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-06 17:26 [PATCH] security/integrity: fix pointer to ESL data and its size on pseries Nayna Jain
2023-06-06 20:51 ` Jarkko Sakkinen
2023-06-07 12:28 ` Nayna
2023-06-07 16:03 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).