| Message ID | 20171003171727.5641-1-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [edk2] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname | expand |
Hi Ard, To me, the ASSERT there seems on purpose to help catch the misuse of that interface. Could you share the case you met the ASSERT? Given that interface is an open API of UefiBootManagerLib, some comments for the behavior of ASSERT may can be added to be more clear. Cc Ruiyu who is the expert of this part code. Thanks, Star -----Original Message----- From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] Sent: Wednesday, October 4, 2017 1:17 AM To: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com> Cc: leif.lindholm@linaro.org; Ard Biesheuvel <ard.biesheuvel@linaro.org> Subject: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname When invoking EfiBootManagerIsValidLoadOptionVariableName() with the variable name 'BootNext', we should simply return FALSE, given that it is not an indexed Boot#### load option. However, in DEBUG mode, we will hit an assert in BmCharToUint() and crash. So remove the assert. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c | 1 - 1 file changed, 1 deletion(-) diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c index 11ab86792a52..a3fa25424592 100644 --- a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c @@ -420,7 +420,6 @@ BmCharToUint ( return (Char - L'A' + 0xA); } - ASSERT (FALSE); return (UINTN) -1; } -- 2.11.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 4 October 2017 at 00:56, Zeng, Star <star.zeng@intel.com> wrote: > Hi Ard, > > To me, the ASSERT there seems on purpose to help catch the misuse of that interface. > Could you share the case you met the ASSERT? > When using the 'fwupdate' Linux tool to perform capsule updates, BootNext is set to the id of the Boot### variable it creates to run fwupx64.efi, which executes in UEFI context. I haven't looked in great detail how exactly the code ends up calling this function on L"BootNext", but the ASSERT () is wrong, because it is called on variable names that are modifiable externally. For example, if I create a variable Boot000@ from the UEFI Shell, the firmware should not crash. > Given that interface is an open API of UefiBootManagerLib, some comments for the behavior of ASSERT may can be added to be more clear. > I still think the assert should be removed. > Cc Ruiyu who is the expert of this part code. > Thanks, Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
I agree. If creating Boot000@ can hit this ASSERT, this ASSERT must be removed. Error handling must be used to handle such case. Thank you Yao Jiewen From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel Sent: Wednesday, October 4, 2017 8:00 AM To: Zeng, Star <star.zeng@intel.com> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; leif.lindholm@linaro.org Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname On 4 October 2017 at 00:56, Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> wrote: > Hi Ard, > > To me, the ASSERT there seems on purpose to help catch the misuse of that interface. > Could you share the case you met the ASSERT? > When using the 'fwupdate' Linux tool to perform capsule updates, BootNext is set to the id of the Boot### variable it creates to run fwupx64.efi, which executes in UEFI context. I haven't looked in great detail how exactly the code ends up calling this function on L"BootNext", but the ASSERT () is wrong, because it is called on variable names that are modifiable externally. For example, if I create a variable Boot000@ from the UEFI Shell, the firmware should not crash. > Given that interface is an open API of UefiBootManagerLib, some comments for the behavior of ASSERT may can be added to be more clear. > I still think the assert should be removed. > Cc Ruiyu who is the expert of this part code. > Thanks, Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Creating Boot000@ with gEfiGlobalVariableGuid can not succeed as it will be rejected by MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf that will check the VariableName against UEFI spec "Table 13. Global Variables" if the VendorGuid is gEfiGlobalVariableGuid. I would suspect there is a bug at other place if the code ends up calling this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext". Ard, Is the fix urgent or not for you? I may want to wait for Ruiyu's back to take some look at the detail of it. At the same time, you may help check the code flow in some detail if you have free time, I think that will be helpful. :) Thanks, Star From: Yao, Jiewen Sent: Wednesday, October 4, 2017 8:18 AM To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Zeng, Star <star.zeng@intel.com> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; leif.lindholm@linaro.org Subject: RE: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname I agree. If creating Boot000@ can hit this ASSERT, this ASSERT must be removed. Error handling must be used to handle such case. Thank you Yao Jiewen From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel Sent: Wednesday, October 4, 2017 8:00 AM To: Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; leif.lindholm@linaro.org<mailto:leif.lindholm@linaro.org> Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname On 4 October 2017 at 00:56, Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> wrote: > Hi Ard, > > To me, the ASSERT there seems on purpose to help catch the misuse of that interface. > Could you share the case you met the ASSERT? > When using the 'fwupdate' Linux tool to perform capsule updates, BootNext is set to the id of the Boot### variable it creates to run fwupx64.efi, which executes in UEFI context. I haven't looked in great detail how exactly the code ends up calling this function on L"BootNext", but the ASSERT () is wrong, because it is called on variable names that are modifiable externally. For example, if I create a variable Boot000@ from the UEFI Shell, the firmware should not crash. > Given that interface is an open API of UefiBootManagerLib, some comments for the behavior of ASSERT may can be added to be more clear. > I still think the assert should be removed. > Cc Ruiyu who is the expert of this part code. > Thanks, Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 4 October 2017 at 14:49, Zeng, Star <star.zeng@intel.com> wrote: > Creating Boot000@ with gEfiGlobalVariableGuid can not succeed as it will be > rejected by MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf that > will check the VariableName against UEFI spec “Table 13. Global Variables” > if the VendorGuid is gEfiGlobalVariableGuid. > > > > I would suspect there is a bug at other place if the code ends up calling > this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext". > That still does not mean you should ASSERT() here. The state of the variable store != the internals of the code, and so it should be considered external input to some extent. ASSERTs are meant to catch programming errors, not errors in the varstore image. > > Ard, > > Is the fix urgent or not for you? > Not really. But fwupdate is shipping as part of many distros, so I guess others may run into it as well. > I may want to wait for Ruiyu’s back to take some look at the detail of it. > That is fine. > At the same time, you may help check the code flow in some detail if you > have free time, I think that will be helpful. J > OK.
Thanks for confirming the urgency. I have no strong motivation to keep/remove the ASSERT, I would like Ruiyu to argue and make the decision. I mainly want the issue (the code ends up calling this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext") could be root caused. Thanks, Star -----Original Message----- From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] Sent: Wednesday, October 4, 2017 9:54 PM To: Zeng, Star <star.zeng@intel.com> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; leif.lindholm@linaro.org Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname On 4 October 2017 at 14:49, Zeng, Star <star.zeng@intel.com> wrote: > Creating Boot000@ with gEfiGlobalVariableGuid can not succeed as it > will be rejected by > MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf that will check the VariableName against UEFI spec “Table 13. Global Variables” > if the VendorGuid is gEfiGlobalVariableGuid. > > > > I would suspect there is a bug at other place if the code ends up > calling this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext". > That still does not mean you should ASSERT() here. The state of the variable store != the internals of the code, and so it should be considered external input to some extent. ASSERTs are meant to catch programming errors, not errors in the varstore image. > > Ard, > > Is the fix urgent or not for you? > Not really. But fwupdate is shipping as part of many distros, so I guess others may run into it as well. > I may want to wait for Ruiyu’s back to take some look at the detail of it. > That is fine. > At the same time, you may help check the code flow in some detail if > you have free time, I think that will be helpful. J > OK.
On 10/04/17 15:54, Ard Biesheuvel wrote: > On 4 October 2017 at 14:49, Zeng, Star <star.zeng@intel.com> wrote: >> Creating Boot000@ with gEfiGlobalVariableGuid can not succeed as it will be >> rejected by MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf that >> will check the VariableName against UEFI spec “Table 13. Global Variables” >> if the VendorGuid is gEfiGlobalVariableGuid. >> >> >> >> I would suspect there is a bug at other place if the code ends up calling >> this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext". >> > > That still does not mean you should ASSERT() here. The state of the > variable store != the internals of the code, and so it should be > considered external input to some extent. At least under some circumstances, I disagree with this. The assumption that the variable store can be written only by privileged firmware routines is core to Secure Boot, for example. > ASSERTs are meant to catch > programming errors, not errors in the varstore image. I agree. However, as a corollary to the above, if said "privileged routines" are supposed to catch all invalid inputs passed to gRT->SetVariable(), then the rest of the firmware is right to assume that the contents of the variable store are valid. If it is found invalid at some point, then it is indeed due to a programming error (somewhere in the gRT->SetVariable() machinery, that is), so the ASSERT() is justified. Another example in support of this argument is the Fault Tolerant Write machinery -- the firmware tries very hard to recover from power loss during a varstore update. If, on reboot, the error proves non-recoverable (i.e. we cannot even roll back to a previous pristine state), then that can be considered a bug (or design error) in FTW. That said, I agree with the patch. BmCharToUint() explicitly documents "conversion failed" as a return condition, and both functions that call BmCharToUint(), namely EfiBootManagerIsValidLoadOptionVariableName() and BmIsKeyOptionVariable(), check for that return condition. Thanks, Laszlo
On 4 October 2017 at 15:40, Laszlo Ersek <lersek@redhat.com> wrote: > On 10/04/17 15:54, Ard Biesheuvel wrote: >> On 4 October 2017 at 14:49, Zeng, Star <star.zeng@intel.com> wrote: >>> Creating Boot000@ with gEfiGlobalVariableGuid can not succeed as it will be >>> rejected by MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf that >>> will check the VariableName against UEFI spec “Table 13. Global Variables” >>> if the VendorGuid is gEfiGlobalVariableGuid. >>> >>> >>> >>> I would suspect there is a bug at other place if the code ends up calling >>> this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext". >>> >> >> That still does not mean you should ASSERT() here. The state of the >> variable store != the internals of the code, and so it should be >> considered external input to some extent. > > At least under some circumstances, I disagree with this. The assumption > that the variable store can be written only by privileged firmware > routines is core to Secure Boot, for example. > That is true. But the firmware that wrote to the varstore may be a different version from the one reading it. >> ASSERTs are meant to catch >> programming errors, not errors in the varstore image. > > I agree. > > However, as a corollary to the above, if said "privileged routines" are > supposed to catch all invalid inputs passed to gRT->SetVariable(), then > the rest of the firmware is right to assume that the contents of the > variable store are valid. If it is found invalid at some point, then it > is indeed due to a programming error (somewhere in the > gRT->SetVariable() machinery, that is), so the ASSERT() is justified. > > Another example in support of this argument is the Fault Tolerant Write > machinery -- the firmware tries very hard to recover from power loss > during a varstore update. If, on reboot, the error proves > non-recoverable (i.e. we cannot even roll back to a previous pristine > state), then that can be considered a bug (or design error) in FTW. > > > That said, I agree with the patch. BmCharToUint() explicitly documents > "conversion failed" as a return condition, and both functions that call > BmCharToUint(), namely EfiBootManagerIsValidLoadOptionVariableName() and > BmIsKeyOptionVariable(), check for that return condition. > Thanks, Ard.
Star, On 10/04/17 16:09, Zeng, Star wrote: > Thanks for confirming the urgency. > > I have no strong motivation to keep/remove the ASSERT, I would like Ruiyu to argue and make the decision. > I mainly want the issue (the code ends up calling this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext") could be root caused. it might be interesting to find out about the exact call stack. However, I'd like to point out that the exact purpose of the EfiBootManagerIsValidLoadOptionVariableName() function is to check *whether* the variable name is a valid boot option name or not. If not -- for whatever reason -- then it shouldn't ASSERT(); it should just return FALSE. Perhaps it's relevant: the function was made public in commit 3dc5c1ae5c757. Thanks Laszlo > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: Wednesday, October 4, 2017 9:54 PM > To: Zeng, Star <star.zeng@intel.com> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; leif.lindholm@linaro.org > Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname > > On 4 October 2017 at 14:49, Zeng, Star <star.zeng@intel.com> wrote: >> Creating Boot000@ with gEfiGlobalVariableGuid can not succeed as it >> will be rejected by >> MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf that will check the VariableName against UEFI spec “Table 13. Global Variables” >> if the VendorGuid is gEfiGlobalVariableGuid. >> >> >> >> I would suspect there is a bug at other place if the code ends up >> calling this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext". >> > > That still does not mean you should ASSERT() here. The state of the variable store != the internals of the code, and so it should be considered external input to some extent. ASSERTs are meant to catch programming errors, not errors in the varstore image. > > >> >> Ard, >> >> Is the fix urgent or not for you? >> > > Not really. But fwupdate is shipping as part of many distros, so I guess others may run into it as well. > >> I may want to wait for Ruiyu’s back to take some look at the detail of it. >> > > That is fine. > >> At the same time, you may help check the code flow in some detail if >> you have free time, I think that will be helpful. J >> > > OK. > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >
I got your point.
From literal meaning of the API, I agree the ASSERT should be removed.
If the input parameter is assumed to be valid always, the API could be not called at all.
If the input parameter is not assumed to be valid always, the API should not assert.
I just tried an experiment and can easily reproduce the assert.
1. Boot NT32 to shell.
2. Create L"BootNext" variable with shell command: setvar BootNext -NV -RT -BS =0000.
3. Reboot and then ASSERT.
ASSERT!: [BdsDxe] i:\git\edk2git\edk2\MdeModulePkg\Library\UefiBootManagerLib\BmMisc.c (423): ((BOOLEAN)(0==1))
The calling stack is:
BdsEntry(BdsEntry.c L844) ->
EfiBootManagerGetLoadOptions(BdsLoadOption.c L1092) ->
BmCollectLoadOptions() with L"BootNext" from the loop in BmForEachVariable() ->
EfiBootManagerIsValidLoadOptionVariableName() ->
BmCharToUint() ->
ASSERT(FALSE)
The assert seems new caused by 0e6584e38650cef9a6b4579553679c0f12d897bc as L"BootNext" was deleted before calling EfiBootManagerGetLoadOptions() when no this commit.
Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Wednesday, October 4, 2017 11:06 PM
To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; leif.lindholm@linaro.org; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname
Star,
On 10/04/17 16:09, Zeng, Star wrote:
> Thanks for confirming the urgency.
>
> I have no strong motivation to keep/remove the ASSERT, I would like Ruiyu to argue and make the decision.
> I mainly want the issue (the code ends up calling this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext") could be root caused.
it might be interesting to find out about the exact call stack. However, I'd like to point out that the exact purpose of the
EfiBootManagerIsValidLoadOptionVariableName() function is to check
*whether* the variable name is a valid boot option name or not. If not
-- for whatever reason -- then it shouldn't ASSERT(); it should just return FALSE.
Perhaps it's relevant: the function was made public in commit 3dc5c1ae5c757.
Thanks
Laszlo
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Wednesday, October 4, 2017 9:54 PM
> To: Zeng, Star <star.zeng@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong, Eric
> <eric.dong@intel.com>; leif.lindholm@linaro.org
> Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't
> ASSERT on 'BootNext' varname
>
> On 4 October 2017 at 14:49, Zeng, Star <star.zeng@intel.com> wrote:
>> Creating Boot000@ with gEfiGlobalVariableGuid can not succeed as it
>> will be rejected by
>> MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf that will check the VariableName against UEFI spec “Table 13. Global Variables”
>> if the VendorGuid is gEfiGlobalVariableGuid.
>>
>>
>>
>> I would suspect there is a bug at other place if the code ends up
>> calling this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext".
>>
>
> That still does not mean you should ASSERT() here. The state of the variable store != the internals of the code, and so it should be considered external input to some extent. ASSERTs are meant to catch programming errors, not errors in the varstore image.
>
>
>>
>> Ard,
>>
>> Is the fix urgent or not for you?
>>
>
> Not really. But fwupdate is shipping as part of many distros, so I guess others may run into it as well.
>
>> I may want to wait for Ruiyu’s back to take some look at the detail of it.
>>
>
> That is fine.
>
>> At the same time, you may help check the code flow in some detail if
>> you have free time, I think that will be helpful. J
>>
>
> OK.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
On 10/05/17 09:31, Zeng, Star wrote: > I got your point. > From literal meaning of the API, I agree the ASSERT should be removed. > If the input parameter is assumed to be valid always, the API could be not called at all. > If the input parameter is not assumed to be valid always, the API should not assert. > > I just tried an experiment and can easily reproduce the assert. > 1. Boot NT32 to shell. > 2. Create L"BootNext" variable with shell command: setvar BootNext -NV -RT -BS =0000. > 3. Reboot and then ASSERT. > ASSERT!: [BdsDxe] i:\git\edk2git\edk2\MdeModulePkg\Library\UefiBootManagerLib\BmMisc.c (423): ((BOOLEAN)(0==1)) > > The calling stack is: > BdsEntry(BdsEntry.c L844) -> > EfiBootManagerGetLoadOptions(BdsLoadOption.c L1092) -> > BmCollectLoadOptions() with L"BootNext" from the loop in BmForEachVariable() -> > EfiBootManagerIsValidLoadOptionVariableName() -> > BmCharToUint() -> > ASSERT(FALSE) > > The assert seems new caused by 0e6584e38650cef9a6b4579553679c0f12d897bc as L"BootNext" was deleted before calling EfiBootManagerGetLoadOptions() when no this commit. Ah, good point! OK, so let's wait until Ray acks the removal of the assert. Thanks! Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Wednesday, October 4, 2017 11:06 PM > To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; leif.lindholm@linaro.org; Yao, Jiewen <jiewen.yao@intel.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname > > Star, > > On 10/04/17 16:09, Zeng, Star wrote: >> Thanks for confirming the urgency. >> >> I have no strong motivation to keep/remove the ASSERT, I would like Ruiyu to argue and make the decision. >> I mainly want the issue (the code ends up calling this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext") could be root caused. > > it might be interesting to find out about the exact call stack. However, I'd like to point out that the exact purpose of the > EfiBootManagerIsValidLoadOptionVariableName() function is to check > *whether* the variable name is a valid boot option name or not. If not > -- for whatever reason -- then it shouldn't ASSERT(); it should just return FALSE. > > Perhaps it's relevant: the function was made public in commit 3dc5c1ae5c757. > > Thanks > Laszlo > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: Wednesday, October 4, 2017 9:54 PM >> To: Zeng, Star <star.zeng@intel.com> >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu >> <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong, Eric >> <eric.dong@intel.com>; leif.lindholm@linaro.org >> Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't >> ASSERT on 'BootNext' varname >> >> On 4 October 2017 at 14:49, Zeng, Star <star.zeng@intel.com> wrote: >>> Creating Boot000@ with gEfiGlobalVariableGuid can not succeed as it >>> will be rejected by >>> MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf that will check the VariableName against UEFI spec “Table 13. Global Variables” >>> if the VendorGuid is gEfiGlobalVariableGuid. >>> >>> >>> >>> I would suspect there is a bug at other place if the code ends up >>> calling this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext". >>> >> >> That still does not mean you should ASSERT() here. The state of the variable store != the internals of the code, and so it should be considered external input to some extent. ASSERTs are meant to catch programming errors, not errors in the varstore image. >> >> >>> >>> Ard, >>> >>> Is the fix urgent or not for you? >>> >> >> Not really. But fwupdate is shipping as part of many distros, so I guess others may run into it as well. >> >>> I may want to wait for Ruiyu’s back to take some look at the detail of it. >>> >> >> That is fine. >> >>> At the same time, you may help check the code flow in some detail if >>> you have free time, I think that will be helpful. J >>> >> >> OK. >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel >> >
Thank you star. I ack this update. I recall we did a review for bds on variable usage and assert usage and fixed such issue. If this is a regression, we probable need review again. thank you! Yao, Jiewen > 在 2017年10月5日，下午3:59，Laszlo Ersek <lersek@redhat.com> 写道： > >> On 10/05/17 09:31, Zeng, Star wrote: >> I got your point. >> From literal meaning of the API, I agree the ASSERT should be removed. >> If the input parameter is assumed to be valid always, the API could be not called at all. >> If the input parameter is not assumed to be valid always, the API should not assert. >> >> I just tried an experiment and can easily reproduce the assert. >> 1. Boot NT32 to shell. >> 2. Create L"BootNext" variable with shell command: setvar BootNext -NV -RT -BS =0000. >> 3. Reboot and then ASSERT. >> ASSERT!: [BdsDxe] i:\git\edk2git\edk2\MdeModulePkg\Library\UefiBootManagerLib\BmMisc.c (423): ((BOOLEAN)(0==1)) >> >> The calling stack is: >> BdsEntry(BdsEntry.c L844) -> >> EfiBootManagerGetLoadOptions(BdsLoadOption.c L1092) -> >> BmCollectLoadOptions() with L"BootNext" from the loop in BmForEachVariable() -> >> EfiBootManagerIsValidLoadOptionVariableName() -> >> BmCharToUint() -> >> ASSERT(FALSE) >> >> The assert seems new caused by 0e6584e38650cef9a6b4579553679c0f12d897bc as L"BootNext" was deleted before calling EfiBootManagerGetLoadOptions() when no this commit. > > Ah, good point! > > OK, so let's wait until Ray acks the removal of the assert. > > Thanks! > Laszlo > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Wednesday, October 4, 2017 11:06 PM >> To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; leif.lindholm@linaro.org; Yao, Jiewen <jiewen.yao@intel.com> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname >> >> Star, >> >>> On 10/04/17 16:09, Zeng, Star wrote: >>> Thanks for confirming the urgency. >>> >>> I have no strong motivation to keep/remove the ASSERT, I would like Ruiyu to argue and make the decision. >>> I mainly want the issue (the code ends up calling this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext") could be root caused. >> >> it might be interesting to find out about the exact call stack. However, I'd like to point out that the exact purpose of the >> EfiBootManagerIsValidLoadOptionVariableName() function is to check >> *whether* the variable name is a valid boot option name or not. If not >> -- for whatever reason -- then it shouldn't ASSERT(); it should just return FALSE. >> >> Perhaps it's relevant: the function was made public in commit 3dc5c1ae5c757. >> >> Thanks >> Laszlo >> >>> -----Original Message----- >>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >>> Sent: Wednesday, October 4, 2017 9:54 PM >>> To: Zeng, Star <star.zeng@intel.com> >>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu >>> <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong, Eric >>> <eric.dong@intel.com>; leif.lindholm@linaro.org >>> Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't >>> ASSERT on 'BootNext' varname >>> >>>> On 4 October 2017 at 14:49, Zeng, Star <star.zeng@intel.com> wrote: >>>> Creating Boot000@ with gEfiGlobalVariableGuid can not succeed as it >>>> will be rejected by >>>> MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf that will check the VariableName against UEFI spec “Table 13. Global Variables” >>>> if the VendorGuid is gEfiGlobalVariableGuid. >>>> >>>> >>>> >>>> I would suspect there is a bug at other place if the code ends up >>>> calling this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext". >>>> >>> >>> That still does not mean you should ASSERT() here. The state of the variable store != the internals of the code, and so it should be considered external input to some extent. ASSERTs are meant to catch programming errors, not errors in the varstore image. >>> >>> >>>> >>>> Ard, >>>> >>>> Is the fix urgent or not for you? >>>> >>> >>> Not really. But fwupdate is shipping as part of many distros, so I guess others may run into it as well. >>> >>>> I may want to wait for Ruiyu’s back to take some look at the detail of it. >>>> >>> >>> That is fine. >>> >>>> At the same time, you may help check the code flow in some detail if >>>> you have free time, I think that will be helpful. J >>>> >>> >>> OK. >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> >> >
All, I just sent out two patches. One to remove the assertion, the other to fix a bug in EfiBootManagerIsValidLoadOptionVariableName. Thanks/Ray > -----Original Message----- > From: Yao, Jiewen > Sent: Thursday, October 5, 2017 5:08 PM > To: Laszlo Ersek <lersek@redhat.com> > Cc: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel > <ard.biesheuvel@linaro.org>; Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric > <eric.dong@intel.com>; edk2-devel@lists.01.org; leif.lindholm@linaro.org > Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT > on 'BootNext' varname > > Thank you star. > > I ack this update. > > I recall we did a review for bds on variable usage and assert usage and fixed such > issue. If this is a regression, we probable need review again. > > thank you! > Yao, Jiewen > > > > 在 2017年10月5日，下午3:59，Laszlo Ersek <lersek@redhat.com> 写道： > > > >> On 10/05/17 09:31, Zeng, Star wrote: > >> I got your point. > >> From literal meaning of the API, I agree the ASSERT should be removed. > >> If the input parameter is assumed to be valid always, the API could be not > called at all. > >> If the input parameter is not assumed to be valid always, the API should not > assert. > >> > >> I just tried an experiment and can easily reproduce the assert. > >> 1. Boot NT32 to shell. > >> 2. Create L"BootNext" variable with shell command: setvar BootNext -NV -RT > -BS =0000. > >> 3. Reboot and then ASSERT. > >> ASSERT!: [BdsDxe] > >> i:\git\edk2git\edk2\MdeModulePkg\Library\UefiBootManagerLib\BmMisc.c > >> (423): ((BOOLEAN)(0==1)) > >> > >> The calling stack is: > >> BdsEntry(BdsEntry.c L844) -> > >> EfiBootManagerGetLoadOptions(BdsLoadOption.c L1092) -> > >> BmCollectLoadOptions() with L"BootNext" from the loop in > BmForEachVariable() -> > >> EfiBootManagerIsValidLoadOptionVariableName() -> > >> BmCharToUint() -> > >> ASSERT(FALSE) > >> > >> The assert seems new caused by > 0e6584e38650cef9a6b4579553679c0f12d897bc as L"BootNext" was deleted > before calling EfiBootManagerGetLoadOptions() when no this commit. > > > > Ah, good point! > > > > OK, so let's wait until Ray acks the removal of the assert. > > > > Thanks! > > Laszlo > > > >> -----Original Message----- > >> From: Laszlo Ersek [mailto:lersek@redhat.com] > >> Sent: Wednesday, October 4, 2017 11:06 PM > >> To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel > >> <ard.biesheuvel@linaro.org> > >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; > >> edk2-devel@lists.01.org; leif.lindholm@linaro.org; Yao, Jiewen > >> <jiewen.yao@intel.com> > >> Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't > >> ASSERT on 'BootNext' varname > >> > >> Star, > >> > >>> On 10/04/17 16:09, Zeng, Star wrote: > >>> Thanks for confirming the urgency. > >>> > >>> I have no strong motivation to keep/remove the ASSERT, I would like Ruiyu > to argue and make the decision. > >>> I mainly want the issue (the code ends up calling this > function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext") > could be root caused. > >> > >> it might be interesting to find out about the exact call stack. > >> However, I'd like to point out that the exact purpose of the > >> EfiBootManagerIsValidLoadOptionVariableName() function is to check > >> *whether* the variable name is a valid boot option name or not. If > >> not > >> -- for whatever reason -- then it shouldn't ASSERT(); it should just return > FALSE. > >> > >> Perhaps it's relevant: the function was made public in commit 3dc5c1ae5c757. > >> > >> Thanks > >> Laszlo > >> > >>> -----Original Message----- > >>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > >>> Sent: Wednesday, October 4, 2017 9:54 PM > >>> To: Zeng, Star <star.zeng@intel.com> > >>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu > >>> <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong, Eric > >>> <eric.dong@intel.com>; leif.lindholm@linaro.org > >>> Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't > >>> ASSERT on 'BootNext' varname > >>> > >>>> On 4 October 2017 at 14:49, Zeng, Star <star.zeng@intel.com> wrote: > >>>> Creating Boot000@ with gEfiGlobalVariableGuid can not succeed as it > >>>> will be rejected by > >>>> MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf that will > check the VariableName against UEFI spec “Table 13. Global Variables” > >>>> if the VendorGuid is gEfiGlobalVariableGuid. > >>>> > >>>> > >>>> > >>>> I would suspect there is a bug at other place if the code ends up > >>>> calling this function(EfiBootManagerIsValidLoadOptionVariableName) on > L"BootNext". > >>>> > >>> > >>> That still does not mean you should ASSERT() here. The state of the variable > store != the internals of the code, and so it should be considered external input > to some extent. ASSERTs are meant to catch programming errors, not errors in > the varstore image. > >>> > >>> > >>>> > >>>> Ard, > >>>> > >>>> Is the fix urgent or not for you? > >>>> > >>> > >>> Not really. But fwupdate is shipping as part of many distros, so I guess > others may run into it as well. > >>> > >>>> I may want to wait for Ruiyu’s back to take some look at the detail of it. > >>>> > >>> > >>> That is fine. > >>> > >>>> At the same time, you may help check the code flow in some detail > >>>> if you have free time, I think that will be helpful. J > >>>> > >>> > >>> OK. > >>> _______________________________________________ > >>> edk2-devel mailing list > >>> edk2-devel@lists.01.org > >>> https://lists.01.org/mailman/listinfo/edk2-devel > >>> > >> > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c index 11ab86792a52..a3fa25424592 100644 --- a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c @@ -420,7 +420,6 @@ BmCharToUint ( return (Char - L'A' + 0xA); } - ASSERT (FALSE); return (UINTN) -1; }
When invoking EfiBootManagerIsValidLoadOptionVariableName() with the variable name 'BootNext', we should simply return FALSE, given that it is not an indexed Boot#### load option. However, in DEBUG mode, we will hit an assert in BmCharToUint() and crash. So remove the assert. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c | 1 - 1 file changed, 1 deletion(-) -- 2.11.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel