| Message ID | 20231109125541.10845-1-o451686892@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: Fix UEFI variable error handling | expand |
On 11/9/23 04:55, Weizhao Ouyang wrote: > Correct some UEFI variable error handing code paths. > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > --- > lib/efi_loader/efi_var_file.c | 1 + > lib/efi_loader/efi_variable.c | 8 ++++---- > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c > index 62e071bd83..dbb9b1f3fc 100644 > --- a/lib/efi_loader/efi_var_file.c > +++ b/lib/efi_loader/efi_var_file.c > @@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void) > log_err("Invalid EFI variables file\n"); > error: > free(buf); > + return ret; Hello Weizhao, thank you for looking into the error handling. U-Boot's UEFI variables can either be stored in file ubootefi.var in the ESP or in the RPMB (Replay Protected Memory Block) partition of the eMMC. The suggested changes are about handling of errors when reading or writing the file ubootefi.var. Security relevant variables (PK, KEK, db, dbx) are never read from file. On first boot a file ubootefi.var will not exist. It has to be created by U-Boot. If efi_var_from_file() would return an error if the file does not exist or is corrupted, we would never be able to boot such a system. This is why deliberately we return EFI_SUCCESS here. What is missing in the code is a comment explaining the design decision. > #endif > return EFI_SUCCESS; > } > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > index be95ed44e6..13966297c6 100644 > --- a/lib/efi_loader/efi_variable.c > +++ b/lib/efi_loader/efi_variable.c > @@ -350,17 +350,17 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, > > if (var_type == EFI_AUTH_VAR_PK) > ret = efi_init_secure_state(); > - else > - ret = EFI_SUCCESS; The two lines are unreachable code and should be removed. > + if (ret != EFI_SUCCESS) > + return ret; These new lines should only be reached if var_type == EFI_AUTH_VAR_PK. I am not sure what Would be the right error handling if efi_init_secure_state() fails: * Do we have to set PK to the old value? * Should we still persist PK to ubootefi.var? However we decide we should describe our decisions in a code comment. > > /* > * Write non-volatile EFI variables to file > * TODO: check if a value change has occured to avoid superfluous writes > */ > if (attributes & EFI_VARIABLE_NON_VOLATILE) > - efi_var_to_file(); > + ret = efi_var_to_file(); The discussion here should focus on the treatment of errors in the file-system. The following error cases come to my mind: * ESP partition is missing * ESP FAT file system is corrupted * There is no space on the ESP. * The medium (e.g. SD card) is in write only mode The current code gives priority to enable booting in all adverse situations. Do you think this is a bad choice? @Ilias: Please, chime in. Best regards Heinrich > > - return EFI_SUCCESS; > + return ret; > } > > efi_status_t efi_query_variable_info_int(u32 attributes,
On Thu, Nov 9, 2023 at 9:57 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 11/9/23 04:55, Weizhao Ouyang wrote: > > Correct some UEFI variable error handing code paths. > > > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > > --- > > lib/efi_loader/efi_var_file.c | 1 + > > lib/efi_loader/efi_variable.c | 8 ++++---- > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c > > index 62e071bd83..dbb9b1f3fc 100644 > > --- a/lib/efi_loader/efi_var_file.c > > +++ b/lib/efi_loader/efi_var_file.c > > @@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void) > > log_err("Invalid EFI variables file\n"); > > error: > > free(buf); > > + return ret; > > Hello Weizhao, > > thank you for looking into the error handling. > > U-Boot's UEFI variables can either be stored in file ubootefi.var in the > ESP or in the RPMB (Replay Protected Memory Block) partition of the > eMMC. The suggested changes are about handling of errors when reading or > writing the file ubootefi.var. Security relevant variables (PK, KEK, db, > dbx) are never read from file. > Hi Heinrich, Yes, my intention was to check for errors related to non-volatile variables. > On first boot a file ubootefi.var will not exist. It has to be created > by U-Boot. If efi_var_from_file() would return an error if the file does > not exist or is corrupted, we would never be able to boot such a system. > This is why deliberately we return EFI_SUCCESS here. What is missing in > the code is a comment explaining the design decision. Sorry, I missed this scene. Maybe a comment is needed or we can split the scene. > > > #endif > > return EFI_SUCCESS; > > } > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > > index be95ed44e6..13966297c6 100644 > > --- a/lib/efi_loader/efi_variable.c > > +++ b/lib/efi_loader/efi_variable.c > > @@ -350,17 +350,17 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, > > > > if (var_type == EFI_AUTH_VAR_PK) > > ret = efi_init_secure_state(); > > - else > > - ret = EFI_SUCCESS; > > The two lines are unreachable code and should be removed. > > > + if (ret != EFI_SUCCESS) > > + return ret; > > These new lines should only be reached if var_type == EFI_AUTH_VAR_PK. > > I am not sure what Would be the right error handling if > efi_init_secure_state() fails: > > * Do we have to set PK to the old value? > * Should we still persist PK to ubootefi.var? > > However we decide we should describe our decisions in a code comment. IMO, efi_init_secure_state() is not just dealing with PK but also includes Secure Boot mode transitions. So it may returns some appropriate error when dealing with variable authentication. > > > > > /* > > * Write non-volatile EFI variables to file > > * TODO: check if a value change has occured to avoid superfluous writes > > */ > > if (attributes & EFI_VARIABLE_NON_VOLATILE) > > - efi_var_to_file(); > > + ret = efi_var_to_file(); > > The discussion here should focus on the treatment of errors in the > file-system. > > The following error cases come to my mind: > > * ESP partition is missing > * ESP FAT file system is corrupted > * There is no space on the ESP. > * The medium (e.g. SD card) is in write only mode > > The current code gives priority to enable booting in all adverse > situations. Do you think this is a bad choice? I think the ESP status (including fs corruption) is the most likely situation to have an effect. But we should catch it earlier anyway. BR, Weizhao > > @Ilias: Please, chime in. > > Best regards > > Heinrich > > > > > - return EFI_SUCCESS; > > + return ret; > > } > > > > efi_status_t efi_query_variable_info_int(u32 attributes, >
Hi Heinrich, Weizhao On Thu, 9 Nov 2023 at 15:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 11/9/23 04:55, Weizhao Ouyang wrote: > > Correct some UEFI variable error handing code paths. > > > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > > --- > > lib/efi_loader/efi_var_file.c | 1 + > > lib/efi_loader/efi_variable.c | 8 ++++---- > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c > > index 62e071bd83..dbb9b1f3fc 100644 > > --- a/lib/efi_loader/efi_var_file.c > > +++ b/lib/efi_loader/efi_var_file.c > > @@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void) > > log_err("Invalid EFI variables file\n"); > > error: > > free(buf); > > + return ret; > > Hello Weizhao, > > thank you for looking into the error handling. > > U-Boot's UEFI variables can either be stored in file ubootefi.var in the > ESP or in the RPMB (Replay Protected Memory Block) partition of the > eMMC. The suggested changes are about handling of errors when reading or > writing the file ubootefi.var. Security relevant variables (PK, KEK, db, > dbx) are never read from file. > > On first boot a file ubootefi.var will not exist. It has to be created > by U-Boot. If efi_var_from_file() would return an error if the file does > not exist or is corrupted, we would never be able to boot such a system. > This is why deliberately we return EFI_SUCCESS here. What is missing in > the code is a comment explaining the design decision. Yes, that's correct. The function description tries to explain that but is a bit vague. > > > #endif > > return EFI_SUCCESS; > > } > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > > index be95ed44e6..13966297c6 100644 > > --- a/lib/efi_loader/efi_variable.c > > +++ b/lib/efi_loader/efi_variable.c > > @@ -350,17 +350,17 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, > > > > if (var_type == EFI_AUTH_VAR_PK) > > ret = efi_init_secure_state(); > > - else > > - ret = EFI_SUCCESS; > > The two lines are unreachable code and should be removed. > > > + if (ret != EFI_SUCCESS) > > + return ret; > > These new lines should only be reached if var_type == EFI_AUTH_VAR_PK. > Yea agree here > I am not sure what Would be the right error handling if > efi_init_secure_state() fails: > > * Do we have to set PK to the old value? What do you mean by old value? > * Should we still persist PK to ubootefi.var? I would argue that we don't really care about what happens in this case. Writing authenticated variables on a file is only supported if preseeding is disabled and it *never* gets restored. We basically allow that code to test EFI secure boot by writing PK, KEK, DB on the fly, but once we reboot those variables are gone. If preseeding is enabled we don't write that at all. We return EFI_WRITE_PROTECTED. We could do that regardless. But since we have those tests, the memory backend should still be allowed to write those. > > However we decide we should describe our decisions in a code comment. I think the logic here should be 1. If variables are preseeded and restoring any authenticated variables fails, the EFI subsystem should refuse to start (which it already does) 2. If preseeding is not configured we can continue as best effort and try to recover the board and rewrite variables. We don't trust variables stored in a file and we should keep it that way > > > > > /* > > * Write non-volatile EFI variables to file > > * TODO: check if a value change has occured to avoid superfluous writes > > */ > > if (attributes & EFI_VARIABLE_NON_VOLATILE) > > - efi_var_to_file(); > > + ret = efi_var_to_file(); > > The discussion here should focus on the treatment of errors in the > file-system. > > The following error cases come to my mind: > > * ESP partition is missing > * ESP FAT file system is corrupted > * There is no space on the ESP. > * The medium (e.g. SD card) is in write only mode > > The current code gives priority to enable booting in all adverse > situations. Do you think this is a bad choice? > > @Ilias: Please, chime in. I think we should continue doing that *unless* preseeding is enabled. If we preseed authenticated variables and the restoration of the switching of the secure boot mode fails we must stop the EFI subsystem. The first function that gets called is efi_init_variables(). commit 77bb14758dcb1 explains some of the logic in that file but in short - We load variables from a file first, excluding authenticated ones - We load preseeded variables. That step allows you to load PK, KEK, db etc that are stored as part of the u-boot binary. We assume that in a sane design a chain of trust *will* authenticate u-boot prior to loading, so we 'trust' those variables. If this fails which includes setting the secure state we correctly stop the EFI subsystem - efi_var_restore() is checking for duplicates. So a non-authenticated variable set by the file load won't be overwritten by built-in ones. This allows users to override built-in variables (apart from auth ones obviously) Cheers /Ilias > > Best regards > > Heinrich > > > > > - return EFI_SUCCESS; > > + return ret; > > } > > > > efi_status_t efi_query_variable_info_int(u32 attributes, >
Am 10. November 2023 11:04:24 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: >Hi Heinrich, Weizhao > >On Thu, 9 Nov 2023 at 15:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 11/9/23 04:55, Weizhao Ouyang wrote: >> > Correct some UEFI variable error handing code paths. >> > >> > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> >> > --- >> > lib/efi_loader/efi_var_file.c | 1 + >> > lib/efi_loader/efi_variable.c | 8 ++++---- >> > 2 files changed, 5 insertions(+), 4 deletions(-) >> > >> > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c >> > index 62e071bd83..dbb9b1f3fc 100644 >> > --- a/lib/efi_loader/efi_var_file.c >> > +++ b/lib/efi_loader/efi_var_file.c >> > @@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void) >> > log_err("Invalid EFI variables file\n"); >> > error: >> > free(buf); >> > + return ret; >> >> Hello Weizhao, >> >> thank you for looking into the error handling. >> >> U-Boot's UEFI variables can either be stored in file ubootefi.var in the >> ESP or in the RPMB (Replay Protected Memory Block) partition of the >> eMMC. The suggested changes are about handling of errors when reading or >> writing the file ubootefi.var. Security relevant variables (PK, KEK, db, >> dbx) are never read from file. >> >> On first boot a file ubootefi.var will not exist. It has to be created >> by U-Boot. If efi_var_from_file() would return an error if the file does >> not exist or is corrupted, we would never be able to boot such a system. >> This is why deliberately we return EFI_SUCCESS here. What is missing in >> the code is a comment explaining the design decision. > >Yes, that's correct. The function description tries to explain that >but is a bit vague. > >> >> > #endif >> > return EFI_SUCCESS; >> > } >> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c >> > index be95ed44e6..13966297c6 100644 >> > --- a/lib/efi_loader/efi_variable.c >> > +++ b/lib/efi_loader/efi_variable.c >> > @@ -350,17 +350,17 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, >> > >> > if (var_type == EFI_AUTH_VAR_PK) >> > ret = efi_init_secure_state(); >> > - else >> > - ret = EFI_SUCCESS; >> >> The two lines are unreachable code and should be removed. >> >> > + if (ret != EFI_SUCCESS) >> > + return ret; >> >> These new lines should only be reached if var_type == EFI_AUTH_VAR_PK. >> > >Yea agree here > >> I am not sure what Would be the right error handling if >> efi_init_secure_state() fails: >> >> * Do we have to set PK to the old value? > >What do you mean by old value? We are in SetVariable() and set, changed, or deleted PK in memory. But this has lead to some inconsistency. Should the prior state be restored? Regards Heinrich > >> * Should we still persist PK to ubootefi.var? > >I would argue that we don't really care about what happens in this >case. Writing authenticated variables on a file is only supported if >preseeding is disabled and it *never* gets restored. We basically >allow that code to test EFI secure boot by writing PK, KEK, DB on the >fly, but once we reboot those variables are gone. >If preseeding is enabled we don't write that at all. We return >EFI_WRITE_PROTECTED. We could do that regardless. But since we have >those tests, the memory backend should still be allowed to write >those. > >> >> However we decide we should describe our decisions in a code comment. > >I think the logic here should be >1. If variables are preseeded and restoring any authenticated >variables fails, the EFI subsystem should refuse to start (which it >already does) >2. If preseeding is not configured we can continue as best effort and >try to recover the board and rewrite variables. We don't trust >variables stored in a file and we should keep it that way > >> >> > >> > /* >> > * Write non-volatile EFI variables to file >> > * TODO: check if a value change has occured to avoid superfluous writes >> > */ >> > if (attributes & EFI_VARIABLE_NON_VOLATILE) >> > - efi_var_to_file(); >> > + ret = efi_var_to_file(); >> >> The discussion here should focus on the treatment of errors in the >> file-system. >> >> The following error cases come to my mind: >> >> * ESP partition is missing >> * ESP FAT file system is corrupted >> * There is no space on the ESP. >> * The medium (e.g. SD card) is in write only mode >> >> The current code gives priority to enable booting in all adverse >> situations. Do you think this is a bad choice? >> >> @Ilias: Please, chime in. > >I think we should continue doing that *unless* preseeding is enabled. >If we preseed authenticated variables and the restoration of the >switching of the secure boot mode fails we must stop the EFI >subsystem. >The first function that gets called is efi_init_variables(). >commit 77bb14758dcb1 explains some of the logic in that file but in short >- We load variables from a file first, excluding authenticated ones >- We load preseeded variables. That step allows you to load PK, KEK, >db etc that are stored as part of the u-boot binary. We assume that >in a sane design a chain of trust *will* authenticate u-boot prior to >loading, so we 'trust' those variables. If this fails which includes >setting the secure state we correctly stop the EFI subsystem >- efi_var_restore() is checking for duplicates. So a non-authenticated >variable set by the file load won't be overwritten by built-in ones. >This allows users to override built-in variables (apart from auth ones >obviously) > >Cheers >/Ilias > > > > > > >> >> Best regards >> >> Heinrich >> >> > >> > - return EFI_SUCCESS; >> > + return ret; >> > } >> > >> > efi_status_t efi_query_variable_info_int(u32 attributes, >>
On Fri, Nov 10, 2023 at 9:12 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > Am 10. November 2023 11:04:24 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: > >Hi Heinrich, Weizhao > > > >On Thu, 9 Nov 2023 at 15:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> On 11/9/23 04:55, Weizhao Ouyang wrote: > >> > Correct some UEFI variable error handing code paths. > >> > > >> > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > >> > --- > >> > lib/efi_loader/efi_var_file.c | 1 + > >> > lib/efi_loader/efi_variable.c | 8 ++++---- > >> > 2 files changed, 5 insertions(+), 4 deletions(-) > >> > > >> > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c > >> > index 62e071bd83..dbb9b1f3fc 100644 > >> > --- a/lib/efi_loader/efi_var_file.c > >> > +++ b/lib/efi_loader/efi_var_file.c > >> > @@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void) > >> > log_err("Invalid EFI variables file\n"); > >> > error: > >> > free(buf); > >> > + return ret; > >> > >> Hello Weizhao, > >> > >> thank you for looking into the error handling. > >> > >> U-Boot's UEFI variables can either be stored in file ubootefi.var in the > >> ESP or in the RPMB (Replay Protected Memory Block) partition of the > >> eMMC. The suggested changes are about handling of errors when reading or > >> writing the file ubootefi.var. Security relevant variables (PK, KEK, db, > >> dbx) are never read from file. > >> > >> On first boot a file ubootefi.var will not exist. It has to be created > >> by U-Boot. If efi_var_from_file() would return an error if the file does > >> not exist or is corrupted, we would never be able to boot such a system. > >> This is why deliberately we return EFI_SUCCESS here. What is missing in > >> the code is a comment explaining the design decision. > > > >Yes, that's correct. The function description tries to explain that > >but is a bit vague. > > > >> > >> > #endif > >> > return EFI_SUCCESS; > >> > } > >> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > >> > index be95ed44e6..13966297c6 100644 > >> > --- a/lib/efi_loader/efi_variable.c > >> > +++ b/lib/efi_loader/efi_variable.c > >> > @@ -350,17 +350,17 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, > >> > > >> > if (var_type == EFI_AUTH_VAR_PK) > >> > ret = efi_init_secure_state(); > >> > - else > >> > - ret = EFI_SUCCESS; > >> > >> The two lines are unreachable code and should be removed. > >> > >> > + if (ret != EFI_SUCCESS) > >> > + return ret; > >> > >> These new lines should only be reached if var_type == EFI_AUTH_VAR_PK. > >> > > > >Yea agree here > > > >> I am not sure what Would be the right error handling if > >> efi_init_secure_state() fails: > >> > >> * Do we have to set PK to the old value? > > > >What do you mean by old value? > > We are in SetVariable() and set, changed, or deleted PK in memory. But this has lead to some inconsistency. Should the prior state be restored? Shall we restore the variable "SecureBoot"? > > Regards > > Heinrich > > > > >> * Should we still persist PK to ubootefi.var? > > > >I would argue that we don't really care about what happens in this > >case. Writing authenticated variables on a file is only supported if > >preseeding is disabled and it *never* gets restored. We basically > >allow that code to test EFI secure boot by writing PK, KEK, DB on the > >fly, but once we reboot those variables are gone. > >If preseeding is enabled we don't write that at all. We return > >EFI_WRITE_PROTECTED. We could do that regardless. But since we have > >those tests, the memory backend should still be allowed to write > >those. > > > >> > >> However we decide we should describe our decisions in a code comment. > > > >I think the logic here should be > >1. If variables are preseeded and restoring any authenticated > >variables fails, the EFI subsystem should refuse to start (which it > >already does) > >2. If preseeding is not configured we can continue as best effort and > >try to recover the board and rewrite variables. We don't trust > >variables stored in a file and we should keep it that way > > > >> > >> > > >> > /* > >> > * Write non-volatile EFI variables to file > >> > * TODO: check if a value change has occured to avoid superfluous writes > >> > */ > >> > if (attributes & EFI_VARIABLE_NON_VOLATILE) > >> > - efi_var_to_file(); > >> > + ret = efi_var_to_file(); > >> > >> The discussion here should focus on the treatment of errors in the > >> file-system. > >> > >> The following error cases come to my mind: > >> > >> * ESP partition is missing > >> * ESP FAT file system is corrupted > >> * There is no space on the ESP. > >> * The medium (e.g. SD card) is in write only mode > >> > >> The current code gives priority to enable booting in all adverse > >> situations. Do you think this is a bad choice? > >> > >> @Ilias: Please, chime in. > > > >I think we should continue doing that *unless* preseeding is enabled. > >If we preseed authenticated variables and the restoration of the > >switching of the secure boot mode fails we must stop the EFI > >subsystem. > >The first function that gets called is efi_init_variables(). > >commit 77bb14758dcb1 explains some of the logic in that file but in short > >- We load variables from a file first, excluding authenticated ones > >- We load preseeded variables. That step allows you to load PK, KEK, > >db etc that are stored as part of the u-boot binary. We assume that > >in a sane design a chain of trust *will* authenticate u-boot prior to > >loading, so we 'trust' those variables. If this fails which includes > >setting the secure state we correctly stop the EFI subsystem > >- efi_var_restore() is checking for duplicates. So a non-authenticated > >variable set by the file load won't be overwritten by built-in ones. > >This allows users to override built-in variables (apart from auth ones > >obviously) nit: currently if efi_var_restore() fails to set the EFI variable, it will still return EFI_SUCCESS. BR, Weizhao > > > >Cheers > >/Ilias > > > > > > > > > > > > > >> > >> Best regards > >> > >> Heinrich > >> > >> > > >> > - return EFI_SUCCESS; > >> > + return ret; > >> > } > >> > > >> > efi_status_t efi_query_variable_info_int(u32 attributes, > >>
On Fri, 10 Nov 2023 at 15:12, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > Am 10. November 2023 11:04:24 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: > >Hi Heinrich, Weizhao > > > >On Thu, 9 Nov 2023 at 15:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> On 11/9/23 04:55, Weizhao Ouyang wrote: > >> > Correct some UEFI variable error handing code paths. > >> > > >> > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > >> > --- > >> > lib/efi_loader/efi_var_file.c | 1 + > >> > lib/efi_loader/efi_variable.c | 8 ++++---- > >> > 2 files changed, 5 insertions(+), 4 deletions(-) > >> > > >> > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c > >> > index 62e071bd83..dbb9b1f3fc 100644 > >> > --- a/lib/efi_loader/efi_var_file.c > >> > +++ b/lib/efi_loader/efi_var_file.c > >> > @@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void) > >> > log_err("Invalid EFI variables file\n"); > >> > error: > >> > free(buf); > >> > + return ret; > >> > >> Hello Weizhao, > >> > >> thank you for looking into the error handling. > >> > >> U-Boot's UEFI variables can either be stored in file ubootefi.var in the > >> ESP or in the RPMB (Replay Protected Memory Block) partition of the > >> eMMC. The suggested changes are about handling of errors when reading or > >> writing the file ubootefi.var. Security relevant variables (PK, KEK, db, > >> dbx) are never read from file. > >> > >> On first boot a file ubootefi.var will not exist. It has to be created > >> by U-Boot. If efi_var_from_file() would return an error if the file does > >> not exist or is corrupted, we would never be able to boot such a system. > >> This is why deliberately we return EFI_SUCCESS here. What is missing in > >> the code is a comment explaining the design decision. > > > >Yes, that's correct. The function description tries to explain that > >but is a bit vague. > > > >> > >> > #endif > >> > return EFI_SUCCESS; > >> > } > >> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > >> > index be95ed44e6..13966297c6 100644 > >> > --- a/lib/efi_loader/efi_variable.c > >> > +++ b/lib/efi_loader/efi_variable.c > >> > @@ -350,17 +350,17 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, > >> > > >> > if (var_type == EFI_AUTH_VAR_PK) > >> > ret = efi_init_secure_state(); > >> > - else > >> > - ret = EFI_SUCCESS; > >> > >> The two lines are unreachable code and should be removed. > >> > >> > + if (ret != EFI_SUCCESS) > >> > + return ret; > >> > >> These new lines should only be reached if var_type == EFI_AUTH_VAR_PK. > >> > > > >Yea agree here > > > >> I am not sure what Would be the right error handling if > >> efi_init_secure_state() fails: > >> > >> * Do we have to set PK to the old value? > > > >What do you mean by old value? > > We are in SetVariable() and set, changed, or deleted PK in memory. But this has lead to some inconsistency. Should the prior state be restored? Ah right. As I said this code is basically there to allow us to run self-tests. If setting the security state fails, those tests should fail whether we revert or not. So I think we don't care. We could add a comment explaining the situation with the secure boot state transition when auth variables are not preseseded Thanks /Ilias > > Regards > > Heinrich > > > > >> * Should we still persist PK to ubootefi.var? > > > >I would argue that we don't really care about what happens in this > >case. Writing authenticated variables on a file is only supported if > >preseeding is disabled and it *never* gets restored. We basically > >allow that code to test EFI secure boot by writing PK, KEK, DB on the > >fly, but once we reboot those variables are gone. > >If preseeding is enabled we don't write that at all. We return > >EFI_WRITE_PROTECTED. We could do that regardless. But since we have > >those tests, the memory backend should still be allowed to write > >those. > > > >> > >> However we decide we should describe our decisions in a code comment. > > > >I think the logic here should be > >1. If variables are preseeded and restoring any authenticated > >variables fails, the EFI subsystem should refuse to start (which it > >already does) > >2. If preseeding is not configured we can continue as best effort and > >try to recover the board and rewrite variables. We don't trust > >variables stored in a file and we should keep it that way > > > >> > >> > > >> > /* > >> > * Write non-volatile EFI variables to file > >> > * TODO: check if a value change has occured to avoid superfluous writes > >> > */ > >> > if (attributes & EFI_VARIABLE_NON_VOLATILE) > >> > - efi_var_to_file(); > >> > + ret = efi_var_to_file(); > >> > >> The discussion here should focus on the treatment of errors in the > >> file-system. > >> > >> The following error cases come to my mind: > >> > >> * ESP partition is missing > >> * ESP FAT file system is corrupted > >> * There is no space on the ESP. > >> * The medium (e.g. SD card) is in write only mode > >> > >> The current code gives priority to enable booting in all adverse > >> situations. Do you think this is a bad choice? > >> > >> @Ilias: Please, chime in. > > > >I think we should continue doing that *unless* preseeding is enabled. > >If we preseed authenticated variables and the restoration of the > >switching of the secure boot mode fails we must stop the EFI > >subsystem. > >The first function that gets called is efi_init_variables(). > >commit 77bb14758dcb1 explains some of the logic in that file but in short > >- We load variables from a file first, excluding authenticated ones > >- We load preseeded variables. That step allows you to load PK, KEK, > >db etc that are stored as part of the u-boot binary. We assume that > >in a sane design a chain of trust *will* authenticate u-boot prior to > >loading, so we 'trust' those variables. If this fails which includes > >setting the secure state we correctly stop the EFI subsystem > >- efi_var_restore() is checking for duplicates. So a non-authenticated > >variable set by the file load won't be overwritten by built-in ones. > >This allows users to override built-in variables (apart from auth ones > >obviously) > > > >Cheers > >/Ilias > > > > > > > > > > > > > >> > >> Best regards > >> > >> Heinrich > >> > >> > > >> > - return EFI_SUCCESS; > >> > + return ret; > >> > } > >> > > >> > efi_status_t efi_query_variable_info_int(u32 attributes, > >>
diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c index 62e071bd83..dbb9b1f3fc 100644 --- a/lib/efi_loader/efi_var_file.c +++ b/lib/efi_loader/efi_var_file.c @@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void) log_err("Invalid EFI variables file\n"); error: free(buf); + return ret; #endif return EFI_SUCCESS; } diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index be95ed44e6..13966297c6 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -350,17 +350,17 @@ efi_status_t efi_set_variable_int(const u16 *variable_name, if (var_type == EFI_AUTH_VAR_PK) ret = efi_init_secure_state(); - else - ret = EFI_SUCCESS; + if (ret != EFI_SUCCESS) + return ret; /* * Write non-volatile EFI variables to file * TODO: check if a value change has occured to avoid superfluous writes */ if (attributes & EFI_VARIABLE_NON_VOLATILE) - efi_var_to_file(); + ret = efi_var_to_file(); - return EFI_SUCCESS; + return ret; } efi_status_t efi_query_variable_info_int(u32 attributes,
Correct some UEFI variable error handing code paths. Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> --- lib/efi_loader/efi_var_file.c | 1 + lib/efi_loader/efi_variable.c | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-)