diff mbox series

[3/4] efi_loader: simplify efi_sigstore_parse_sigdb()

Message ID 20210911072832.16991-4-xypron.glpk@gmx.de
State Superseded, archived
Delegated to: Heinrich Schuchardt
Headers show
Series efi_loader: centralize known vendor GUIDs | expand

Commit Message

Heinrich Schuchardt Sept. 11, 2021, 7:28 a.m. UTC 
Simplify efi_sigstore_parse_sigdb() by using existing functions.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_signature.c | 35 ++++++----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

--
2.30.2

Comments

Ilias Apalodimas Sept. 11, 2021, 2:25 p.m. UTC | #1 
On Sat, Sep 11, 2021 at 09:28:31AM +0200, Heinrich Schuchardt wrote:
> Simplify efi_sigstore_parse_sigdb() by using existing functions.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_signature.c | 35 ++++++----------------------------
>  1 file changed, 6 insertions(+), 29 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> index bdd09881fc..b741905a99 100644
> --- a/lib/efi_loader/efi_signature.c
> +++ b/lib/efi_loader/efi_signature.c
> @@ -7,6 +7,7 @@
>  #include <common.h>
>  #include <charset.h>
>  #include <efi_loader.h>
> +#include <efi_variable.h>
>  #include <image.h>
>  #include <hexdump.h>
>  #include <malloc.h>
> @@ -740,44 +741,20 @@ err:
>   */
>  struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
>  {
> -	struct efi_signature_store *sigstore = NULL;
>  	const efi_guid_t *vendor;
>  	void *db;
>  	efi_uintn_t db_size;
> -	efi_status_t ret;
> 
> -	if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
> -		vendor = &efi_global_variable_guid;
> -	} else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
> -		vendor = &efi_guid_image_security_database;
> -	} else {
> +	vendor = efi_auth_var_get_guid(name);
> +	if (!vendor) {
>  		EFI_PRINT("unknown signature database, %ls\n", name);
>  		return NULL;
>  	}

efi_auth_var_get_guid() will return &efi_global_variable_guid if the
GUID for the variable name isn't found.

> 
> -	/* retrieve variable data */
> -	db_size = 0;
> -	ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
> -	if (ret == EFI_NOT_FOUND) {
> -		EFI_PRINT("variable, %ls, not found\n", name);
> -		sigstore = calloc(sizeof(*sigstore), 1);
> -		return sigstore;
> -	} else if (ret != EFI_BUFFER_TOO_SMALL) {
> -		EFI_PRINT("Getting variable, %ls, failed\n", name);
> -		return NULL;
> -	}
> -
> -	db = malloc(db_size);
> +	db = efi_get_var(name, vendor, &db_size);
>  	if (!db) {
> -		EFI_PRINT("Out of memory\n");
> -		return NULL;
> -	}
> -
> -	ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, db));
> -	if (ret != EFI_SUCCESS) {
> -		EFI_PRINT("Getting variable, %ls, failed\n", name);
> -		free(db);
> -		return NULL;
> +		EFI_PRINT("variable, %ls, not found\n", name);
> +		return calloc(sizeof(struct efi_signature_store), 1);
>  	}
> 
>  	return efi_build_signature_store(db, db_size);
> --
> 2.30.2
>
Heinrich Schuchardt Sept. 12, 2021, 7:16 p.m. UTC | #2 
On 9/11/21 4:25 PM, Ilias Apalodimas wrote:
> On Sat, Sep 11, 2021 at 09:28:31AM +0200, Heinrich Schuchardt wrote:
>> Simplify efi_sigstore_parse_sigdb() by using existing functions.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   lib/efi_loader/efi_signature.c | 35 ++++++----------------------------
>>   1 file changed, 6 insertions(+), 29 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
>> index bdd09881fc..b741905a99 100644
>> --- a/lib/efi_loader/efi_signature.c
>> +++ b/lib/efi_loader/efi_signature.c
>> @@ -7,6 +7,7 @@
>>   #include <common.h>
>>   #include <charset.h>
>>   #include <efi_loader.h>
>> +#include <efi_variable.h>
>>   #include <image.h>
>>   #include <hexdump.h>
>>   #include <malloc.h>
>> @@ -740,44 +741,20 @@ err:
>>    */
>>   struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
>>   {
>> -	struct efi_signature_store *sigstore = NULL;
>>   	const efi_guid_t *vendor;
>>   	void *db;
>>   	efi_uintn_t db_size;
>> -	efi_status_t ret;
>>
>> -	if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
>> -		vendor = &efi_global_variable_guid;
>> -	} else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
>> -		vendor = &efi_guid_image_security_database;
>> -	} else {
>> +	vendor = efi_auth_var_get_guid(name);
>> +	if (!vendor) {
>>   		EFI_PRINT("unknown signature database, %ls\n", name);
>>   		return NULL;
>>   	}
>
> efi_auth_var_get_guid() will return &efi_global_variable_guid if the
> GUID for the variable name isn't found.

Hello Ilias, that is on purpose. In nevedit_efi we need a default GUID.
I want to reuse the same function there in future.

Best regards

Heinrich

>
>>
>> -	/* retrieve variable data */
>> -	db_size = 0;
>> -	ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
>> -	if (ret == EFI_NOT_FOUND) {
>> -		EFI_PRINT("variable, %ls, not found\n", name);
>> -		sigstore = calloc(sizeof(*sigstore), 1);
>> -		return sigstore;
>> -	} else if (ret != EFI_BUFFER_TOO_SMALL) {
>> -		EFI_PRINT("Getting variable, %ls, failed\n", name);
>> -		return NULL;
>> -	}
>> -
>> -	db = malloc(db_size);
>> +	db = efi_get_var(name, vendor, &db_size);
>>   	if (!db) {
>> -		EFI_PRINT("Out of memory\n");
>> -		return NULL;
>> -	}
>> -
>> -	ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, db));
>> -	if (ret != EFI_SUCCESS) {
>> -		EFI_PRINT("Getting variable, %ls, failed\n", name);
>> -		free(db);
>> -		return NULL;
>> +		EFI_PRINT("variable, %ls, not found\n", name);
>> +		return calloc(sizeof(struct efi_signature_store), 1);
>>   	}
>>
>>   	return efi_build_signature_store(db, db_size);
>> --
>> 2.30.2
>>
Ilias Apalodimas Sept. 12, 2021, 7:23 p.m. UTC | #3 
Hi Heinrich

