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

[Nayna <nayna@xxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxx # v6.3
>
> ?

Aah yes. Missed that.. Thanks..


> > Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
> > Signed-off-by: Nayna Jain <nayna@xxxxxxxxxxxxx>
> > ---
> >   .../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