| Message ID | 20240327140628.106143-1-o451686892@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | [RFC] efi_loader: Fix EFI_VARIABLE_APPEND_WRITE hash check | expand |
On 27.03.24 15:06, Weizhao Ouyang wrote: > According to UEFI v2.10 spec section 8.2.6, if a caller invokes the > SetVariables() service, it will produce a digest from hash(VariableName, > VendorGuid, Attributes, TimeStamp, DataNew_variable_content), then the > firmware that implements the SetVariable() service will compare the > digest with the result of applying the signer’s public key to the > signature. For EFI variable append write, efitools sign-efi-sig-list has > an option "-a" to add EFI_VARIABLE_APPEND_WRITE attr, and u-boot will > drop this attribute in efi_set_variable_int(). So if a caller uses > "sign-efi-sig-list -a" to create the authenticated variable, this append > write will fail in the u-boot due to "hash check failed". Currently U-Boot does not allow to use EFI_VARIABLE_APPEND_WRITE for any non-existent variable, signed or not. This does not match the EDK II behavior. There is a patch queued for this issue: [PATCH v2] efi_loader: fix append write behavior to non-existent variable https://lore.kernel.org/u-boot/20240402090950.3819705-1-kojima.masahisa@socionext.com/ Could you, please, retest with that patch. > > This patch resumes writing the EFI_VARIABLE_APPEND_WRITE attr to ensure > that the hash check is correct. And also update the "test_efi_secboot" > test case to compliance with the change. > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > --- > lib/efi_loader/efi_variable.c | 3 +-- > test/py/tests/test_efi_secboot/conftest.py | 10 ++++++++++ > test/py/tests/test_efi_secboot/test_authvar.py | 4 ++-- > test/py/tests/test_efi_secboot/test_signed.py | 10 +++++----- > 4 files changed, 18 insertions(+), 9 deletions(-) > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > index 40f7a0fb10..f41aa8f9ad 100644 > --- a/lib/efi_loader/efi_variable.c > +++ b/lib/efi_loader/efi_variable.c > @@ -259,7 +259,6 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, > /* check if a variable exists */ > var = efi_var_mem_find(vendor, variable_name, NULL); > append = !!(attributes & EFI_VARIABLE_APPEND_WRITE); > - attributes &= ~(u32)EFI_VARIABLE_APPEND_WRITE; According to the UEFI specification for GetVariable(): "The EFI_VARIABLE_APPEND_WRITE attribute will never be set in the returned Attributes bitmask parameter." We don't want to set EFI_VARIABLE_APPEND_WRITE in a variable when calling efi_var_mem_ins(). > delete = !append && (!data_size || !attributes); > > /* check attributes */ > @@ -275,7 +274,7 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, > > /* attributes won't be changed */ > if (!delete && > - ((ro_check && var->attr != attributes) || > + ((ro_check && var->attr != (attributes & ~(u32)EFI_VARIABLE_APPEND_WRITE)) || > (!ro_check && ((var->attr & ~(u32)EFI_VARIABLE_READ_ONLY) The (u32) conversions are superfluous. Best regards Heinrich > != (attributes & ~(u32)EFI_VARIABLE_READ_ONLY))))) { > return EFI_INVALID_PARAMETER; > diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py > index ff7ac7c810..0fa0747fc7 100644 > --- a/test/py/tests/test_efi_secboot/conftest.py > +++ b/test/py/tests/test_efi_secboot/conftest.py > @@ -64,6 +64,12 @@ def efi_boot_env(request, u_boot_config): > check_call('cd %s; %scert-to-efi-sig-list -g %s db1.crt db1.esl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key db db1.esl db1.auth' > % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > shell=True) > + # db2 (APPEND_WRITE) > + check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_db2/ -keyout db2.key -out db2.crt -nodes -days 365' > + % mnt_point, shell=True) > + check_call('cd %s; %scert-to-efi-sig-list -g %s db2.crt db2.esl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key db db2.esl db2.auth' > + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > + shell=True) > # dbx (TEST_dbx certificate) > check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_dbx/ -keyout dbx.key -out dbx.crt -nodes -days 365' > % mnt_point, shell=True) > @@ -84,6 +90,10 @@ def efi_boot_env(request, u_boot_config): > check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db1.crt dbx_hash1.crl; %ssign-efi-sig-list -t "2020-04-06" -c KEK.crt -k KEK.key dbx dbx_hash1.crl dbx_hash1.auth' > % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > shell=True) > + # dbx_hash2 (digest of TEST_db2 certificate, with APPEND_WRITE) > + check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db2.crt dbx_hash2.crl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key dbx dbx_hash2.crl dbx_hash2.auth' > + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > + shell=True) > # dbx_db (with TEST_db certificate) > check_call('cd %s; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx db.esl dbx_db.auth' > % (mnt_point, EFITOOLS_PATH), > diff --git a/test/py/tests/test_efi_secboot/test_authvar.py b/test/py/tests/test_efi_secboot/test_authvar.py > index f99b8270a6..d5aeb65048 100644 > --- a/test/py/tests/test_efi_secboot/test_authvar.py > +++ b/test/py/tests/test_efi_secboot/test_authvar.py > @@ -183,7 +183,7 @@ class TestEfiAuthVar(object): > assert 'db:' in ''.join(output) > > output = u_boot_console.run_command_list([ > - 'fatload host 0:1 4000000 db1.auth', > + 'fatload host 0:1 4000000 db2.auth', > 'setenv -e -nv -bs -rt -a -i 4000000:$filesize db']) > assert 'Failed to set EFI variable' in ''.join(output) > > @@ -197,7 +197,7 @@ class TestEfiAuthVar(object): > with u_boot_console.log.section('Test Case 3c'): > # Test Case 3c, update with correct signature > output = u_boot_console.run_command_list([ > - 'fatload host 0:1 4000000 db1.auth', > + 'fatload host 0:1 4000000 db2.auth', > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db']) > assert 'Failed to set EFI variable' not in ''.join(output) > diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py > index 2f862a259a..34719ad1f2 100644 > --- a/test/py/tests/test_efi_secboot/test_signed.py > +++ b/test/py/tests/test_efi_secboot/test_signed.py > @@ -177,7 +177,7 @@ class TestEfiSignedImage(object): > with u_boot_console.log.section('Test Case 5b'): > # Test Case 5b, authenticated if both signatures are verified > output = u_boot_console.run_command_list([ > - 'fatload host 0:1 4000000 db1.auth', > + 'fatload host 0:1 4000000 db2.auth', > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db']) > assert 'Failed to set EFI variable' not in ''.join(output) > output = u_boot_console.run_command_list([ > @@ -201,7 +201,7 @@ class TestEfiSignedImage(object): > with u_boot_console.log.section('Test Case 5d'): > # Test Case 5d, rejected if both of signatures are revoked > output = u_boot_console.run_command_list([ > - 'fatload host 0:1 4000000 dbx_hash1.auth', > + 'fatload host 0:1 4000000 dbx_hash2.auth', > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize dbx']) > assert 'Failed to set EFI variable' not in ''.join(output) > output = u_boot_console.run_command_list([ > @@ -223,7 +223,7 @@ class TestEfiSignedImage(object): > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > 'fatload host 0:1 4000000 PK.auth', > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > - 'fatload host 0:1 4000000 db1.auth', > + 'fatload host 0:1 4000000 db2.auth', > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > 'fatload host 0:1 4000000 dbx_hash1.auth', > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) > @@ -300,7 +300,7 @@ class TestEfiSignedImage(object): > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > 'fatload host 0:1 4000000 PK.auth', > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > - 'fatload host 0:1 4000000 db1.auth', > + 'fatload host 0:1 4000000 db2.auth', > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > 'fatload host 0:1 4000000 dbx_hash384.auth', > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) > @@ -323,7 +323,7 @@ class TestEfiSignedImage(object): > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > 'fatload host 0:1 4000000 PK.auth', > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > - 'fatload host 0:1 4000000 db1.auth', > + 'fatload host 0:1 4000000 db2.auth', > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > 'fatload host 0:1 4000000 dbx_hash512.auth', > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])
Hi Heinrich, On Wed, Apr 3, 2024 at 10:54 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 27.03.24 15:06, Weizhao Ouyang wrote: > > According to UEFI v2.10 spec section 8.2.6, if a caller invokes the > > SetVariables() service, it will produce a digest from hash(VariableName, > > VendorGuid, Attributes, TimeStamp, DataNew_variable_content), then the > > firmware that implements the SetVariable() service will compare the > > digest with the result of applying the signer’s public key to the > > signature. For EFI variable append write, efitools sign-efi-sig-list has > > an option "-a" to add EFI_VARIABLE_APPEND_WRITE attr, and u-boot will > > drop this attribute in efi_set_variable_int(). So if a caller uses > > "sign-efi-sig-list -a" to create the authenticated variable, this append > > write will fail in the u-boot due to "hash check failed". > > Currently U-Boot does not allow to use EFI_VARIABLE_APPEND_WRITE for any > non-existent variable, signed or not. This does not match the EDK II > behavior. I'm not referring to the non-existent variable situation, it's a normal existent variable update. I mean: 1. process PK 2. process KEK 3. process a new APPEND_WRITE KEK (came from "sign-efi-sig-list -a ...", so the digest and attrtibute of the KEK auth file will contain APPEND_WRITE) Then the hash check will fail. > > There is a patch queued for this issue: > > [PATCH v2] efi_loader: fix append write behavior to non-existent variable > https://lore.kernel.org/u-boot/20240402090950.3819705-1-kojima.masahisa@socionext.com/ > > Could you, please, retest with that patch. > > > > > This patch resumes writing the EFI_VARIABLE_APPEND_WRITE attr to ensure > > that the hash check is correct. And also update the "test_efi_secboot" > > test case to compliance with the change. > > > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > > --- > > lib/efi_loader/efi_variable.c | 3 +-- > > test/py/tests/test_efi_secboot/conftest.py | 10 ++++++++++ > > test/py/tests/test_efi_secboot/test_authvar.py | 4 ++-- > > test/py/tests/test_efi_secboot/test_signed.py | 10 +++++----- > > 4 files changed, 18 insertions(+), 9 deletions(-) > > > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > > index 40f7a0fb10..f41aa8f9ad 100644 > > --- a/lib/efi_loader/efi_variable.c > > +++ b/lib/efi_loader/efi_variable.c > > @@ -259,7 +259,6 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, > > /* check if a variable exists */ > > var = efi_var_mem_find(vendor, variable_name, NULL); > > append = !!(attributes & EFI_VARIABLE_APPEND_WRITE); > > - attributes &= ~(u32)EFI_VARIABLE_APPEND_WRITE; > > According to the UEFI specification for GetVariable(): > > "The EFI_VARIABLE_APPEND_WRITE attribute will never be set in the > returned Attributes bitmask parameter." Yes, that's why I called it a RFC patch. > > We don't want to set EFI_VARIABLE_APPEND_WRITE in a variable when > calling efi_var_mem_ins(). The APPEND_WRITE attribute is allowed to be set prior to invoking the service (UEFI section 8.2.6). So anyway we should consider compatibility with this case, along with making changes to GetVariable(). BR, Weizhao > > > delete = !append && (!data_size || !attributes); > > > > /* check attributes */ > > @@ -275,7 +274,7 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, > > > > /* attributes won't be changed */ > > if (!delete && > > - ((ro_check && var->attr != attributes) || > > + ((ro_check && var->attr != (attributes & ~(u32)EFI_VARIABLE_APPEND_WRITE)) || > > (!ro_check && ((var->attr & ~(u32)EFI_VARIABLE_READ_ONLY) > > The (u32) conversions are superfluous. > > Best regards > > Heinrich > > > != (attributes & ~(u32)EFI_VARIABLE_READ_ONLY))))) { > > return EFI_INVALID_PARAMETER; > > diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py > > index ff7ac7c810..0fa0747fc7 100644 > > --- a/test/py/tests/test_efi_secboot/conftest.py > > +++ b/test/py/tests/test_efi_secboot/conftest.py > > @@ -64,6 +64,12 @@ def efi_boot_env(request, u_boot_config): > > check_call('cd %s; %scert-to-efi-sig-list -g %s db1.crt db1.esl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key db db1.esl db1.auth' > > % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > > shell=True) > > + # db2 (APPEND_WRITE) > > + check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_db2/ -keyout db2.key -out db2.crt -nodes -days 365' > > + % mnt_point, shell=True) > > + check_call('cd %s; %scert-to-efi-sig-list -g %s db2.crt db2.esl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key db db2.esl db2.auth' > > + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > > + shell=True) > > # dbx (TEST_dbx certificate) > > check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_dbx/ -keyout dbx.key -out dbx.crt -nodes -days 365' > > % mnt_point, shell=True) > > @@ -84,6 +90,10 @@ def efi_boot_env(request, u_boot_config): > > check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db1.crt dbx_hash1.crl; %ssign-efi-sig-list -t "2020-04-06" -c KEK.crt -k KEK.key dbx dbx_hash1.crl dbx_hash1.auth' > > % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > > shell=True) > > + # dbx_hash2 (digest of TEST_db2 certificate, with APPEND_WRITE) > > + check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db2.crt dbx_hash2.crl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key dbx dbx_hash2.crl dbx_hash2.auth' > > + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > > + shell=True) > > # dbx_db (with TEST_db certificate) > > check_call('cd %s; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx db.esl dbx_db.auth' > > % (mnt_point, EFITOOLS_PATH), > > diff --git a/test/py/tests/test_efi_secboot/test_authvar.py b/test/py/tests/test_efi_secboot/test_authvar.py > > index f99b8270a6..d5aeb65048 100644 > > --- a/test/py/tests/test_efi_secboot/test_authvar.py > > +++ b/test/py/tests/test_efi_secboot/test_authvar.py > > @@ -183,7 +183,7 @@ class TestEfiAuthVar(object): > > assert 'db:' in ''.join(output) > > > > output = u_boot_console.run_command_list([ > > - 'fatload host 0:1 4000000 db1.auth', > > + 'fatload host 0:1 4000000 db2.auth', > > 'setenv -e -nv -bs -rt -a -i 4000000:$filesize db']) > > assert 'Failed to set EFI variable' in ''.join(output) > > > > @@ -197,7 +197,7 @@ class TestEfiAuthVar(object): > > with u_boot_console.log.section('Test Case 3c'): > > # Test Case 3c, update with correct signature > > output = u_boot_console.run_command_list([ > > - 'fatload host 0:1 4000000 db1.auth', > > + 'fatload host 0:1 4000000 db2.auth', > > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > > 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db']) > > assert 'Failed to set EFI variable' not in ''.join(output) > > diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py > > index 2f862a259a..34719ad1f2 100644 > > --- a/test/py/tests/test_efi_secboot/test_signed.py > > +++ b/test/py/tests/test_efi_secboot/test_signed.py > > @@ -177,7 +177,7 @@ class TestEfiSignedImage(object): > > with u_boot_console.log.section('Test Case 5b'): > > # Test Case 5b, authenticated if both signatures are verified > > output = u_boot_console.run_command_list([ > > - 'fatload host 0:1 4000000 db1.auth', > > + 'fatload host 0:1 4000000 db2.auth', > > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db']) > > assert 'Failed to set EFI variable' not in ''.join(output) > > output = u_boot_console.run_command_list([ > > @@ -201,7 +201,7 @@ class TestEfiSignedImage(object): > > with u_boot_console.log.section('Test Case 5d'): > > # Test Case 5d, rejected if both of signatures are revoked > > output = u_boot_console.run_command_list([ > > - 'fatload host 0:1 4000000 dbx_hash1.auth', > > + 'fatload host 0:1 4000000 dbx_hash2.auth', > > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize dbx']) > > assert 'Failed to set EFI variable' not in ''.join(output) > > output = u_boot_console.run_command_list([ > > @@ -223,7 +223,7 @@ class TestEfiSignedImage(object): > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > > 'fatload host 0:1 4000000 PK.auth', > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > > - 'fatload host 0:1 4000000 db1.auth', > > + 'fatload host 0:1 4000000 db2.auth', > > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > > 'fatload host 0:1 4000000 dbx_hash1.auth', > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) > > @@ -300,7 +300,7 @@ class TestEfiSignedImage(object): > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > > 'fatload host 0:1 4000000 PK.auth', > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > > - 'fatload host 0:1 4000000 db1.auth', > > + 'fatload host 0:1 4000000 db2.auth', > > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > > 'fatload host 0:1 4000000 dbx_hash384.auth', > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) > > @@ -323,7 +323,7 @@ class TestEfiSignedImage(object): > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > > 'fatload host 0:1 4000000 PK.auth', > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > > - 'fatload host 0:1 4000000 db1.auth', > > + 'fatload host 0:1 4000000 db2.auth', > > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > > 'fatload host 0:1 4000000 dbx_hash512.auth', > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) >
On Thu, Apr 4, 2024 at 1:48 AM Weizhao Ouyang <o451686892@gmail.com> wrote: > > Hi Heinrich, > > On Wed, Apr 3, 2024 at 10:54 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > On 27.03.24 15:06, Weizhao Ouyang wrote: > > > According to UEFI v2.10 spec section 8.2.6, if a caller invokes the > > > SetVariables() service, it will produce a digest from hash(VariableName, > > > VendorGuid, Attributes, TimeStamp, DataNew_variable_content), then the > > > firmware that implements the SetVariable() service will compare the > > > digest with the result of applying the signer’s public key to the > > > signature. For EFI variable append write, efitools sign-efi-sig-list has > > > an option "-a" to add EFI_VARIABLE_APPEND_WRITE attr, and u-boot will > > > drop this attribute in efi_set_variable_int(). So if a caller uses > > > "sign-efi-sig-list -a" to create the authenticated variable, this append > > > write will fail in the u-boot due to "hash check failed". > > > > Currently U-Boot does not allow to use EFI_VARIABLE_APPEND_WRITE for any > > non-existent variable, signed or not. This does not match the EDK II > > behavior. > > I'm not referring to the non-existent variable situation, it's a normal > existent variable update. > > I mean: > 1. process PK > 2. process KEK > 3. process a new APPEND_WRITE KEK (came from "sign-efi-sig-list -a ...", > so the digest and attrtibute of the KEK auth file will contain > APPEND_WRITE) > Then the hash check will fail. > > > > > There is a patch queued for this issue: > > > > [PATCH v2] efi_loader: fix append write behavior to non-existent variable > > https://lore.kernel.org/u-boot/20240402090950.3819705-1-kojima.masahisa@socionext.com/ > > > > Could you, please, retest with that patch. > > > > > > > > This patch resumes writing the EFI_VARIABLE_APPEND_WRITE attr to ensure > > > that the hash check is correct. And also update the "test_efi_secboot" > > > test case to compliance with the change. > > > > > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > > > --- > > > lib/efi_loader/efi_variable.c | 3 +-- > > > test/py/tests/test_efi_secboot/conftest.py | 10 ++++++++++ > > > test/py/tests/test_efi_secboot/test_authvar.py | 4 ++-- > > > test/py/tests/test_efi_secboot/test_signed.py | 10 +++++----- > > > 4 files changed, 18 insertions(+), 9 deletions(-) > > > > > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > > > index 40f7a0fb10..f41aa8f9ad 100644 > > > --- a/lib/efi_loader/efi_variable.c > > > +++ b/lib/efi_loader/efi_variable.c > > > @@ -259,7 +259,6 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, > > > /* check if a variable exists */ > > > var = efi_var_mem_find(vendor, variable_name, NULL); > > > append = !!(attributes & EFI_VARIABLE_APPEND_WRITE); > > > - attributes &= ~(u32)EFI_VARIABLE_APPEND_WRITE; > > > > According to the UEFI specification for GetVariable(): > > > > "The EFI_VARIABLE_APPEND_WRITE attribute will never be set in the > > returned Attributes bitmask parameter." > > Yes, that's why I called it a RFC patch. > > > > > We don't want to set EFI_VARIABLE_APPEND_WRITE in a variable when > > calling efi_var_mem_ins(). > > The APPEND_WRITE attribute is allowed to be set prior to invoking the > service (UEFI section 8.2.6). So anyway we should consider compatibility > with this case, along with making changes to GetVariable(). Part of the process in edk2 is: VerifyTimeBasedPayloadAndUpdate --> VerifyTimeBasedPayload // calculate digest and compare digest --> AuthServiceInternalUpdateVariableWithTimeStamp --> VariableExLibUpdateVariable --> UpdateVariable // unset the APPEND_WRITE We can align with this implementation. BR, Weizhao > > BR, > Weizhao > > > > > > delete = !append && (!data_size || !attributes); > > > > > > /* check attributes */ > > > @@ -275,7 +274,7 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, > > > > > > /* attributes won't be changed */ > > > if (!delete && > > > - ((ro_check && var->attr != attributes) || > > > + ((ro_check && var->attr != (attributes & ~(u32)EFI_VARIABLE_APPEND_WRITE)) || > > > (!ro_check && ((var->attr & ~(u32)EFI_VARIABLE_READ_ONLY) > > > > The (u32) conversions are superfluous. > > > > Best regards > > > > Heinrich > > > > > != (attributes & ~(u32)EFI_VARIABLE_READ_ONLY))))) { > > > return EFI_INVALID_PARAMETER; > > > diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py > > > index ff7ac7c810..0fa0747fc7 100644 > > > --- a/test/py/tests/test_efi_secboot/conftest.py > > > +++ b/test/py/tests/test_efi_secboot/conftest.py > > > @@ -64,6 +64,12 @@ def efi_boot_env(request, u_boot_config): > > > check_call('cd %s; %scert-to-efi-sig-list -g %s db1.crt db1.esl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key db db1.esl db1.auth' > > > % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > > > shell=True) > > > + # db2 (APPEND_WRITE) > > > + check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_db2/ -keyout db2.key -out db2.crt -nodes -days 365' > > > + % mnt_point, shell=True) > > > + check_call('cd %s; %scert-to-efi-sig-list -g %s db2.crt db2.esl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key db db2.esl db2.auth' > > > + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > > > + shell=True) > > > # dbx (TEST_dbx certificate) > > > check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_dbx/ -keyout dbx.key -out dbx.crt -nodes -days 365' > > > % mnt_point, shell=True) > > > @@ -84,6 +90,10 @@ def efi_boot_env(request, u_boot_config): > > > check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db1.crt dbx_hash1.crl; %ssign-efi-sig-list -t "2020-04-06" -c KEK.crt -k KEK.key dbx dbx_hash1.crl dbx_hash1.auth' > > > % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > > > shell=True) > > > + # dbx_hash2 (digest of TEST_db2 certificate, with APPEND_WRITE) > > > + check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db2.crt dbx_hash2.crl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key dbx dbx_hash2.crl dbx_hash2.auth' > > > + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > > > + shell=True) > > > # dbx_db (with TEST_db certificate) > > > check_call('cd %s; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx db.esl dbx_db.auth' > > > % (mnt_point, EFITOOLS_PATH), > > > diff --git a/test/py/tests/test_efi_secboot/test_authvar.py b/test/py/tests/test_efi_secboot/test_authvar.py > > > index f99b8270a6..d5aeb65048 100644 > > > --- a/test/py/tests/test_efi_secboot/test_authvar.py > > > +++ b/test/py/tests/test_efi_secboot/test_authvar.py > > > @@ -183,7 +183,7 @@ class TestEfiAuthVar(object): > > > assert 'db:' in ''.join(output) > > > > > > output = u_boot_console.run_command_list([ > > > - 'fatload host 0:1 4000000 db1.auth', > > > + 'fatload host 0:1 4000000 db2.auth', > > > 'setenv -e -nv -bs -rt -a -i 4000000:$filesize db']) > > > assert 'Failed to set EFI variable' in ''.join(output) > > > > > > @@ -197,7 +197,7 @@ class TestEfiAuthVar(object): > > > with u_boot_console.log.section('Test Case 3c'): > > > # Test Case 3c, update with correct signature > > > output = u_boot_console.run_command_list([ > > > - 'fatload host 0:1 4000000 db1.auth', > > > + 'fatload host 0:1 4000000 db2.auth', > > > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > > > 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db']) > > > assert 'Failed to set EFI variable' not in ''.join(output) > > > diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py > > > index 2f862a259a..34719ad1f2 100644 > > > --- a/test/py/tests/test_efi_secboot/test_signed.py > > > +++ b/test/py/tests/test_efi_secboot/test_signed.py > > > @@ -177,7 +177,7 @@ class TestEfiSignedImage(object): > > > with u_boot_console.log.section('Test Case 5b'): > > > # Test Case 5b, authenticated if both signatures are verified > > > output = u_boot_console.run_command_list([ > > > - 'fatload host 0:1 4000000 db1.auth', > > > + 'fatload host 0:1 4000000 db2.auth', > > > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db']) > > > assert 'Failed to set EFI variable' not in ''.join(output) > > > output = u_boot_console.run_command_list([ > > > @@ -201,7 +201,7 @@ class TestEfiSignedImage(object): > > > with u_boot_console.log.section('Test Case 5d'): > > > # Test Case 5d, rejected if both of signatures are revoked > > > output = u_boot_console.run_command_list([ > > > - 'fatload host 0:1 4000000 dbx_hash1.auth', > > > + 'fatload host 0:1 4000000 dbx_hash2.auth', > > > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize dbx']) > > > assert 'Failed to set EFI variable' not in ''.join(output) > > > output = u_boot_console.run_command_list([ > > > @@ -223,7 +223,7 @@ class TestEfiSignedImage(object): > > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > > > 'fatload host 0:1 4000000 PK.auth', > > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > > > - 'fatload host 0:1 4000000 db1.auth', > > > + 'fatload host 0:1 4000000 db2.auth', > > > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > > > 'fatload host 0:1 4000000 dbx_hash1.auth', > > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) > > > @@ -300,7 +300,7 @@ class TestEfiSignedImage(object): > > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > > > 'fatload host 0:1 4000000 PK.auth', > > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > > > - 'fatload host 0:1 4000000 db1.auth', > > > + 'fatload host 0:1 4000000 db2.auth', > > > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > > > 'fatload host 0:1 4000000 dbx_hash384.auth', > > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) > > > @@ -323,7 +323,7 @@ class TestEfiSignedImage(object): > > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > > > 'fatload host 0:1 4000000 PK.auth', > > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > > > - 'fatload host 0:1 4000000 db1.auth', > > > + 'fatload host 0:1 4000000 db2.auth', > > > 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > > > 'fatload host 0:1 4000000 dbx_hash512.auth', > > > 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) > >
On 10.04.24 13:53, Weizhao Ouyang wrote: > On Thu, Apr 4, 2024 at 1:48 AM Weizhao Ouyang <o451686892@gmail.com> wrote: >> >> Hi Heinrich, >> >> On Wed, Apr 3, 2024 at 10:54 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>> >>> On 27.03.24 15:06, Weizhao Ouyang wrote: >>>> According to UEFI v2.10 spec section 8.2.6, if a caller invokes the >>>> SetVariables() service, it will produce a digest from hash(VariableName, >>>> VendorGuid, Attributes, TimeStamp, DataNew_variable_content), then the >>>> firmware that implements the SetVariable() service will compare the >>>> digest with the result of applying the signer’s public key to the >>>> signature. For EFI variable append write, efitools sign-efi-sig-list has >>>> an option "-a" to add EFI_VARIABLE_APPEND_WRITE attr, and u-boot will >>>> drop this attribute in efi_set_variable_int(). So if a caller uses >>>> "sign-efi-sig-list -a" to create the authenticated variable, this append >>>> write will fail in the u-boot due to "hash check failed". >>> >>> Currently U-Boot does not allow to use EFI_VARIABLE_APPEND_WRITE for any >>> non-existent variable, signed or not. This does not match the EDK II >>> behavior. >> >> I'm not referring to the non-existent variable situation, it's a normal >> existent variable update. >> >> I mean: >> 1. process PK >> 2. process KEK >> 3. process a new APPEND_WRITE KEK (came from "sign-efi-sig-list -a ...", >> so the digest and attrtibute of the KEK auth file will contain >> APPEND_WRITE) >> Then the hash check will fail. >> >>> >>> There is a patch queued for this issue: >>> >>> [PATCH v2] efi_loader: fix append write behavior to non-existent variable >>> https://lore.kernel.org/u-boot/20240402090950.3819705-1-kojima.masahisa@socionext.com/ >>> >>> Could you, please, retest with that patch. >>> >>>> >>>> This patch resumes writing the EFI_VARIABLE_APPEND_WRITE attr to ensure >>>> that the hash check is correct. And also update the "test_efi_secboot" >>>> test case to compliance with the change. >>>> >>>> Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> >>>> --- >>>> lib/efi_loader/efi_variable.c | 3 +-- >>>> test/py/tests/test_efi_secboot/conftest.py | 10 ++++++++++ >>>> test/py/tests/test_efi_secboot/test_authvar.py | 4 ++-- >>>> test/py/tests/test_efi_secboot/test_signed.py | 10 +++++----- >>>> 4 files changed, 18 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c >>>> index 40f7a0fb10..f41aa8f9ad 100644 >>>> --- a/lib/efi_loader/efi_variable.c >>>> +++ b/lib/efi_loader/efi_variable.c >>>> @@ -259,7 +259,6 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, >>>> /* check if a variable exists */ >>>> var = efi_var_mem_find(vendor, variable_name, NULL); >>>> append = !!(attributes & EFI_VARIABLE_APPEND_WRITE); >>>> - attributes &= ~(u32)EFI_VARIABLE_APPEND_WRITE; >>> >>> According to the UEFI specification for GetVariable(): >>> >>> "The EFI_VARIABLE_APPEND_WRITE attribute will never be set in the >>> returned Attributes bitmask parameter." >> >> Yes, that's why I called it a RFC patch. >> >>> >>> We don't want to set EFI_VARIABLE_APPEND_WRITE in a variable when >>> calling efi_var_mem_ins(). >> >> The APPEND_WRITE attribute is allowed to be set prior to invoking the >> service (UEFI section 8.2.6). So anyway we should consider compatibility >> with this case, along with making changes to GetVariable(). > > Part of the process in edk2 is: > > VerifyTimeBasedPayloadAndUpdate > --> VerifyTimeBasedPayload // calculate digest and compare digest > --> AuthServiceInternalUpdateVariableWithTimeStamp > --> VariableExLibUpdateVariable > --> UpdateVariable // unset the APPEND_WRITE > > We can align with this implementation. I was referring to the attribute value passed to efi_var_mem_ins(), not to the value passed to SetVariable(). efi_var_mem_ins() does not remove the EFI_VARIABLE_APPEND_WRITE bit. The code calling efi_var_mem_ins() should not pass such a value. Best regards Heinrich > > BR, > Weizhao > >> >> BR, >> Weizhao >> >>> >>>> delete = !append && (!data_size || !attributes); >>>> >>>> /* check attributes */ >>>> @@ -275,7 +274,7 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, >>>> >>>> /* attributes won't be changed */ >>>> if (!delete && >>>> - ((ro_check && var->attr != attributes) || >>>> + ((ro_check && var->attr != (attributes & ~(u32)EFI_VARIABLE_APPEND_WRITE)) || >>>> (!ro_check && ((var->attr & ~(u32)EFI_VARIABLE_READ_ONLY) >>> >>> The (u32) conversions are superfluous. >>> >>> Best regards >>> >>> Heinrich >>> >>>> != (attributes & ~(u32)EFI_VARIABLE_READ_ONLY))))) { >>>> return EFI_INVALID_PARAMETER; >>>> diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py >>>> index ff7ac7c810..0fa0747fc7 100644 >>>> --- a/test/py/tests/test_efi_secboot/conftest.py >>>> +++ b/test/py/tests/test_efi_secboot/conftest.py >>>> @@ -64,6 +64,12 @@ def efi_boot_env(request, u_boot_config): >>>> check_call('cd %s; %scert-to-efi-sig-list -g %s db1.crt db1.esl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key db db1.esl db1.auth' >>>> % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), >>>> shell=True) >>>> + # db2 (APPEND_WRITE) >>>> + check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_db2/ -keyout db2.key -out db2.crt -nodes -days 365' >>>> + % mnt_point, shell=True) >>>> + check_call('cd %s; %scert-to-efi-sig-list -g %s db2.crt db2.esl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key db db2.esl db2.auth' >>>> + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), >>>> + shell=True) >>>> # dbx (TEST_dbx certificate) >>>> check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_dbx/ -keyout dbx.key -out dbx.crt -nodes -days 365' >>>> % mnt_point, shell=True) >>>> @@ -84,6 +90,10 @@ def efi_boot_env(request, u_boot_config): >>>> check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db1.crt dbx_hash1.crl; %ssign-efi-sig-list -t "2020-04-06" -c KEK.crt -k KEK.key dbx dbx_hash1.crl dbx_hash1.auth' >>>> % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), >>>> shell=True) >>>> + # dbx_hash2 (digest of TEST_db2 certificate, with APPEND_WRITE) >>>> + check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db2.crt dbx_hash2.crl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key dbx dbx_hash2.crl dbx_hash2.auth' >>>> + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), >>>> + shell=True) >>>> # dbx_db (with TEST_db certificate) >>>> check_call('cd %s; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx db.esl dbx_db.auth' >>>> % (mnt_point, EFITOOLS_PATH), >>>> diff --git a/test/py/tests/test_efi_secboot/test_authvar.py b/test/py/tests/test_efi_secboot/test_authvar.py >>>> index f99b8270a6..d5aeb65048 100644 >>>> --- a/test/py/tests/test_efi_secboot/test_authvar.py >>>> +++ b/test/py/tests/test_efi_secboot/test_authvar.py >>>> @@ -183,7 +183,7 @@ class TestEfiAuthVar(object): >>>> assert 'db:' in ''.join(output) >>>> >>>> output = u_boot_console.run_command_list([ >>>> - 'fatload host 0:1 4000000 db1.auth', >>>> + 'fatload host 0:1 4000000 db2.auth', >>>> 'setenv -e -nv -bs -rt -a -i 4000000:$filesize db']) >>>> assert 'Failed to set EFI variable' in ''.join(output) >>>> >>>> @@ -197,7 +197,7 @@ class TestEfiAuthVar(object): >>>> with u_boot_console.log.section('Test Case 3c'): >>>> # Test Case 3c, update with correct signature >>>> output = u_boot_console.run_command_list([ >>>> - 'fatload host 0:1 4000000 db1.auth', >>>> + 'fatload host 0:1 4000000 db2.auth', >>>> 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', >>>> 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db']) >>>> assert 'Failed to set EFI variable' not in ''.join(output) >>>> diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py >>>> index 2f862a259a..34719ad1f2 100644 >>>> --- a/test/py/tests/test_efi_secboot/test_signed.py >>>> +++ b/test/py/tests/test_efi_secboot/test_signed.py >>>> @@ -177,7 +177,7 @@ class TestEfiSignedImage(object): >>>> with u_boot_console.log.section('Test Case 5b'): >>>> # Test Case 5b, authenticated if both signatures are verified >>>> output = u_boot_console.run_command_list([ >>>> - 'fatload host 0:1 4000000 db1.auth', >>>> + 'fatload host 0:1 4000000 db2.auth', >>>> 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db']) >>>> assert 'Failed to set EFI variable' not in ''.join(output) >>>> output = u_boot_console.run_command_list([ >>>> @@ -201,7 +201,7 @@ class TestEfiSignedImage(object): >>>> with u_boot_console.log.section('Test Case 5d'): >>>> # Test Case 5d, rejected if both of signatures are revoked >>>> output = u_boot_console.run_command_list([ >>>> - 'fatload host 0:1 4000000 dbx_hash1.auth', >>>> + 'fatload host 0:1 4000000 dbx_hash2.auth', >>>> 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize dbx']) >>>> assert 'Failed to set EFI variable' not in ''.join(output) >>>> output = u_boot_console.run_command_list([ >>>> @@ -223,7 +223,7 @@ class TestEfiSignedImage(object): >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', >>>> 'fatload host 0:1 4000000 PK.auth', >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', >>>> - 'fatload host 0:1 4000000 db1.auth', >>>> + 'fatload host 0:1 4000000 db2.auth', >>>> 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', >>>> 'fatload host 0:1 4000000 dbx_hash1.auth', >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) >>>> @@ -300,7 +300,7 @@ class TestEfiSignedImage(object): >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', >>>> 'fatload host 0:1 4000000 PK.auth', >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', >>>> - 'fatload host 0:1 4000000 db1.auth', >>>> + 'fatload host 0:1 4000000 db2.auth', >>>> 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', >>>> 'fatload host 0:1 4000000 dbx_hash384.auth', >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) >>>> @@ -323,7 +323,7 @@ class TestEfiSignedImage(object): >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', >>>> 'fatload host 0:1 4000000 PK.auth', >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', >>>> - 'fatload host 0:1 4000000 db1.auth', >>>> + 'fatload host 0:1 4000000 db2.auth', >>>> 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', >>>> 'fatload host 0:1 4000000 dbx_hash512.auth', >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) >>>
On Wed, Apr 10, 2024 at 8:09 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 10.04.24 13:53, Weizhao Ouyang wrote: > > On Thu, Apr 4, 2024 at 1:48 AM Weizhao Ouyang <o451686892@gmail.com> wrote: > >> > >> Hi Heinrich, > >> > >> On Wed, Apr 3, 2024 at 10:54 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >>> > >>> On 27.03.24 15:06, Weizhao Ouyang wrote: > >>>> According to UEFI v2.10 spec section 8.2.6, if a caller invokes the > >>>> SetVariables() service, it will produce a digest from hash(VariableName, > >>>> VendorGuid, Attributes, TimeStamp, DataNew_variable_content), then the > >>>> firmware that implements the SetVariable() service will compare the > >>>> digest with the result of applying the signer’s public key to the > >>>> signature. For EFI variable append write, efitools sign-efi-sig-list has > >>>> an option "-a" to add EFI_VARIABLE_APPEND_WRITE attr, and u-boot will > >>>> drop this attribute in efi_set_variable_int(). So if a caller uses > >>>> "sign-efi-sig-list -a" to create the authenticated variable, this append > >>>> write will fail in the u-boot due to "hash check failed". > >>> > >>> Currently U-Boot does not allow to use EFI_VARIABLE_APPEND_WRITE for any > >>> non-existent variable, signed or not. This does not match the EDK II > >>> behavior. > >> > >> I'm not referring to the non-existent variable situation, it's a normal > >> existent variable update. > >> > >> I mean: > >> 1. process PK > >> 2. process KEK > >> 3. process a new APPEND_WRITE KEK (came from "sign-efi-sig-list -a ...", > >> so the digest and attrtibute of the KEK auth file will contain > >> APPEND_WRITE) > >> Then the hash check will fail. > >> > >>> > >>> There is a patch queued for this issue: > >>> > >>> [PATCH v2] efi_loader: fix append write behavior to non-existent variable > >>> https://lore.kernel.org/u-boot/20240402090950.3819705-1-kojima.masahisa@socionext.com/ > >>> > >>> Could you, please, retest with that patch. > >>> > >>>> > >>>> This patch resumes writing the EFI_VARIABLE_APPEND_WRITE attr to ensure > >>>> that the hash check is correct. And also update the "test_efi_secboot" > >>>> test case to compliance with the change. > >>>> > >>>> Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > >>>> --- > >>>> lib/efi_loader/efi_variable.c | 3 +-- > >>>> test/py/tests/test_efi_secboot/conftest.py | 10 ++++++++++ > >>>> test/py/tests/test_efi_secboot/test_authvar.py | 4 ++-- > >>>> test/py/tests/test_efi_secboot/test_signed.py | 10 +++++----- > >>>> 4 files changed, 18 insertions(+), 9 deletions(-) > >>>> > >>>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > >>>> index 40f7a0fb10..f41aa8f9ad 100644 > >>>> --- a/lib/efi_loader/efi_variable.c > >>>> +++ b/lib/efi_loader/efi_variable.c > >>>> @@ -259,7 +259,6 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, > >>>> /* check if a variable exists */ > >>>> var = efi_var_mem_find(vendor, variable_name, NULL); > >>>> append = !!(attributes & EFI_VARIABLE_APPEND_WRITE); > >>>> - attributes &= ~(u32)EFI_VARIABLE_APPEND_WRITE; > >>> > >>> According to the UEFI specification for GetVariable(): > >>> > >>> "The EFI_VARIABLE_APPEND_WRITE attribute will never be set in the > >>> returned Attributes bitmask parameter." > >> > >> Yes, that's why I called it a RFC patch. > >> > >>> > >>> We don't want to set EFI_VARIABLE_APPEND_WRITE in a variable when > >>> calling efi_var_mem_ins(). > >> > >> The APPEND_WRITE attribute is allowed to be set prior to invoking the > >> service (UEFI section 8.2.6). So anyway we should consider compatibility > >> with this case, along with making changes to GetVariable(). > > > > Part of the process in edk2 is: > > > > VerifyTimeBasedPayloadAndUpdate > > --> VerifyTimeBasedPayload // calculate digest and compare digest > > --> AuthServiceInternalUpdateVariableWithTimeStamp > > --> VariableExLibUpdateVariable > > --> UpdateVariable // unset the APPEND_WRITE > > > > We can align with this implementation. > > I was referring to the attribute value passed to efi_var_mem_ins(), not > to the value passed to SetVariable(). > > efi_var_mem_ins() does not remove the EFI_VARIABLE_APPEND_WRITE bit. The > code calling efi_var_mem_ins() should not pass such a value. Yeah, efi_var_mem_ins() should escape from this bit. Could we temporarily unset this attr here? BR, Weizhao > > Best regards > > Heinrich > > > > > BR, > > Weizhao > > > >> > >> BR, > >> Weizhao > >> > >>> > >>>> delete = !append && (!data_size || !attributes); > >>>> > >>>> /* check attributes */ > >>>> @@ -275,7 +274,7 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, > >>>> > >>>> /* attributes won't be changed */ > >>>> if (!delete && > >>>> - ((ro_check && var->attr != attributes) || > >>>> + ((ro_check && var->attr != (attributes & ~(u32)EFI_VARIABLE_APPEND_WRITE)) || > >>>> (!ro_check && ((var->attr & ~(u32)EFI_VARIABLE_READ_ONLY) > >>> > >>> The (u32) conversions are superfluous. > >>> > >>> Best regards > >>> > >>> Heinrich > >>> > >>>> != (attributes & ~(u32)EFI_VARIABLE_READ_ONLY))))) { > >>>> return EFI_INVALID_PARAMETER; > >>>> diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py > >>>> index ff7ac7c810..0fa0747fc7 100644 > >>>> --- a/test/py/tests/test_efi_secboot/conftest.py > >>>> +++ b/test/py/tests/test_efi_secboot/conftest.py > >>>> @@ -64,6 +64,12 @@ def efi_boot_env(request, u_boot_config): > >>>> check_call('cd %s; %scert-to-efi-sig-list -g %s db1.crt db1.esl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key db db1.esl db1.auth' > >>>> % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > >>>> shell=True) > >>>> + # db2 (APPEND_WRITE) > >>>> + check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_db2/ -keyout db2.key -out db2.crt -nodes -days 365' > >>>> + % mnt_point, shell=True) > >>>> + check_call('cd %s; %scert-to-efi-sig-list -g %s db2.crt db2.esl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key db db2.esl db2.auth' > >>>> + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > >>>> + shell=True) > >>>> # dbx (TEST_dbx certificate) > >>>> check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_dbx/ -keyout dbx.key -out dbx.crt -nodes -days 365' > >>>> % mnt_point, shell=True) > >>>> @@ -84,6 +90,10 @@ def efi_boot_env(request, u_boot_config): > >>>> check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db1.crt dbx_hash1.crl; %ssign-efi-sig-list -t "2020-04-06" -c KEK.crt -k KEK.key dbx dbx_hash1.crl dbx_hash1.auth' > >>>> % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > >>>> shell=True) > >>>> + # dbx_hash2 (digest of TEST_db2 certificate, with APPEND_WRITE) > >>>> + check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db2.crt dbx_hash2.crl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key dbx dbx_hash2.crl dbx_hash2.auth' > >>>> + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), > >>>> + shell=True) > >>>> # dbx_db (with TEST_db certificate) > >>>> check_call('cd %s; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx db.esl dbx_db.auth' > >>>> % (mnt_point, EFITOOLS_PATH), > >>>> diff --git a/test/py/tests/test_efi_secboot/test_authvar.py b/test/py/tests/test_efi_secboot/test_authvar.py > >>>> index f99b8270a6..d5aeb65048 100644 > >>>> --- a/test/py/tests/test_efi_secboot/test_authvar.py > >>>> +++ b/test/py/tests/test_efi_secboot/test_authvar.py > >>>> @@ -183,7 +183,7 @@ class TestEfiAuthVar(object): > >>>> assert 'db:' in ''.join(output) > >>>> > >>>> output = u_boot_console.run_command_list([ > >>>> - 'fatload host 0:1 4000000 db1.auth', > >>>> + 'fatload host 0:1 4000000 db2.auth', > >>>> 'setenv -e -nv -bs -rt -a -i 4000000:$filesize db']) > >>>> assert 'Failed to set EFI variable' in ''.join(output) > >>>> > >>>> @@ -197,7 +197,7 @@ class TestEfiAuthVar(object): > >>>> with u_boot_console.log.section('Test Case 3c'): > >>>> # Test Case 3c, update with correct signature > >>>> output = u_boot_console.run_command_list([ > >>>> - 'fatload host 0:1 4000000 db1.auth', > >>>> + 'fatload host 0:1 4000000 db2.auth', > >>>> 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > >>>> 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db']) > >>>> assert 'Failed to set EFI variable' not in ''.join(output) > >>>> diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py > >>>> index 2f862a259a..34719ad1f2 100644 > >>>> --- a/test/py/tests/test_efi_secboot/test_signed.py > >>>> +++ b/test/py/tests/test_efi_secboot/test_signed.py > >>>> @@ -177,7 +177,7 @@ class TestEfiSignedImage(object): > >>>> with u_boot_console.log.section('Test Case 5b'): > >>>> # Test Case 5b, authenticated if both signatures are verified > >>>> output = u_boot_console.run_command_list([ > >>>> - 'fatload host 0:1 4000000 db1.auth', > >>>> + 'fatload host 0:1 4000000 db2.auth', > >>>> 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db']) > >>>> assert 'Failed to set EFI variable' not in ''.join(output) > >>>> output = u_boot_console.run_command_list([ > >>>> @@ -201,7 +201,7 @@ class TestEfiSignedImage(object): > >>>> with u_boot_console.log.section('Test Case 5d'): > >>>> # Test Case 5d, rejected if both of signatures are revoked > >>>> output = u_boot_console.run_command_list([ > >>>> - 'fatload host 0:1 4000000 dbx_hash1.auth', > >>>> + 'fatload host 0:1 4000000 dbx_hash2.auth', > >>>> 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize dbx']) > >>>> assert 'Failed to set EFI variable' not in ''.join(output) > >>>> output = u_boot_console.run_command_list([ > >>>> @@ -223,7 +223,7 @@ class TestEfiSignedImage(object): > >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > >>>> 'fatload host 0:1 4000000 PK.auth', > >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > >>>> - 'fatload host 0:1 4000000 db1.auth', > >>>> + 'fatload host 0:1 4000000 db2.auth', > >>>> 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > >>>> 'fatload host 0:1 4000000 dbx_hash1.auth', > >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) > >>>> @@ -300,7 +300,7 @@ class TestEfiSignedImage(object): > >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > >>>> 'fatload host 0:1 4000000 PK.auth', > >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > >>>> - 'fatload host 0:1 4000000 db1.auth', > >>>> + 'fatload host 0:1 4000000 db2.auth', > >>>> 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > >>>> 'fatload host 0:1 4000000 dbx_hash384.auth', > >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) > >>>> @@ -323,7 +323,7 @@ class TestEfiSignedImage(object): > >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > >>>> 'fatload host 0:1 4000000 PK.auth', > >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > >>>> - 'fatload host 0:1 4000000 db1.auth', > >>>> + 'fatload host 0:1 4000000 db2.auth', > >>>> 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > >>>> 'fatload host 0:1 4000000 dbx_hash512.auth', > >>>> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) > >>> >
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index 40f7a0fb10..f41aa8f9ad 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -259,7 +259,6 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, /* check if a variable exists */ var = efi_var_mem_find(vendor, variable_name, NULL); append = !!(attributes & EFI_VARIABLE_APPEND_WRITE); - attributes &= ~(u32)EFI_VARIABLE_APPEND_WRITE; delete = !append && (!data_size || !attributes); /* check attributes */ @@ -275,7 +274,7 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, /* attributes won't be changed */ if (!delete && - ((ro_check && var->attr != attributes) || + ((ro_check && var->attr != (attributes & ~(u32)EFI_VARIABLE_APPEND_WRITE)) || (!ro_check && ((var->attr & ~(u32)EFI_VARIABLE_READ_ONLY) != (attributes & ~(u32)EFI_VARIABLE_READ_ONLY))))) { return EFI_INVALID_PARAMETER; diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py index ff7ac7c810..0fa0747fc7 100644 --- a/test/py/tests/test_efi_secboot/conftest.py +++ b/test/py/tests/test_efi_secboot/conftest.py @@ -64,6 +64,12 @@ def efi_boot_env(request, u_boot_config): check_call('cd %s; %scert-to-efi-sig-list -g %s db1.crt db1.esl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key db db1.esl db1.auth' % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), shell=True) + # db2 (APPEND_WRITE) + check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_db2/ -keyout db2.key -out db2.crt -nodes -days 365' + % mnt_point, shell=True) + check_call('cd %s; %scert-to-efi-sig-list -g %s db2.crt db2.esl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key db db2.esl db2.auth' + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), + shell=True) # dbx (TEST_dbx certificate) check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_dbx/ -keyout dbx.key -out dbx.crt -nodes -days 365' % mnt_point, shell=True) @@ -84,6 +90,10 @@ def efi_boot_env(request, u_boot_config): check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db1.crt dbx_hash1.crl; %ssign-efi-sig-list -t "2020-04-06" -c KEK.crt -k KEK.key dbx dbx_hash1.crl dbx_hash1.auth' % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), shell=True) + # dbx_hash2 (digest of TEST_db2 certificate, with APPEND_WRITE) + check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db2.crt dbx_hash2.crl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key dbx dbx_hash2.crl dbx_hash2.auth' + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), + shell=True) # dbx_db (with TEST_db certificate) check_call('cd %s; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx db.esl dbx_db.auth' % (mnt_point, EFITOOLS_PATH), diff --git a/test/py/tests/test_efi_secboot/test_authvar.py b/test/py/tests/test_efi_secboot/test_authvar.py index f99b8270a6..d5aeb65048 100644 --- a/test/py/tests/test_efi_secboot/test_authvar.py +++ b/test/py/tests/test_efi_secboot/test_authvar.py @@ -183,7 +183,7 @@ class TestEfiAuthVar(object): assert 'db:' in ''.join(output) output = u_boot_console.run_command_list([ - 'fatload host 0:1 4000000 db1.auth', + 'fatload host 0:1 4000000 db2.auth', 'setenv -e -nv -bs -rt -a -i 4000000:$filesize db']) assert 'Failed to set EFI variable' in ''.join(output) @@ -197,7 +197,7 @@ class TestEfiAuthVar(object): with u_boot_console.log.section('Test Case 3c'): # Test Case 3c, update with correct signature output = u_boot_console.run_command_list([ - 'fatload host 0:1 4000000 db1.auth', + 'fatload host 0:1 4000000 db2.auth', 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db']) assert 'Failed to set EFI variable' not in ''.join(output) diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py index 2f862a259a..34719ad1f2 100644 --- a/test/py/tests/test_efi_secboot/test_signed.py +++ b/test/py/tests/test_efi_secboot/test_signed.py @@ -177,7 +177,7 @@ class TestEfiSignedImage(object): with u_boot_console.log.section('Test Case 5b'): # Test Case 5b, authenticated if both signatures are verified output = u_boot_console.run_command_list([ - 'fatload host 0:1 4000000 db1.auth', + 'fatload host 0:1 4000000 db2.auth', 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db']) assert 'Failed to set EFI variable' not in ''.join(output) output = u_boot_console.run_command_list([ @@ -201,7 +201,7 @@ class TestEfiSignedImage(object): with u_boot_console.log.section('Test Case 5d'): # Test Case 5d, rejected if both of signatures are revoked output = u_boot_console.run_command_list([ - 'fatload host 0:1 4000000 dbx_hash1.auth', + 'fatload host 0:1 4000000 dbx_hash2.auth', 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize dbx']) assert 'Failed to set EFI variable' not in ''.join(output) output = u_boot_console.run_command_list([ @@ -223,7 +223,7 @@ class TestEfiSignedImage(object): 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', 'fatload host 0:1 4000000 PK.auth', 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', - 'fatload host 0:1 4000000 db1.auth', + 'fatload host 0:1 4000000 db2.auth', 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', 'fatload host 0:1 4000000 dbx_hash1.auth', 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) @@ -300,7 +300,7 @@ class TestEfiSignedImage(object): 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', 'fatload host 0:1 4000000 PK.auth', 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', - 'fatload host 0:1 4000000 db1.auth', + 'fatload host 0:1 4000000 db2.auth', 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', 'fatload host 0:1 4000000 dbx_hash384.auth', 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) @@ -323,7 +323,7 @@ class TestEfiSignedImage(object): 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', 'fatload host 0:1 4000000 PK.auth', 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', - 'fatload host 0:1 4000000 db1.auth', + 'fatload host 0:1 4000000 db2.auth', 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', 'fatload host 0:1 4000000 dbx_hash512.auth', 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])
According to UEFI v2.10 spec section 8.2.6, if a caller invokes the SetVariables() service, it will produce a digest from hash(VariableName, VendorGuid, Attributes, TimeStamp, DataNew_variable_content), then the firmware that implements the SetVariable() service will compare the digest with the result of applying the signer’s public key to the signature. For EFI variable append write, efitools sign-efi-sig-list has an option "-a" to add EFI_VARIABLE_APPEND_WRITE attr, and u-boot will drop this attribute in efi_set_variable_int(). So if a caller uses "sign-efi-sig-list -a" to create the authenticated variable, this append write will fail in the u-boot due to "hash check failed". This patch resumes writing the EFI_VARIABLE_APPEND_WRITE attr to ensure that the hash check is correct. And also update the "test_efi_secboot" test case to compliance with the change. Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> --- lib/efi_loader/efi_variable.c | 3 +-- test/py/tests/test_efi_secboot/conftest.py | 10 ++++++++++ test/py/tests/test_efi_secboot/test_authvar.py | 4 ++-- test/py/tests/test_efi_secboot/test_signed.py | 10 +++++----- 4 files changed, 18 insertions(+), 9 deletions(-)