[...]
> >> -    if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
> >> -            vendor = &efi_global_variable_guid;
> >> -    } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
> >> -            vendor = &efi_guid_image_security_database;
> >> -    } else {
> >> +    vendor = efi_auth_var_get_guid(name);
> >> +    if (!vendor) {
> >>              EFI_PRINT("unknown signature database, %ls\n", name);
> >>              return NULL;
> >>      }
> >
> > efi_auth_var_get_guid() will return &efi_global_variable_guid if the
> > GUID for the variable name isn't found.
>
> Hello Ilias, that is on purpose. In nevedit_efi we need a default GUID.
> I want to reuse the same function there in future.
>
> Best regards

Then I guess the check can go away ?

>
> Heinrich
>
> >
> >>
> >> -    /* retrieve variable data */
> >> -    db_size = 0;
> >> -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
> >> -    if (ret == EFI_NOT_FOUND) {
> >> -            EFI_PRINT("variable, %ls, not found\n", name);
> >> -            sigstore = calloc(sizeof(*sigstore), 1);
> >> -            return sigstore;
> >> -    } else if (ret != EFI_BUFFER_TOO_SMALL) {
> >> -            EFI_PRINT("Getting variable, %ls, failed\n", name);
> >> -            return NULL;
> >> -    }
> >> -
> >> -    db = malloc(db_size);
> >> +    db = efi_get_var(name, vendor, &db_size);
> >>      if (!db) {
> >> -            EFI_PRINT("Out of memory\n");
> >> -            return NULL;
> >> -    }
> >> -
> >> -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, db));
> >> -    if (ret != EFI_SUCCESS) {
> >> -            EFI_PRINT("Getting variable, %ls, failed\n", name);
> >> -            free(db);
> >> -            return NULL;
> >> +            EFI_PRINT("variable, %ls, not found\n", name);
> >> +            return calloc(sizeof(struct efi_signature_store), 1);

Why? From the patch alone it's not clear why you want to allocate
memory here instead of returning NULL.

> >>      }
> >>
> >>      return efi_build_signature_store(db, db_size);
> >> --
> >> 2.30.2
> >>

Cheers
/Ilias
Heinrich Schuchardt Oct. 1, 2021, 4:42 p.m. UTC | #4 
On 9/12/21 21:23, Ilias Apalodimas wrote:
> Hi Heinrich
>
> [...]
>>>> -    if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
>>>> -            vendor = &efi_global_variable_guid;
>>>> -    } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
>>>> -            vendor = &efi_guid_image_security_database;
>>>> -    } else {
>>>> +    vendor = efi_auth_var_get_guid(name);
>>>> +    if (!vendor) {
>>>>               EFI_PRINT("unknown signature database, %ls\n", name);
>>>>               return NULL;
>>>>       }
>>>
>>> efi_auth_var_get_guid() will return &efi_global_variable_guid if the
>>> GUID for the variable name isn't found.
>>
>> Hello Ilias, that is on purpose. In nevedit_efi we need a default GUID.
>> I want to reuse the same function there in future.
>>
>> Best regards
>
> Then I guess the check can go away ?

Yes

>
>>
>> Heinrich
>>
>>>
>>>>
>>>> -    /* retrieve variable data */
>>>> -    db_size = 0;
>>>> -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
>>>> -    if (ret == EFI_NOT_FOUND) {
>>>> -            EFI_PRINT("variable, %ls, not found\n", name);
>>>> -            sigstore = calloc(sizeof(*sigstore), 1);
>>>> -            return sigstore;
>>>> -    } else if (ret != EFI_BUFFER_TOO_SMALL) {
>>>> -            EFI_PRINT("Getting variable, %ls, failed\n", name);
>>>> -            return NULL;
>>>> -    }
>>>> -
>>>> -    db = malloc(db_size);
>>>> +    db = efi_get_var(name, vendor, &db_size);
>>>>       if (!db) {
>>>> -            EFI_PRINT("Out of memory\n");
>>>> -            return NULL;
>>>> -    }
>>>> -
>>>> -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, db));
>>>> -    if (ret != EFI_SUCCESS) {
>>>> -            EFI_PRINT("Getting variable, %ls, failed\n", name);
>>>> -            free(db);
>>>> -            return NULL;
>>>> +            EFI_PRINT("variable, %ls, not found\n", name);
>>>> +            return calloc(sizeof(struct efi_signature_store), 1);
>
> Why? From the patch alone it's not clear why you want to allocate
> memory here instead of returning NULL.

This is existing code. See the same lines deleted above.

Best regards

Heinrich

>
>>>>       }
>>>>
>>>>       return efi_build_signature_store(db, db_size);
>>>> --
>>>> 2.30.2
>>>>
>
> Cheers
> /Ilias
>
Ilias Apalodimas Oct. 1, 2021, 7:08 p.m. UTC | #5 
Hi Heinrich,

