Test change per your request #1
Conversation
Test test
|
it seems I couldn't attach a comment to the commit message. Let's see if I can attach a comment to a line of code. |
|
Not sure why my original comment ended up doubly in this view. |
| @@ -1,6 +1,6 @@ | |||
| # EDK II Project | |||
|
|
|||
| A modern, feature-rich, cross-platform firmware development environment | |||
There was a problem hiding this comment.
Review comment attached to a single line of code.
Not sure how the buttons differ, "add single comment" vs. "start a review".
| @@ -1,6 +1,6 @@ | |||
| # EDK II Project | |||
|
|
|||
| A modern, feature-rich, cross-platform firmware development environment | |||
|
While reviewing my above comments, I'm having trouble getting a complete view of the patch, with subject line, commit message, code changes, and my comments, included. |
|
Ah, OK, I have to ignore the "lersek commented XXX minutes ago" entries, and just go directly to the "commits" view. There I'll find everything together. |
|
OK. While I find this interface somewhat cumbersome for now, it seems like I can attach comments to specific patch lines, and general comments to the commit message (although not to specific lines of the commit message). In addition, if I click the "commits" button at the top, I can view the commits in full detail, optionally displaying vs. hiding the comments on them as well. This looks good. The discussion / history-like view of my comments, in the general pullreq interface, seems rather unhelpful, as they show basically no context (the commit in question is not identified in any way, neither are the lines other than the ones having received a remark). That's no problem, I can ignore those entries. |
|
Alright; @out0xb2 can you please make a non-fast-forwardable change on the same |
|
My next test request will be that you please delete your subject repository from github. (If that's too much, then please create another repo, submit a pullreq from that, and then delete that repo.) I'd like to test whether the commits under review, or the comments, disappear when the original requestor's topic branch & repo disappear. Thanks! |
|
The email notifications that I've received about my patch comments are not good enough. Again, the main problem is that they completely lack context. The notification email does not name the commit itself (the subject line is not quoted, only the PR subject is put in the email's subject). This is an issue in a multi-patch series. Furthermore, while the file is named, too little context is cited. If I understand right, the leading context is quoted, but the trailing context isn't. |
|
Oh and the commit message ("test test") is also not quoted in the emails. |
|
Am I right to think that the HTML contents of the notification emails include "web bugs"? Thunderbird keeps warning me that it has blocked remote content in the emails. |
|
Rejecting this to see if that changes anything about the accessibility of commit 364268d. |
|
No immediate problems, using the link at the top (i.e., |
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
MpInitLib sets X2ApicEnable in two places. 1. CollectProcessorCount() This function is called when MpInitLibInitialize() hasn't been called before. It sets X2ApicEnable and later in the same function it configures all CPUs to operate in X2 APIC mode. 2. MpInitLibInitialize() The X2ApicEnable setting happens when this function is called in second time. But after that setting, no code consumes that flag. With the above analysis and with the purpose of simplifying the code, the X2ApicEnable in #1 is changed to local variable and the #2 can be changed to remove the setting of X2ApicEnable. Signed-off-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Eric Dong <eric.dong@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
The unit test app supports running in 3 mode:
1. MtrrLibUnitTest generate-random-numbers
<path to MtrrLib/UnitTest/RandomNumber.c> <random-number count>
It generates random numbers and writes to RandomNumber.c.
2. MtrrLibUnitTest [<iterations>]
It tests MtrrLib APIs using configurations generated from static
numbers generated by mode #1.
This is the default execution mode running in CI environment.
3. MtrrLibUnitTest <iterations> random
It tests MtrrLib APIs using configurations generated from random
numbers.
This is what developers can use to test MtrrLib for regressions.
Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ming Shao <ming.shao@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
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>
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>
Testing 1, 2, 3