REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1235
When update contents in HiiDatabase, like:
1. Add/update/remove package list
2. Add/update string
3. Add/update image
We should make these operations atomic to prevent the
potential issue that the one update operation with
higher TPL may interrupt another.
This commit is to make the HiiDatabase update behaviors
atomic by adding EfiAcquireLock/EfiReleaseLock function.
Cc: Liming Gao <liming.gao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1235
Function "HiiGetConfigurationSetting" may be called
when HiiDatabase contents have been updated.
And it is just to call function "HiiGetDatabaseInfo" to
get the contents in HiiDatabase and call function
"HiiGetConfigRespInfo" and get the configuration response
string form HII drivers.
So here we can remove function "HiiGetConfigurationSetting"
and make caller to call "HiiGetDatabaseInfo" and
"HiiGetConfigRespInfo" directly.
And thus it also can distinguish which code blocks are to
operate HiiDatabase contents and which code blocks are not.
Then it's easy to know which code blocks should be atomic
when updating HiiDatabase contents.
Cc: Liming Gao <liming.gao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1255
For function ReadFile():
If the line
Status = GetAedAdsData (
...
);
is reached multiple times during the 'for' loop, freeing the data pointed
by variable 'Data' may potentially lead to variable 'Ad' referencing the
already-freed data.
After calling function GetAllocationDescriptor(), 'Data' and 'Ad' may
point to the same memory (with some possible offset). Hence, this commit
will move the FreePool() call backwards to ensure the data will no longer
be used.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Acked-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1254
This commit will add an additional check within function GetPdFromLongAd()
when getting a Partition Descriptor from given a Long Allocation
Descriptor.
According to UDF 2.60 Spec, Section 2.2.13:
> The partition reference numbers used are determined by the order of the
> Partition Maps in the LVD.
(Also the picture comes before the above contents)
And a more detailed explanation of the partition reference numbers is at
https://sites.google.com/site/udfintro/ (seems not a formal documentation
though), Section 5.3.6.
Based on the above findings, the 'PartitionReferenceNumber' field in a
Long Allocation Descriptor is used as an index to access the Partition
Maps data within a Logical Volume Descriptor.
Hence, the new check focuses on the validity of this
'PartitionReferenceNumber' field in a Long Allocation Descriptor. Since
the current implementation of UdfDxe driver supports only one partition on
a Logical Volume, so the value of 'PartitionReferenceNumber' should be 0.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Acked-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1175
This commit will update the UdfGetInfo() function with the support of
EFI_FILE_SYSTEM_VOLUME_LABEL data information request.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Acked-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1253
Within function SetFileInfo():
This commit will fix a typo where 'Minute' should be used instead of
'Second'.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Acked-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1252
Calling the 'SetPosition' service of the EFI_FILE_PROTOCOL with 'Position'
equals to 0xFFFFFFFFFFFFFFFF for a file is to set the current position to
the end of the file. But the current implementation of function
UdfSetPosition() is to set it to the last byte (not EOF).
This commit will resolve this issue.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Acked-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=828
Within GetVolumeSize():
The boundary check will validate the 'NumberOfPartitions' field of a
Logical Volume Integrity Descriptor matches the data within the relating
Logical Volume Descriptor.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Acked-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=828
Within ResolveSymlink():
The boundary check will validate the 'LengthofComponentIdentifier' field
of a Path Component matches the data within the relating (Extended) File
Entry.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Acked-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=828
Within ReadFile():
Add checks to ensure that when getting the raw data or the Allocation
Descriptors' data from a FE/EFE, it will not consume data beyond the
size of a FE/EFE.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Acked-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=828
The commit refines the boundary checks for file/path name string to
prevent possible buffer overrun.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Acked-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=828
Within FindAnchorVolumeDescriptorPointer():
Add a check for the underlying device block size to ensure it is greater
than the size of an Anchor Volume Descriptor Pointer.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Acked-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1249
We found potential dead codes within File.c during the code coverage test.
After manual review, we think the below ones are positive reports:
A. For function GetAllocationDescriptor():
Due to the all the calling places for this function, the input parameter
'RecordingFlags' can only with value 'LongAdsSequence' or
'ShortAdsSequence'. Moreover, this is also mentioned in the function
description comments for GetAllocationDescriptor().
So the below code will never be reached:
return EFI_DEVICE_ERROR;
This commit will add "ASSERT (FALSE);" before the above line to indicate
this and thus matching the function description comments.
B. For function GetAllocationDescriptorLsn():
Due to the all the calling places for this function, the input parameter
'RecordingFlags' can only with value 'LongAdsSequence' or
'ShortAdsSequence'. Moreover, this is also mentioned in the function
description comments for GetAllocationDescriptorLsn().
So the below code will never be reached:
return EFI_UNSUPPORTED;
This commit will add "ASSERT (FALSE);" before the above line to indicate
this and thus matching the function description comments.
Cc: Paulo Alcantara <palcantara@suse.de>
Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1249
We found potential dead codes within File.c during the code coverage test.
After manual review, we think the below ones are positive reports:
A. In function UdfRead():
else if (IS_FID_DELETED_FILE (Parent->FileIdentifierDesc)) {
Status = EFI_DEVICE_ERROR;
}
A File Identifier Descriptor will be get from the UDF media only by
function ReadDirectoryEntry(). And within this function, all the File
Identifier Descriptor with 'DELETED_FILE' characteristics will be skipped
and will not be returned. Hence, the above codes in function UdfRead()
will never be hit.
This commit will add "ASSERT (FALSE);" before the above line to indicate
this.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Reviewed-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1248
Within function UdfOpen():
This commit will use debug messages instead of using ASSERT when an error
occurs after calling GetFileSize().
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Reviewed-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1248
This commit will refine the ASSERTs in function GetLongAdLsn() and
GetAllocationDescriptorLsn() when the logical sector number cannot be
returned due to unrecognized media format.
Error handling logic will be used for those ASSERTs.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Reviewed-by: Star Zeng <star.zeng@intel.com>
This commit adds ASSERTs to address false positive reports of NULL
pointer dereference issues raised from static analysis with regard to
function ReadDirectoryEntry().
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Reviewed-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1247
For functions DuplicateFid() and DuplicateFe(), this commit will use error
handling logic instead of ASSERTs for memory allocation failure.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Reviewed-by: Star Zeng <star.zeng@intel.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=415
When SetVariable() to a time based auth variable with APPEND_WRITE
attribute, and if the EFI_VARIABLE_AUTHENTICATION_2.TimeStamp in
the input Data is earlier than current value, it will cause timestamp
zeroing.
This issue may bring time based auth variable downgrade problem.
For example:
A vendor released three certs at 2014, 2015, and 2016, and system
integrated the 2016 cert. User can SetVariable() with 2015 cert and
APPEND_WRITE attribute to cause timestamp zeroing first, then
SetVariable() with 2014 cert to downgrade the cert.
This patch fixes this issue.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng <star.zeng@intel.com>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
This is the part of commit 3948c510ed.
Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dongao Guo <dongao.guo@intel.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
Set macro for variable arguments, and remove the hard code definition
to avoid the potential duplicated definition.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao <liming.gao@intel.com>
Reviewed-by: Dongao Guo <dongao.guo@intel.com>
There are five check not necessary in logic ,just for pass static
analysis. More detail please refer to comment in code.
And the rest changes are all accepted by owner, they are reasonable.
Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dongao Guo <dongao.guo@intel.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1245
In current implementation and according to the description in
MdeModulePkg.dec, PcdSetNvStoreDefaultId should be set in PEI
phase to take effect.
This patch ASSERTs PcdSetNvStoreDefaultId set in PcdDxe to alert
the invalid operation.
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng <star.zeng@intel.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
There are three warnings reported by GCC 4.8 and the later GCC release
are workaround with them.
And all the three warnings are invalid,so I just disable warnings rather
than fix them at now.
Following is the analysis from Laszlo Ersek.
(1)
> MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regcomp.c: In
> function 'compile_length_tree':
> MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regcomp.c:1516:7:
> warning: 'len' may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> int len;
> ^
I think this is an invalid warning; the type of the controlling expression
(node->type) is enum GimmickType, and the case labels cover all values of
the enum. An assert(0) could be added, I guess, but again, upstream
Oniguruma would be justified to reject the idea.
(2)
> MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c: In
> function 'parse_callout_args.isra.10.constprop.30':
> MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c:6753:25:
> warning: 'rl' may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> vals[n].l = rl;
> ^
This warning is invalid, given:
6749 if (cn > 0) {
6750 long rl;
6751 r = parse_long(enc, buf, bufend, 1, LONG_MAX, &rl);
6752 if (r == ONIG_NORMAL) {
6753 vals[n].l = rl;
Because parse_long() only returns ONIG_NORMAL after it sets (*rl).
(3)
> MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c: In
> function 'parse_callout_of_name.constprop.29':
> MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c:6861:38:
> warning: 'tag_end' may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> if (! is_allowed_callout_tag_name(enc, tag_start, tag_end))
This warning is also invalid, given:
6852 if (c == '[') {
6853 if (PEND) return ONIGERR_END_PATTERN_IN_GROUP;
6854 tag_start = p;
6855 while (! PEND) {
6856 if (PEND) return ONIGERR_END_PATTERN_IN_GROUP;
6857 tag_end = p;
6858 PFETCH_S(c);
6859 if (c == ']') break;
6860 }
6861 if (! is_allowed_callout_tag_name(enc, tag_start, tag_end))
6862 return ONIGERR_INVALID_CALLOUT_TAG_NAME;
6863
To see that, first we should note:
#define PEND (p < end ? 0 : 1)
therefore PEND doesn't change if neither "p" nor "end" change.
Second, when we reach line 6855 (the "while") for the very first time,
(!PEND) is certainly true (i.e., PEND is false), because otherwise we
would have bailed at line 6853. Therefore we reach line 6857, and assign
"tag_end". Regardless of whether we iterate zero or more *additional*
times around the loop, "tag_end" will have been set, whenever we reach
line 6861.
Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dongao Guo <dongao.guo@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1224
When covert IFR binary to EFI_IFR_CHECKBOX structure,
Current code has following incorrect code logic:
IfrCheckBox = (EFI_IFR_CHECKBOX *) (IfrOpHdr + 1);
The correct one should be:
IfrCheckBox = (EFI_IFR_CHECKBOX *) IfrOpHdr;
1. The bug is in function "UpdateDefaultSettingInFormPackage()"
which is to update the default setting of some HII Questions
in the IFR binary data. So it only has impact when platform
overrides default setting in HII VarStore through DynamicHii
PCD setting in Platform DSC file. If platform doesn't
override default setting, it has no impact.
2. The implementation updates the "Flags" filed in the
EFI_IFR_CHECKBOX structure to update the default
setting of checkbox.
If using "IfrCheckBox = (EFI_IFR_CHECKBOX *) (IfrOpHdr + 1);"
when wants to update the " Flags" filed in checkbox,
but in fact it will update the opcode binary
data(opcode binary length) behind checkbox binary.
And then it will cause Browser can't parse the IFR
binary data correctly. And then the possible symptom
is that some HII Question and forms may be not parsed
and then cannot be shown.
This patch is to fix this bug.
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194
Speculative execution is used by processor to avoid having to wait for
data to arrive from memory, or for previous operations to finish, the
processor may speculate as to what will be executed.
If the speculation is incorrect, the speculatively executed instructions
might leave hints such as which memory locations have been brought into
cache. Malicious actors can use the bounds check bypass method (code
gadgets with controlled external inputs) to infer data values that have
been used in speculative operations to reveal secrets which should not
otherwise be accessed.
This commit will focus on the SMI handler(s) registered within the
Variable\RuntimeDxe driver and insert AsmLfence API to mitigate the
bounds check bypass issue.
For SMI handler SmmVariableHandler():
Under "case SMM_VARIABLE_FUNCTION_GET_VARIABLE:",
'SmmVariableHeader->NameSize' can be a potential cross boundary access of
the 'CommBuffer' (controlled external input) during speculative execution.
This cross boundary access is later used as the index to access array
'SmmVariableHeader->Name' by code:
"SmmVariableHeader->Name[SmmVariableHeader->NameSize/sizeof (CHAR16) - 1]"
One can observe which part of the content within array was brought into
cache to possibly reveal the value of 'SmmVariableHeader->NameSize'.
Hence, this commit adds a AsmLfence() after the boundary/range checks of
'CommBuffer' to prevent the speculative execution.
And there are 2 similar cases under
"case SMM_VARIABLE_FUNCTION_SET_VARIABLE:" and
"case SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET:" as well.
This commits also handles them.
Also, under "case SMM_VARIABLE_FUNCTION_SET_VARIABLE:",
'(UINT8 *)SmmVariableHeader->Name + SmmVariableHeader->NameSize' points to
the 'CommBuffer' (with some offset) and then passed as parameter 'Data' to
function VariableServiceSetVariable().
Within function VariableServiceSetVariable(), there is a sanity check for
EFI_VARIABLE_AUTHENTICATION_2 descriptor for the data pointed by 'Data'.
If this check is speculatively bypassed, potential cross-boundary data
access for 'Data' is possible to be revealed via the below function calls
sequence during speculative execution:
AuthVariableLibProcessVariable()
ProcessVarWithPk() or ProcessVarWithKek()
Within function ProcessVarWithPk() or ProcessVarWithKek(), for the code
"PayloadSize = DataSize - AUTHINFO2_SIZE (Data);", 'AUTHINFO2_SIZE (Data)'
can be a cross boundary access during speculative execution.
Then, 'PayloadSize' is possible to be revealed by the function call
sequence:
AuthServiceInternalUpdateVariableWithTimeStamp()
mAuthVarLibContextIn->UpdateVariable()
VariableExLibUpdateVariable()
UpdateVariable()
CopyMem()
Hence, this commit adds a AsmLfence() after the sanity check for
EFI_VARIABLE_AUTHENTICATION_2 descriptor upon 'Data' within function
VariableServiceSetVariable() to prevent the speculative execution.
Also, please note that the change made within function
VariableServiceSetVariable() will affect DXE as well. However, since we
only focuses on the SMM codes, the commit will introduce a new module
internal function called VariableLoadFence() to handle this. This internal
function will have 2 implementations (1 for SMM, 1 for DXE). For the SMM
implementation, it is a wrapper to call the AsmLfence() API; for the DXE
implementation, it is empty.
A more detailed explanation of the purpose of commit is under the
'Bounds check bypass mitigation' section of the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation
And the document at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf
Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194
Speculative execution is used by processor to avoid having to wait for
data to arrive from memory, or for previous operations to finish, the
processor may speculate as to what will be executed.
If the speculation is incorrect, the speculatively executed instructions
might leave hints such as which memory locations have been brought into
cache. Malicious actors can use the bounds check bypass method (code
gadgets with controlled external inputs) to infer data values that have
been used in speculative operations to reveal secrets which should not
otherwise be accessed.
This commit will focus on the SMI handler(s) registered within the
SmmLockBox driver and insert AsmLfence API to mitigate the
bounds check bypass issue.
For SMI handler SmmLockBoxHandler():
Under "case EFI_SMM_LOCK_BOX_COMMAND_SAVE:", the 'CommBuffer' (controlled
external inputs) is passed to function SmmLockBoxSave().
'TempLockBoxParameterSave.Length' can be a potential cross boundary access
of the 'CommBuffer' during speculative execution. This cross boundary
access is later passed as parameter 'Length' into function SaveLockBox().
Within function SaveLockBox(), the value of 'Length' can be inferred by
code:
"CopyMem ((VOID *)(UINTN)SmramBuffer, (VOID *)(UINTN)Buffer, Length);".
One can observe which part of the content within 'Buffer' was brought into
cache to possibly reveal the value of 'Length'.
Hence, this commit adds a AsmLfence() after the boundary/range checks of
'CommBuffer' to prevent the speculative execution.
And there is a similar case under "case EFI_SMM_LOCK_BOX_COMMAND_UPDATE:"
function SmmLockBoxUpdate() as well. This commits also handles it.
A more detailed explanation of the purpose of commit is under the
'Bounds check bypass mitigation' section of the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation
And the document at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194
Speculative execution is used by processor to avoid having to wait for
data to arrive from memory, or for previous operations to finish, the
processor may speculate as to what will be executed.
If the speculation is incorrect, the speculatively executed instructions
might leave hints such as which memory locations have been brought into
cache. Malicious actors can use the bounds check bypass method (code
gadgets with controlled external inputs) to infer data values that have
been used in speculative operations to reveal secrets which should not
otherwise be accessed.
This commit will focus on the SMI handler(s) registered within the
FaultTolerantWriteDxe driver and insert AsmLfence API to mitigate the
bounds check bypass issue.
For SMI handler SmmFaultTolerantWriteHandler():
Under "case FTW_FUNCTION_WRITE:", 'SmmFtwWriteHeader->Length' can be a
potential cross boundary access of the 'CommBuffer' (controlled external
inputs) during speculative execution. This cross boundary access is later
passed as parameter 'Length' into function FtwWrite().
Within function FtwWrite(), the value of 'Length' can be inferred by code:
"CopyMem (MyBuffer + Offset, Buffer, Length);". One can observe which part
of the content within 'Buffer' was brought into cache to possibly reveal
the value of 'Length'.
Hence, this commit adds a AsmLfence() after the boundary/range checks of
'CommBuffer' to prevent the speculative execution.
A more detailed explanation of the purpose of commit is under the
'Bounds check bypass mitigation' section of the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation
And the document at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
The PCD below is unused, so it has been removed from inf.
gEfiMdeModulePkgTokenSpaceGuid.PcdExtFpdtBootRecordPadSize
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: shenglei <shenglei.zhang@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1214
This patch only updates comment and function description, so has
no functionality impact.
This patch fixes comment typo 'end' to 'start' in GetStartPointer.
GetStartPointer for PEI and DXE has aligned function description,
but GetEndPointer does not.
This patch also aligns GetEndPointer's function description for
PEI and DXE.
Cc: Liming Gao <liming.gao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng <star.zeng@intel.com>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
https://bugzilla.tianocore.org/show_bug.cgi?id=967
Request to add a library function for GetAcpiTable() in order
to get ACPI table using signature as input.
After evaluation, we found there are many duplicated code to
find ACPI table by signature in different modules.
This patch updates S3SaveStateDxe to use new
EfiLocateFirstAcpiTable() and remove the duplicated code.
Cc: Younas khan <pmdyounaskhan786@gmail.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng <star.zeng@intel.com>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=886
This patch is to support the TFTP windowsize option described in RFC 7440.
The feature allows the client and server to negotiate a window size of
consecutive blocks to send as an alternative for replacing the single-block
lockstep schema.
Currently, the windowsize for write request operation is not supported since
there is no real use cases.
Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Shao Ming <ming.shao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>
Reviewed-by: Ye Ting <ting.ye@intel.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=996
Issue:
In current code logic, when a key is pressed, it will search
the whole NotifyList to find whether a notification has been
registered with the keystroke. if yes, it will en-queue the
key for notification execution later. And now if different
notification functions have been registered with the same key,
then the key will be en-queued more than once. Then it will
cause the notification executed more than once.
This patch is to enhance the code logic to fix this issue.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
v2: use "IP & Netmask" directly instead of defining an additional variable.
This patch is to sync the direct route entry setting in both the default
and Instance route table {Subnet, Mask, NextHope} (
https://bugzilla.tianocore.org/show_bug.cgi?id=1143).
Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
Reviewed-by: Ye Ting <ting.ye@intel.com>
Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>
Today's implementation does an exact device path match to check
whether the device path of a console is in ConIn/ConOut/ErrOut.
But that doesn't work for the USB keyboard.
Because when a platform have multiple USB port, ConIn needs to
carry all device paths corresponding to each port.
Even worse, today's BDS core logic removes the device path from
ConIn/ConOut/ErrOut when the connection to that device path fails.
So if user switches the USB keyboard from one port to another across
boot, the USB keyboard doesn't work in the second boot.
ConPlatform driver solved this problem by adding the
IsHotPlugDevice() function. So that for USB keyboard, ConPlatform
doesn't care whether its device path is in ConIn or not.
But the rule is too loose, and now causes platform BDS cannot control
whether to enable USB keyboard as an active console.
The patch changes ConPlatform to support USB short-form device path
when checking whether the device path of a console is in
ConIn/ConOut/ErrOut.
The logic to always accept USB/PCCARD device as active console is
removed.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
Removing rules for Ipf sources file:
* Remove the source file which path with "ipf" and also listed in
[Sources.IPF] section of INF file.
* Remove the source file which listed in [Components.IPF] section
of DSC file and not listed in any other [Components] section.
* Remove the embedded Ipf code for MDE_CPU_IPF.
Removing rules for Inf file:
* Remove IPF from VALID_ARCHITECTURES comments.
* Remove DXE_SAL_DRIVER from LIBRARY_CLASS in [Defines] section.
* Remove the INF which only listed in [Components.IPF] section in DSC.
* Remove statements from [BuildOptions] that provide IPF specific flags.
* Remove any IPF sepcific sections.
Removing rules for Dec file:
* Remove [Includes.IPF] section from Dec.
Removing rules for Dsc file:
* Remove IPF from SUPPORTED_ARCHITECTURES in [Defines] section of DSC.
* Remove any IPF specific sections.
* Remove statements from [BuildOptions] that provide IPF specific flags.
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chen A Chen <chen.a.chen@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1115
When the type of HiiValue is EFI_IFR_TYPE_BUFFER,
its question type is EFI_IFR_ORDERED_LIST_OP.
And the buffer size allocated for Statement->BufferValue
of orderedList is "Statement->StorageWidth"
in IfrParse.c.
So here when backup the buffer value and copy the size of
"Statement->StorageWidth + sizeof(CHAR16)" is incorrect.
This patch is to fix this issue.
Cc: Eric Dong <eric.dong@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
Reviewed-by: Eric Dong <eric.dong@intel.com>
Liming Gao
Dandan Bi
Hao Wu
Star Zeng
Dongao Guo
Jiaxin Wu
shenglei
Ruiyu Ni
Chen A Chen