[PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Ard Biesheuvel]

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

Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Zeng, Star]

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

Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Ard Biesheuvel]

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.

Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Yao, Jiewen]

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

Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Zeng, Star]

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

Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Ard Biesheuvel]

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.

Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Zeng, Star]

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.

Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Laszlo Ersek]

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
> 

Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Zeng, Star]

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
> 

Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Laszlo Ersek]

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
>>
> 

Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Yao, Jiewen]

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
>>> 
>> 
> 

Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Ni, Ruiyu]

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
> >>>
> >>
> >

Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Laszlo Ersek]

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

Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Ard Biesheuvel]

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.