diff mbox series

[v4,10/16] cmd: env: use appropriate guid for authenticated UEFI variable

Message ID 20191218004512.24939-11-takahiro.akashi@linaro.org
State Superseded
Delegated to: Heinrich Schuchardt
Headers show
Series efi_loader: add secure boot support | expand

Commit Message

AKASHI Takahiro Dec. 18, 2019, 12:45 a.m. UTC 
A signature database variable is associated with a specific guid.
For convenience, if user doesn't supply any guid info, "env set|print -e"
should complement it.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/nvedit_efi.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Comments

Heinrich Schuchardt Jan. 21, 2020, 7:13 a.m. UTC | #1 
On 12/18/19 1:45 AM, AKASHI Takahiro wrote:
> A signature database variable is associated with a specific guid.
> For convenience, if user doesn't supply any guid info, "env set|print -e"
> should complement it.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   cmd/nvedit_efi.c | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
> index 8ea0da01283f..579cf430593c 100644
> --- a/cmd/nvedit_efi.c
> +++ b/cmd/nvedit_efi.c
> @@ -41,6 +41,11 @@ static const struct {
>   } efi_guid_text[] = {
>   	/* signature database */
>   	{EFI_GLOBAL_VARIABLE_GUID, "EFI_GLOBAL_VARIABLE_GUID"},
> +	{EFI_IMAGE_SECURITY_DATABASE_GUID, "EFI_IMAGE_SECURITY_DATABASE_GUID"},
> +	/* certificate type */
> +	{EFI_CERT_SHA256_GUID, "EFI_CERT_SHA256_GUID"},
> +	{EFI_CERT_X509_GUID, "EFI_CERT_X509_GUID"},
> +	{EFI_CERT_TYPE_PKCS7_GUID, "EFI_CERT_TYPE_PKCS7_GUID"},
>   };
>
>   /* "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" */
> @@ -525,9 +530,9 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   			if (*ep != ',')
>   				return CMD_RET_USAGE;
>
> +			/* 0 should be allowed for delete */
>   			size = simple_strtoul(++ep, NULL, 16);
> -			if (!size)
> -				return CMD_RET_FAILURE;
> +
>   			value_on_memory = true;
>   		} else if (!strcmp(argv[0], "-v")) {
>   			verbose = true;
> @@ -539,8 +544,13 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   		return CMD_RET_USAGE;
>
>   	var_name = argv[0];
> -	if (default_guid)
> -		guid = efi_global_variable_guid;
> +	if (default_guid) {
> +		if (!strcmp(var_name, "db") || !strcmp(var_name, "dbx") ||
> +		    !strcmp(var_name, "dbt"))

Why is "dbr" missing?

I guess dbDefault, dbrDefault, dbxDefault, dbtDefault use
EFI_GLOBAL_VARIABLE?

Best regards

Heinrich

> +			guid = efi_guid_image_security_database;
> +		else
> +			guid = efi_global_variable_guid;
> +	}
>
>   	if (verbose) {
>   		printf("GUID: %s\n", efi_guid_to_str((const efi_guid_t *)
>
AKASHI Takahiro Jan. 22, 2020, 1:01 a.m. UTC | #2 
On Tue, Jan 21, 2020 at 08:13:06AM +0100, Heinrich Schuchardt wrote:
> On 12/18/19 1:45 AM, AKASHI Takahiro wrote:
> >A signature database variable is associated with a specific guid.
> >For convenience, if user doesn't supply any guid info, "env set|print -e"
> >should complement it.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  cmd/nvedit_efi.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> >diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
> >index 8ea0da01283f..579cf430593c 100644
> >--- a/cmd/nvedit_efi.c
> >+++ b/cmd/nvedit_efi.c
> >@@ -41,6 +41,11 @@ static const struct {
> >  } efi_guid_text[] = {
> >  	/* signature database */
> >  	{EFI_GLOBAL_VARIABLE_GUID, "EFI_GLOBAL_VARIABLE_GUID"},
> >+	{EFI_IMAGE_SECURITY_DATABASE_GUID, "EFI_IMAGE_SECURITY_DATABASE_GUID"},
> >+	/* certificate type */
> >+	{EFI_CERT_SHA256_GUID, "EFI_CERT_SHA256_GUID"},
> >+	{EFI_CERT_X509_GUID, "EFI_CERT_X509_GUID"},
> >+	{EFI_CERT_TYPE_PKCS7_GUID, "EFI_CERT_TYPE_PKCS7_GUID"},
> >  };
> >
> >  /* "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" */
> >@@ -525,9 +530,9 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  			if (*ep != ',')
> >  				return CMD_RET_USAGE;
> >
> >+			/* 0 should be allowed for delete */
> >  			size = simple_strtoul(++ep, NULL, 16);
> >-			if (!size)
> >-				return CMD_RET_FAILURE;
> >+
> >  			value_on_memory = true;
> >  		} else if (!strcmp(argv[0], "-v")) {
> >  			verbose = true;
> >@@ -539,8 +544,13 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  		return CMD_RET_USAGE;
> >
> >  	var_name = argv[0];
> >-	if (default_guid)
> >-		guid = efi_global_variable_guid;
> >+	if (default_guid) {
> >+		if (!strcmp(var_name, "db") || !strcmp(var_name, "dbx") ||
> >+		    !strcmp(var_name, "dbt"))
> 
> Why is "dbr" missing?

Because it is not yet supported and I have no plan to support it
in short term.

> I guess dbDefault, dbrDefault, dbxDefault, dbtDefault use
> EFI_GLOBAL_VARIABLE?

Yes.
I have a patch for supporting those *Default now, but will submit it
once my core secure boot patch is accepted.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >+			guid = efi_guid_image_security_database;
> >+		else
> >+			guid = efi_global_variable_guid;
> >+	}
> >
> >  	if (verbose) {
> >  		printf("GUID: %s\n", efi_guid_to_str((const efi_guid_t *)
> >
>
Heinrich Schuchardt Jan. 22, 2020, 6:38 a.m. UTC | #3 
On 1/22/20 2:01 AM, AKASHI Takahiro wrote:
> On Tue, Jan 21, 2020 at 08:13:06AM +0100, Heinrich Schuchardt wrote:
>> On 12/18/19 1:45 AM, AKASHI Takahiro wrote:
>>> A signature database variable is associated with a specific guid.
>>> For convenience, if user doesn't supply any guid info, "env set|print -e"
>>> should complement it.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>   cmd/nvedit_efi.c | 18 ++++++++++++++----
>>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
>>> index 8ea0da01283f..579cf430593c 100644
>>> --- a/cmd/nvedit_efi.c
>>> +++ b/cmd/nvedit_efi.c
>>> @@ -41,6 +41,11 @@ static const struct {
>>>   } efi_guid_text[] = {
>>>   	/* signature database */
>>>   	{EFI_GLOBAL_VARIABLE_GUID, "EFI_GLOBAL_VARIABLE_GUID"},
>>> +	{EFI_IMAGE_SECURITY_DATABASE_GUID, "EFI_IMAGE_SECURITY_DATABASE_GUID"},
>>> +	/* certificate type */
>>> +	{EFI_CERT_SHA256_GUID, "EFI_CERT_SHA256_GUID"},
>>> +	{EFI_CERT_X509_GUID, "EFI_CERT_X509_GUID"},
>>> +	{EFI_CERT_TYPE_PKCS7_GUID, "EFI_CERT_TYPE_PKCS7_GUID"},
>>>   };
>>>
>>>   /* "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" */
>>> @@ -525,9 +530,9 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>   			if (*ep != ',')
>>>   				return CMD_RET_USAGE;
>>>
>>> +			/* 0 should be allowed for delete */
>>>   			size = simple_strtoul(++ep, NULL, 16);
>>> -			if (!size)
>>> -				return CMD_RET_FAILURE;
>>> +
>>>   			value_on_memory = true;
>>>   		} else if (!strcmp(argv[0], "-v")) {
>>>   			verbose = true;
>>> @@ -539,8 +544,13 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>   		return CMD_RET_USAGE;
>>>
>>>   	var_name = argv[0];
>>> -	if (default_guid)
>>> -		guid = efi_global_variable_guid;
>>> +	if (default_guid) {
>>> +		if (!strcmp(var_name, "db") || !strcmp(var_name, "dbx") ||
>>> +		    !strcmp(var_name, "dbt"))
>>
>> Why is "dbr" missing?
>
> Because it is not yet supported and I have no plan to support it
> in short term.

This should not stop us from adding "dbr" here just to keep the setenv
command consistent even if the value of dbr will not yet be considered
in our secure boot implementation.

Best regards

Heinrich

>
>> I guess dbDefault, dbrDefault, dbxDefault, dbtDefault use
>> EFI_GLOBAL_VARIABLE?
>
> Yes.
> I have a patch for supporting those *Default now, but will submit it
> once my core secure boot patch is accepted.
>
> Thanks,
> -Takahiro Akashi
>
>> Best regards
>>
>> Heinrich
>>
>>> +			guid = efi_guid_image_security_database;
>>> +		else
>>> +			guid = efi_global_variable_guid;
>>> +	}
>>>
>>>   	if (verbose) {
>>>   		printf("GUID: %s\n", efi_guid_to_str((const efi_guid_t *)
>>>
>>
>
AKASHI Takahiro Jan. 22, 2020, 7:15 a.m. UTC | #4 
On Wed, Jan 22, 2020 at 07:38:06AM +0100, Heinrich Schuchardt wrote:
> On 1/22/20 2:01 AM, AKASHI Takahiro wrote:
> >On Tue, Jan 21, 2020 at 08:13:06AM +0100, Heinrich Schuchardt wrote:
> >>On 12/18/19 1:45 AM, AKASHI Takahiro wrote:
> >>>A signature database variable is associated with a specific guid.
> >>>For convenience, if user doesn't supply any guid info, "env set|print -e"
> >>>should complement it.
> >>>
> >>>Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>---
> >>>  cmd/nvedit_efi.c | 18 ++++++++++++++----
> >>>  1 file changed, 14 insertions(+), 4 deletions(-)
> >>>
> >>>diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
> >>>index 8ea0da01283f..579cf430593c 100644
> >>>--- a/cmd/nvedit_efi.c
> >>>+++ b/cmd/nvedit_efi.c
> >>>@@ -41,6 +41,11 @@ static const struct {
> >>>  } efi_guid_text[] = {
> >>>  	/* signature database */
> >>>  	{EFI_GLOBAL_VARIABLE_GUID, "EFI_GLOBAL_VARIABLE_GUID"},
> >>>+	{EFI_IMAGE_SECURITY_DATABASE_GUID, "EFI_IMAGE_SECURITY_DATABASE_GUID"},
> >>>+	/* certificate type */
> >>>+	{EFI_CERT_SHA256_GUID, "EFI_CERT_SHA256_GUID"},
> >>>+	{EFI_CERT_X509_GUID, "EFI_CERT_X509_GUID"},
> >>>+	{EFI_CERT_TYPE_PKCS7_GUID, "EFI_CERT_TYPE_PKCS7_GUID"},
> >>>  };
> >>>
> >>>  /* "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" */
> >>>@@ -525,9 +530,9 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>  			if (*ep != ',')
> >>>  				return CMD_RET_USAGE;
> >>>
> >>>+			/* 0 should be allowed for delete */
> >>>  			size = simple_strtoul(++ep, NULL, 16);
> >>>-			if (!size)
> >>>-				return CMD_RET_FAILURE;
> >>>+
> >>>  			value_on_memory = true;
> >>>  		} else if (!strcmp(argv[0], "-v")) {
> >>>  			verbose = true;
> >>>@@ -539,8 +544,13 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>  		return CMD_RET_USAGE;
> >>>
> >>>  	var_name = argv[0];
> >>>-	if (default_guid)
> >>>-		guid = efi_global_variable_guid;
> >>>+	if (default_guid) {
> >>>+		if (!strcmp(var_name, "db") || !strcmp(var_name, "dbx") ||
> >>>+		    !strcmp(var_name, "dbt"))
> >>
> >>Why is "dbr" missing?
> >
> >Because it is not yet supported and I have no plan to support it
> >in short term.
> 
> This should not stop us from adding "dbr" here just to keep the setenv
> command consistent even if the value of dbr will not yet be considered
> in our secure boot implementation.

Your comments are inconsistent from time to time.
Please remember when I submitted the patch "cmd: env: extend "env
[set|print] -e" to manage UEFI variables".
In my early versions, "-at" (TIME_BASED_AUTHENTICATED_WRITE_ACCESS)
was also supported. But you rejected it just because secure boot
was not merged yet at the moment.

So nak to your suggestion above.

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >
> >>I guess dbDefault, dbrDefault, dbxDefault, dbtDefault use
> >>EFI_GLOBAL_VARIABLE?
> >
> >Yes.
> >I have a patch for supporting those *Default now, but will submit it
> >once my core secure boot patch is accepted.
> >
> >Thanks,
> >-Takahiro Akashi
> >
> >>Best regards
> >>
> >>Heinrich
> >>
> >>>+			guid = efi_guid_image_security_database;
> >>>+		else
> >>>+			guid = efi_global_variable_guid;
> >>>+	}
> >>>
> >>>  	if (verbose) {
> >>>  		printf("GUID: %s\n", efi_guid_to_str((const efi_guid_t *)
> >>>
> >>
> >
>
diff mbox series

Patch

diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
index 8ea0da01283f..579cf430593c 100644
--- a/cmd/nvedit_efi.c
+++ b/cmd/nvedit_efi.c
@@ -41,6 +41,11 @@  static const struct {
 } efi_guid_text[] = {
 	/* signature database */
 	{EFI_GLOBAL_VARIABLE_GUID, "EFI_GLOBAL_VARIABLE_GUID"},
+	{EFI_IMAGE_SECURITY_DATABASE_GUID, "EFI_IMAGE_SECURITY_DATABASE_GUID"},
+	/* certificate type */
+	{EFI_CERT_SHA256_GUID, "EFI_CERT_SHA256_GUID"},
+	{EFI_CERT_X509_GUID, "EFI_CERT_X509_GUID"},
+	{EFI_CERT_TYPE_PKCS7_GUID, "EFI_CERT_TYPE_PKCS7_GUID"},
 };
 
 /* "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" */
@@ -525,9 +530,9 @@  int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			if (*ep != ',')
 				return CMD_RET_USAGE;
 
+			/* 0 should be allowed for delete */
 			size = simple_strtoul(++ep, NULL, 16);
-			if (!size)
-				return CMD_RET_FAILURE;
+
 			value_on_memory = true;
 		} else if (!strcmp(argv[0], "-v")) {
 			verbose = true;
@@ -539,8 +544,13 @@  int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return CMD_RET_USAGE;
 
 	var_name = argv[0];
-	if (default_guid)
-		guid = efi_global_variable_guid;
+	if (default_guid) {
+		if (!strcmp(var_name, "db") || !strcmp(var_name, "dbx") ||
+		    !strcmp(var_name, "dbt"))
+			guid = efi_guid_image_security_database;
+		else
+			guid = efi_global_variable_guid;
+	}
 
 	if (verbose) {
 		printf("GUID: %s\n", efi_guid_to_str((const efi_guid_t *)