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

[Nayna <nayna@linux.vnet.ibm.com>]

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