Topics

[PATCH edk2 v1 1/1] MdeModulePkg/Variable: Move FindVariable after AutoUpdateLangVariable

Ming Huang
 

When occur reclaim in AutoUpdateLangVariable(), the CurrPtr of
Variable is invalid. The State will be update with wrong position
after UpdateVariable in this situation and two valid PlatformLang or
Lang variables will exist. BmForEachVariable() will enter endless loop
while exist two valid PlatformLang variables. So FindVariable() should
be invoked atfer AutoUpdateLangVariable().

https://bugzilla.tianocore.org/show_bug.cgi?id=2667

Signed-off-by: Ming Huang <huangming23@...>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 26 ++++++++++----------
1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 1e71fc6..0cec981 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -2741,6 +2741,19 @@ VariableServiceSetVariable (
mVariableModuleGlobal->NonVolatileLastVariableOffset = (UINTN) NextVariable - (UINTN) Point;
}

+ if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) {
+ //
+ // Hook the operation of setting PlatformLangCodes/PlatformLang and LangCodes/Lang.
+ //
+ Status = AutoUpdateLangVariable (VariableName, Data, DataSize);
+ if (EFI_ERROR (Status)) {
+ //
+ // The auto update operation failed, directly return to avoid inconsistency between PlatformLang and Lang.
+ //
+ goto Done;
+ }
+ }
+
//
// Check whether the input variable is already existed.
//
@@ -2763,19 +2776,6 @@ VariableServiceSetVariable (
}
}

- if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) {
- //
- // Hook the operation of setting PlatformLangCodes/PlatformLang and LangCodes/Lang.
- //
- Status = AutoUpdateLangVariable (VariableName, Data, DataSize);
- if (EFI_ERROR (Status)) {
- //
- // The auto update operation failed, directly return to avoid inconsistency between PlatformLang and Lang.
- //
- goto Done;
- }
- }
-
if (mVariableModuleGlobal->VariableGlobal.AuthSupport) {
Status = AuthVariableLibProcessVariable (VariableName, VendorGuid, Data, DataSize, Attributes);
} else {
--
2.8.1

Guomin Jiang
 

Hi Huang,

From issue statement, I guess that
1. AutoUpdateLangVariable() invoked, and it will invoke FindVariable() first, at the same time, reclaim occur and Variable.CurrPtr is invalid, it return with success
2. UpdateVariable() is invoked when The old Lang's State is valid and the new Lang's State is also valid.
3. In the situation, FindVariable() checked Lang's State and only enable one Lang's State. But it didn't in fact.
4. BmForEachVariable() deadloop in the situation.

Am I right?

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ming
Huang via groups.io
Sent: Monday, June 29, 2020 2:06 PM
To: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@...>; Wu, Hao A
<hao.a.wu@...>; Gao, Liming <liming.gao@...>
Cc: lidongzhan@...; huangming23@...;
songdongkuang@...; wanghuiqiang@...;
qiuliangen@...; shenlimei@...; @xiewenyi
Subject: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable: Move
FindVariable after AutoUpdateLangVariable

When occur reclaim in AutoUpdateLangVariable(), the CurrPtr of Variable is
invalid. The State will be update with wrong position after UpdateVariable in
this situation and two valid PlatformLang or Lang variables will exist.
BmForEachVariable() will enter endless loop while exist two valid
PlatformLang variables. So FindVariable() should be invoked atfer
AutoUpdateLangVariable().

https://bugzilla.tianocore.org/show_bug.cgi?id=2667

Signed-off-by: Ming Huang <huangming23@...>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 26
++++++++++----------
1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 1e71fc6..0cec981 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -2741,6 +2741,19 @@ VariableServiceSetVariable (
mVariableModuleGlobal->NonVolatileLastVariableOffset = (UINTN)
NextVariable - (UINTN) Point;
}

+ if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) {
+ //
+ // Hook the operation of setting PlatformLangCodes/PlatformLang and
LangCodes/Lang.
+ //
+ Status = AutoUpdateLangVariable (VariableName, Data, DataSize);
+ if (EFI_ERROR (Status)) {
+ //
+ // The auto update operation failed, directly return to avoid
inconsistency between PlatformLang and Lang.
+ //
+ goto Done;
+ }
+ }
+
//
// Check whether the input variable is already existed.
//
@@ -2763,19 +2776,6 @@ VariableServiceSetVariable (
}
}

- if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) {
- //
- // Hook the operation of setting PlatformLangCodes/PlatformLang and
LangCodes/Lang.
- //
- Status = AutoUpdateLangVariable (VariableName, Data, DataSize);
- if (EFI_ERROR (Status)) {
- //
- // The auto update operation failed, directly return to avoid inconsistency
between PlatformLang and Lang.
- //
- goto Done;
- }
- }
-
if (mVariableModuleGlobal->VariableGlobal.AuthSupport) {
Status = AuthVariableLibProcessVariable (VariableName, VendorGuid,
Data, DataSize, Attributes);
} else {
--
2.8.1


Ming Huang
 

在 2020/6/30 8:58, Jiang, Guomin 写道:
Hi Huang,

From issue statement, I guess that
1. AutoUpdateLangVariable() invoked, and it will invoke FindVariable() first, at the same time, reclaim occur and Variable.CurrPtr is invalid, it return with success
2. UpdateVariable() is invoked when The old Lang's State is valid and the new Lang's State is also valid.
3. In the situation, FindVariable() checked Lang's State and only enable one Lang's State. But it didn't in fact.
4. BmForEachVariable() deadloop in the situation.

Am I right?
Yes, right.

Thanks,
Ming


-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ming
Huang via groups.io
Sent: Monday, June 29, 2020 2:06 PM
To: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@...>; Wu, Hao A
<hao.a.wu@...>; Gao, Liming <liming.gao@...>
Cc: lidongzhan@...; huangming23@...;
songdongkuang@...; wanghuiqiang@...;
qiuliangen@...; shenlimei@...; @xiewenyi
Subject: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable: Move
FindVariable after AutoUpdateLangVariable

When occur reclaim in AutoUpdateLangVariable(), the CurrPtr of Variable is
invalid. The State will be update with wrong position after UpdateVariable in
this situation and two valid PlatformLang or Lang variables will exist.
BmForEachVariable() will enter endless loop while exist two valid
PlatformLang variables. So FindVariable() should be invoked atfer
AutoUpdateLangVariable().

https://bugzilla.tianocore.org/show_bug.cgi?id=2667

Signed-off-by: Ming Huang <huangming23@...>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 26
++++++++++----------
1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 1e71fc6..0cec981 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -2741,6 +2741,19 @@ VariableServiceSetVariable (
mVariableModuleGlobal->NonVolatileLastVariableOffset = (UINTN)
NextVariable - (UINTN) Point;
}

+ if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) {
+ //
+ // Hook the operation of setting PlatformLangCodes/PlatformLang and
LangCodes/Lang.
+ //
+ Status = AutoUpdateLangVariable (VariableName, Data, DataSize);
+ if (EFI_ERROR (Status)) {
+ //
+ // The auto update operation failed, directly return to avoid
inconsistency between PlatformLang and Lang.
+ //
+ goto Done;
+ }
+ }
+
//
// Check whether the input variable is already existed.
//
@@ -2763,19 +2776,6 @@ VariableServiceSetVariable (
}
}

- if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) {
- //
- // Hook the operation of setting PlatformLangCodes/PlatformLang and
LangCodes/Lang.
- //
- Status = AutoUpdateLangVariable (VariableName, Data, DataSize);
- if (EFI_ERROR (Status)) {
- //
- // The auto update operation failed, directly return to avoid inconsistency
between PlatformLang and Lang.
- //
- goto Done;
- }
- }
-
if (mVariableModuleGlobal->VariableGlobal.AuthSupport) {
Status = AuthVariableLibProcessVariable (VariableName, VendorGuid,
Data, DataSize, Attributes);
} else {
--
2.8.1



Guomin Jiang
 

So I think the key point is why AutoUpdateLangVariable() return success rather than fail, if is it reasonable for this case or we need other error handing?

I am glad to help you but I can't reproduce it until now, can you provide a step to reproduce it in simulation platform.

If it is urgent, I suggest that discuss with your internal team first and explain that we need consider the risk check it into edk2.

Best Regards
Guomin

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ming
Huang via groups.io
Sent: Tuesday, June 30, 2020 8:26 PM
To: Jiang, Guomin <guomin.jiang@...>; devel@edk2.groups.io; Wang,
Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Gao,
Liming <liming.gao@...>
Cc: lidongzhan@...; songdongkuang@...;
wanghuiqiang@...; qiuliangen@...;
shenlimei@...; @xiewenyi
Subject: Re: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable:
Move FindVariable after AutoUpdateLangVariable



在 2020/6/30 8:58, Jiang, Guomin 写道:
Hi Huang,

From issue statement, I guess that
1. AutoUpdateLangVariable() invoked, and it will invoke FindVariable()
first, at the same time, reclaim occur and Variable.CurrPtr is invalid, it return
with success 2. UpdateVariable() is invoked when The old Lang's State is valid
and the new Lang's State is also valid.
3. In the situation, FindVariable() checked Lang's State and only enable one
Lang's State. But it didn't in fact.
4. BmForEachVariable() deadloop in the situation.

Am I right?
Yes, right.

Thanks,
Ming


-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ming
Huang via groups.io
Sent: Monday, June 29, 2020 2:06 PM
To: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@...>; Wu,
Hao A <hao.a.wu@...>; Gao, Liming <liming.gao@...>
Cc: lidongzhan@...; huangming23@...;
songdongkuang@...; wanghuiqiang@...;
qiuliangen@...; shenlimei@...;
@xiewenyi
Subject: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable:
Move
FindVariable after AutoUpdateLangVariable

When occur reclaim in AutoUpdateLangVariable(), the CurrPtr of
Variable is invalid. The State will be update with wrong position
after UpdateVariable in this situation and two valid PlatformLang or Lang
variables will exist.
BmForEachVariable() will enter endless loop while exist two valid
PlatformLang variables. So FindVariable() should be invoked atfer
AutoUpdateLangVariable().

https://bugzilla.tianocore.org/show_bug.cgi?id=2667

Signed-off-by: Ming Huang <huangming23@...>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 26
++++++++++----------
1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 1e71fc6..0cec981 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -2741,6 +2741,19 @@ VariableServiceSetVariable (
mVariableModuleGlobal->NonVolatileLastVariableOffset = (UINTN)
NextVariable - (UINTN) Point;
}

+ if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) {
+ //
+ // Hook the operation of setting PlatformLangCodes/PlatformLang
+ and
LangCodes/Lang.
+ //
+ Status = AutoUpdateLangVariable (VariableName, Data, DataSize);
+ if (EFI_ERROR (Status)) {
+ //
+ // The auto update operation failed, directly return to avoid
inconsistency between PlatformLang and Lang.
+ //
+ goto Done;
+ }
+ }
+
//
// Check whether the input variable is already existed.
//
@@ -2763,19 +2776,6 @@ VariableServiceSetVariable (
}
}

- if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) {
- //
- // Hook the operation of setting PlatformLangCodes/PlatformLang and
LangCodes/Lang.
- //
- Status = AutoUpdateLangVariable (VariableName, Data, DataSize);
- if (EFI_ERROR (Status)) {
- //
- // The auto update operation failed, directly return to avoid
inconsistency
between PlatformLang and Lang.
- //
- goto Done;
- }
- }
-
if (mVariableModuleGlobal->VariableGlobal.AuthSupport) {
Status = AuthVariableLibProcessVariable (VariableName,
VendorGuid, Data, DataSize, Attributes);
} else {
--
2.8.1




Ming Huang
 

在 2020/7/1 8:22, Jiang, Guomin 写道:
So I think the key point is why AutoUpdateLangVariable() return success rather than fail, if is it reasonable for this case or we need other error handing?
I don't think AutoUpdateLangVariable() should return fail while occur reclaim internal in AutoUpdateLangVariable ()
function. The problem is the Variable(VARIABLE_POINTER_TRACK) get by FindVariable is invald in this situation and
this Variable will be pass to UpdateVariable().

if (mVariableModuleGlobal->VariableGlobal.AuthSupport) {
Status = AuthVariableLibProcessVariable (VariableName, VendorGuid, Data, DataSize, Attributes);
} else {
// This Variable is invald while occur reclaim internal in AutoUpdateLangVariable ()
Status = UpdateVariable (VariableName, VendorGuid, Data, DataSize, Attributes, 0, 0, &Variable, NULL);
}


I am glad to help you but I can't reproduce it until now, can you provide a step to reproduce it in simulation platform.
I am not familiar with simulation platform. We reproduct this issue in our board once.
For accelerating reproduction this issue, Add Reclaim() to AutoUpdateLangVariable() for test.

Thanks,
Ming


If it is urgent, I suggest that discuss with your internal team first and explain that we need consider the risk check it into edk2.

Best Regards
Guomin

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ming
Huang via groups.io
Sent: Tuesday, June 30, 2020 8:26 PM
To: Jiang, Guomin <guomin.jiang@...>; devel@edk2.groups.io; Wang,
Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Gao,
Liming <liming.gao@...>
Cc: lidongzhan@...; songdongkuang@...;
wanghuiqiang@...; qiuliangen@...;
shenlimei@...; @xiewenyi
Subject: Re: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable:
Move FindVariable after AutoUpdateLangVariable



在 2020/6/30 8:58, Jiang, Guomin 写道:
Hi Huang,

From issue statement, I guess that
1. AutoUpdateLangVariable() invoked, and it will invoke FindVariable()
first, at the same time, reclaim occur and Variable.CurrPtr is invalid, it return
with success 2. UpdateVariable() is invoked when The old Lang's State is valid
and the new Lang's State is also valid.
3. In the situation, FindVariable() checked Lang's State and only enable one
Lang's State. But it didn't in fact.
4. BmForEachVariable() deadloop in the situation.

Am I right?
Yes, right.

Thanks,
Ming


-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ming
Huang via groups.io
Sent: Monday, June 29, 2020 2:06 PM
To: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@...>; Wu,
Hao A <hao.a.wu@...>; Gao, Liming <liming.gao@...>
Cc: lidongzhan@...; huangming23@...;
songdongkuang@...; wanghuiqiang@...;
qiuliangen@...; shenlimei@...;
@xiewenyi
Subject: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable:
Move
FindVariable after AutoUpdateLangVariable

When occur reclaim in AutoUpdateLangVariable(), the CurrPtr of
Variable is invalid. The State will be update with wrong position
after UpdateVariable in this situation and two valid PlatformLang or Lang
variables will exist.
BmForEachVariable() will enter endless loop while exist two valid
PlatformLang variables. So FindVariable() should be invoked atfer
AutoUpdateLangVariable().

https://bugzilla.tianocore.org/show_bug.cgi?id=2667

Signed-off-by: Ming Huang <huangming23@...>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 26
++++++++++----------
1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 1e71fc6..0cec981 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -2741,6 +2741,19 @@ VariableServiceSetVariable (
mVariableModuleGlobal->NonVolatileLastVariableOffset = (UINTN)
NextVariable - (UINTN) Point;
}

+ if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) {
+ //
+ // Hook the operation of setting PlatformLangCodes/PlatformLang
+ and
LangCodes/Lang.
+ //
+ Status = AutoUpdateLangVariable (VariableName, Data, DataSize);
+ if (EFI_ERROR (Status)) {
+ //
+ // The auto update operation failed, directly return to avoid
inconsistency between PlatformLang and Lang.
+ //
+ goto Done;
+ }
+ }
+
//
// Check whether the input variable is already existed.
//
@@ -2763,19 +2776,6 @@ VariableServiceSetVariable (
}
}

- if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) {
- //
- // Hook the operation of setting PlatformLangCodes/PlatformLang and
LangCodes/Lang.
- //
- Status = AutoUpdateLangVariable (VariableName, Data, DataSize);
- if (EFI_ERROR (Status)) {
- //
- // The auto update operation failed, directly return to avoid
inconsistency
between PlatformLang and Lang.
- //
- goto Done;
- }
- }
-
if (mVariableModuleGlobal->VariableGlobal.AuthSupport) {
Status = AuthVariableLibProcessVariable (VariableName,
VendorGuid, Data, DataSize, Attributes);
} else {
--
2.8.1





.