MdeModulePkg/PciBusDxe: Improve the flow of testing support attributes #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
https://bugzilla.tianocore.org/show_bug.cgi?id=3635 Currently, in order to test the supported attributes, the PciTestSupportedAttribute() will set the command register to 0x27 (EFI_PCI_COMMAND_IO_SPACE, EFI_PCI_COMMAND_MEMORY_SPACE, EFI_PCI_COMMAND_BUS_MASTER, EFI_PCI_COMMAND_VGA_PALETTE_SNOOP) firstly, and then read back to check whether these attributes are set successfully in the device. This will cause the other enabled bits (other than EFI_PCI_COMMAND_IO_SPACE,EFI_PCI_COMMAND_MEMORY_SPACE, EFI_PCI_COMMAND_BUS_MASTER,EFI_PCI_COMMAND_VGA_PALETTE_SNOOP) be cleared for a short of time This patch fixes this issue by keeping the origina enabled bits when setting 0x27. Signed-off-by: xueshengfeng <xueshengfeng@byosoft.com.cn> Reviewed-by: Ray <ray.ni@intel.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
niruiyu
pushed a commit
that referenced
this pull request
Nov 23, 2023
Root cause: 1. Before DisableReadonlyPageWriteProtect() is called, the return address (#1) is pushed in shadow stack. 2. CET is disabled. 3. DisableReadonlyPageWriteProtect() returns to #1. 4. Page table is modified. 5. EnableReadonlyPageWriteProtect() is called, but the return address (#2) is not pushed in shadow stack. 6. CET is enabled. 7. EnableReadonlyPageWriteProtect() returns to #2. #CP exception happens because the actual return address (#2) doesn't match the return address stored in shadow stack (#1). Analysis: Shadow stack will stop update after CET disable (DisableCet() in DisableReadOnlyPageWriteProtect), but normal smi stack will be continue updated with the function called and return (DisableReadOnlyPageWriteProtect & EnableReadOnlyPageWriteProtect), thus leading stack mismatch after CET re-enabled (EnableCet() in EnableReadOnlyPageWriteProtect). According SDM Vol 3, 6.15-Control Protection Exception: Normal smi stack and shadow stack must be matched when CET enable, otherwise CP Exception will happen, which is caused by a near RET instruction. CET is disabled in DisableCet(), while can be enabled in EnableCet(). This way won't cause the problem because they are implemented in a way that return address of DisableCet() is poped out from shadow stack (Incsspq performs a pop to increases the shadow stack) and EnableCet() doesn't use "RET" but "JMP" to return to caller. So calling EnableCet() and DisableCet() doesn't have the same issue as calling DisableReadonlyPageWriteProtect() and EnableReadonlyPageWriteProtect(). With above root cause & analysis, define below 2 macros instead of functions for WP & CET operation: WRITE_UNPROTECT_RO_PAGES (Wp, Cet) WRITE_PROTECT_RO_PAGES (Wp, Cet) Because DisableCet() & EnableCet() must be in the same function to avoid shadow stack and normal SMI stack mismatch. Note: WRITE_UNPROTECT_RO_PAGES () must be called pair with WRITE_PROTECT_RO_PAGES () in same function. Change-Id: I4e126697efcd8dbfb4887da034d8691bfca969e3 Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Zeng Star <star.zeng@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com>
niruiyu
pushed a commit
that referenced
this pull request
Apr 10, 2024
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4534 Bug Details: PixieFail Bug #1 CVE-2023-45229 CVSS 6.5 : CVSS:3.1/AV:A/AC:L/PR:N/UI:N/S:U/C:H/I:N/A:N CWE-125 Out-of-bounds Read Change Overview: Introduce Dhcp6SeekInnerOptionSafe which performs checks before seeking the Inner Option from a DHCP6 Option. > > EFI_STATUS > Dhcp6SeekInnerOptionSafe ( > IN UINT16 IaType, > IN UINT8 *Option, > IN UINT32 OptionLen, > OUT UINT8 **IaInnerOpt, > OUT UINT16 *IaInnerLen > ); > Lots of code cleanup to improve code readability. Cc: Saloni Kasbekar <saloni.kasbekar@intel.com> Cc: Zachary Clark-williams <zachary.clark-williams@intel.com> Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com> Reviewed-by: Saloni Kasbekar <saloni.kasbekar@intel.com>
niruiyu
pushed a commit
that referenced
this pull request
Nov 14, 2024
This patch does not impact functionality. It aims to clarify the synchronization flow between the BSP and APs to enhance code readability and understanding: Steps tianocore#6 and tianocore#11 are the basic synchronization requirements for all cases. Steps #1 is additional requirements if the MmCpuSyncModeTradition mode is selected. Steps #1, #2, #3, tianocore#4, tianocore#5, tianocore#7, tianocore#8, tianocore#9, and tianocore#10 are additional requirements if the system needs to configure the MTRR. Steps tianocore#9 and tianocore#10 are additional requirements if the system needs to support the mSmmDebugAgentSupport. Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
niruiyu
pushed a commit
that referenced
this pull request
Nov 14, 2024
… func This patch is for PiSmmCpuDxeSmm driver to add one round wait/release sync for BSP and AP to perform the SMM CPU Platform Hook before executing MMI Handler: SmmCpuPlatformHookBeforeMmiHandler (). With the function, SMM CPU driver can perform the platform specific items after one round BSP and AP sync (to make sure all APs in SMI) and before the MMI handlers. After the change, steps #1 and #2 are additional requirements if the MmCpuSyncModeTradition mode is selected. Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://bugzilla.tianocore.org/show_bug.cgi?id=3635
Currently, in order to test the supported attributes, the PciTestSupportedAttribute() will set the command register to 0x27 (EFI_PCI_COMMAND_IO_SPACE, EFI_PCI_COMMAND_MEMORY_SPACE, EFI_PCI_COMMAND_BUS_MASTER, EFI_PCI_COMMAND_VGA_PALETTE_SNOOP) firstly, and then read back to check whether these attributes are set successfully in the device.
This will cause the other enabled bits
(other than EFI_PCI_COMMAND_IO_SPACE,EFI_PCI_COMMAND_MEMORY_SPACE,
EFI_PCI_COMMAND_BUS_MASTER,EFI_PCI_COMMAND_VGA_PALETTE_SNOOP)
be cleared for a short of time
This patch fixes this issue by keeping the origina
enabled bits when setting 0x27.
Reviewed-by: Ray ray.ni@intel.com