REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1417
Since BaseLib API AsmLfence() is a x86 arch specific API and should be
avoided using in generic codes, this commit replaces the usage of
AsmLfence() with arch-generic API SpeculationBarrier().
Please note that speculation execution barriers are intended to be
asserted for SMM codes, hence, this commit still preserve an empty
implementation of the speculation execution barrier for the DXE codes.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1417
Since BaseLib API AsmLfence() is a x86 arch specific API and should be
avoided using in generic codes, this commit replaces the usage of
AsmLfence() with arch-generic API SpeculationBarrier().
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1417
Since BaseLib API AsmLfence() is a x86 arch specific API and should be
avoided using in generic codes, this commit replaces the usage of
AsmLfence() with arch-generic API SpeculationBarrier().
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1278
This patch is to delete the UefiPxeBcDxe driver in MdeModulePkg. The
driver will not be maintained and can't co-work with the dual-stack
UefiPxeBcDxe in NetworkPkg.
People should use below NetworkPkg drivers instead:
NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
Which is actively maintained with more bug fixes and new feature support.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1278
This patch is to delete the IScsiDxe driver in MdeModulePkg. The driver
will not be maintained and can't co-work with the dual-stack IScsiDxe in
NetworkPkg.
People should use below NetworkPkg drivers instead:
NetworkPkg/IScsiDxe/IScsiDxe.inf
Which is actively maintained with more bug fixes and new feature support.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1278
This patch is to delete the Tcp4Dxe driver in MdeModulePkg. The driver
will not be maintained and can't co-work with the dual-stack TcpDxe in
NetworkPkg.
People should use below NetworkPkg drivers instead:
NetworkPkg/TcpDxe/TcpDxe.inf
Which is actively maintained with more bug fixes and new feature support.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
https://bugzilla.tianocore.org/show_bug.cgi?id=1356
Current PcdVpdBaseAddress is 32bit static Pcd. NON SPI platform needs to
configure it as Dynamic PCD. Emulator platform (such as NT32) may set its
value to 64bit address.
To meet with this usage, 64bit DynamicEx PcdVpdBaseAddress64 is introduced.
If its value is not zero, it will be used.
If its value is zero, static PcdVpdBaseAddress will be used.
When NON SPI platform enables VPD PCD, they need to set PcdVpdBaseAddress64.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Print debug messages if size of the VariableName plus DataSize exceeds
Max(Auth|Voltaile)VariableSize bytes. The messages will be useful if any
platform specific value of Max(Auth|Voltaile)VariableSize PCDs have to
be changed.
Cc: Star Zeng <star.zeng@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Vijayenthiran Subramaniam <vijayenthiran.subramaniam@arm.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
In function UiDisplayMenu, the NewPos ptr which used to point to the
highlight menu entry. It will always point to the menu entry which
need to be highlighted or the gMenuOption menu if the highlight menu
is not found.
So we can remove the NULL ptr check for NewPos in this function.
And add the ASSERT code to avoid if any false positive reports
of NULL pointer dereference issue raised from static analysis.
Cc: Liming Gao <liming.gao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
This reverts commit 8cd4e734cc.
It is not a real bug fix. It should not be pushed after
Hard Feature Freeze for edk2-stable201811 tag.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao <liming.gao@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Reviewed-by: Bi Dandan <dandan.bi@intel.com>
In function UiDisplayMenu, the NewPos ptr which used to point to the
highlight menu entry. It will always point to the menu entry which
need to be highlighted or the gMenuOption menu if the highlight menu
is not found.
So we can remove the NULL ptr check for NewPos in this function.
And add the ASSERT code to avoid if any false positive reports
of NULL pointer dereference issue raised from static analysis.
Cc: Liming Gao <liming.gao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
This patch is to fix the invalid setting of MTFTP local port. The
issue can be reproduced by tftp shell command by using [-l port]
option.
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: Fu Siyuan <siyuan.fu@intel.com>
The recent changes in these three source files introduce the trailing space.
This patch removes them to follow edk2 coding style.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1279
For function ResolveSymlink(), the below codes:
if (CompareMem ((VOID *)&PreviousFile, (VOID *)Parent,
sizeof (UDF_FILE_INFO)) != 0) {
CleanupFileInformation (&PreviousFile);
}
CopyMem ((VOID *)&PreviousFile, (VOID *)File, sizeof (UDF_FILE_INFO));
If the contents in 'PreviousFile' and 'File' are the same, call to
"CleanupFileInformation (&PreviousFile);" will free the buffers in 'File'
as well. This will lead to potential memory double free/use after free
issues.
This commit will add additional check to address the above 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>
Reviewed-by: Star Zeng <star.zeng@intel.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1279
The content within 'File' is the output data for ResolveSymlink(). This
commit will add checks to ensure the content in 'File' is valid.
Otherwise, possible null pointer dereference issue will occur during the
subsequent usage of the data returned by ResolveSymlink().
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>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1279
According to the ECMA-167 standard (3rd Edition - June 1997), Section
14.16.1.1, valid values are 1 to 5. All other values will be treated as a
corrupted volume.
This commit will add such check within function ResolveSymlink().
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>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
The block returned from Mtftp4RemoveBlockNum is not the total received and
saved block number if it works in passive (Slave) mode.
The issue was exposed by the EMS test.
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>
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>
Hao Wu
Siyuan Fu
Liming Gao
Vijayenthiran Subramaniam
Dandan Bi
Jiaxin Wu
Star Zeng
Dongao Guo