On Fri, Oct 01, 2021 at 06:42:14PM +0200, Heinrich Schuchardt wrote:
> 
> 
> On 9/12/21 21:23, Ilias Apalodimas wrote:
> > Hi Heinrich
> > 
> > [...]
> > > > > -    if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
> > > > > -            vendor = &efi_global_variable_guid;
> > > > > -    } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
> > > > > -            vendor = &efi_guid_image_security_database;
> > > > > -    } else {
> > > > > +    vendor = efi_auth_var_get_guid(name);
> > > > > +    if (!vendor) {
> > > > >               EFI_PRINT("unknown signature database, %ls\n", name);
> > > > >               return NULL;
> > > > >       }
> > > > 
> > > > efi_auth_var_get_guid() will return &efi_global_variable_guid if the
> > > > GUID for the variable name isn't found.
> > > 
> > > Hello Ilias, that is on purpose. In nevedit_efi we need a default GUID.
> > > I want to reuse the same function there in future.
> > > 
> > > Best regards
> > 
> > Then I guess the check can go away ?
> 
> Yes
> 
> > 
> > > 
> > > Heinrich
> > > 
> > > > 
> > > > > 
> > > > > -    /* retrieve variable data */
> > > > > -    db_size = 0;
> > > > > -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
> > > > > -    if (ret == EFI_NOT_FOUND) {
> > > > > -            EFI_PRINT("variable, %ls, not found\n", name);
> > > > > -            sigstore = calloc(sizeof(*sigstore), 1);
> > > > > -            return sigstore;
> > > > > -    } else if (ret != EFI_BUFFER_TOO_SMALL) {
> > > > > -            EFI_PRINT("Getting variable, %ls, failed\n", name);
> > > > > -            return NULL;
> > > > > -    }
> > > > > -
> > > > > -    db = malloc(db_size);
> > > > > +    db = efi_get_var(name, vendor, &db_size);
> > > > >       if (!db) {
> > > > > -            EFI_PRINT("Out of memory\n");
> > > > > -            return NULL;
> > > > > -    }
> > > > > -
> > > > > -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, db));
> > > > > -    if (ret != EFI_SUCCESS) {
> > > > > -            EFI_PRINT("Getting variable, %ls, failed\n", name);
> > > > > -            free(db);
> > > > > -            return NULL;
> > > > > +            EFI_PRINT("variable, %ls, not found\n", name);
> > > > > +            return calloc(sizeof(struct efi_signature_store), 1);
> > 
> > Why? From the patch alone it's not clear why you want to allocate
> > memory here instead of returning NULL.
> 
> This is existing code. See the same lines deleted above.

If I read the code correctly,  we are trying to be smart about the buffer
outcome.  Check for example efi_image_unsigned_authenticate().  By returning 
an empty buffer the 'dbx' check will succeed but the 'db' check a few lines 
after will fail.

But this is pointless imho... Why don't we just have 
efi_status_t efi_signature_store efi_sigstore_parse_sigdb(u16 *name, struct
														  efi_signature_store *store)

We can the control the EFI return value in efi_sigstore_parse_sigdb() and
any callers would just have to look at the result, instead of getting a
memory that contains empty data and try to reason about it.
IOW you can check for EFI_NOT_FOUND in both cases on the caller function.
If you are working with 'dbx' then that's fine and you can continue.  If
you are working with 'db' you need to fail the authentication.  This imho
is much more readable.

Regards
/Ilias
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index bdd09881fc..b741905a99 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -7,6 +7,7 @@ 
 #include <common.h>
 #include <charset.h>
 #include <efi_loader.h>
+#include <efi_variable.h>
 #include <image.h>
 #include <hexdump.h>
 #include <malloc.h>
@@ -740,44 +741,20 @@  err:
  */
 struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
 {
-	struct efi_signature_store *sigstore = NULL;
 	const efi_guid_t *vendor;
 	void *db;
 	efi_uintn_t db_size;
-	efi_status_t ret;

-	if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
-		vendor = &efi_global_variable_guid;
-	} else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
-		vendor = &efi_guid_image_security_database;
-	} else {
+	vendor = efi_auth_var_get_guid(name);
+	if (!vendor) {
 		EFI_PRINT("unknown signature database, %ls\n", name);
 		return NULL;
 	}

-	/* retrieve variable data */
-	db_size = 0;
-	ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
-	if (ret == EFI_NOT_FOUND) {
-		EFI_PRINT("variable, %ls, not found\n", name);
-		sigstore = calloc(sizeof(*sigstore), 1);
-		return sigstore;
-	} else if (ret != EFI_BUFFER_TOO_SMALL) {
-		EFI_PRINT("Getting variable, %ls, failed\n", name);
-		return NULL;
-	}
-
-	db = malloc(db_size);
+	db = efi_get_var(name, vendor, &db_size);
 	if (!db) {
-		EFI_PRINT("Out of memory\n");
-		return NULL;
-	}
-
-	ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, db));
-	if (ret != EFI_SUCCESS) {
-		EFI_PRINT("Getting variable, %ls, failed\n", name);
-		free(db);
-		return NULL;
+		EFI_PRINT("variable, %ls, not found\n", name);
+		return calloc(sizeof(struct efi_signature_store), 1);
 	}

 	return efi_build_signature_store(db, db_size);