[edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy -- push #324
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
In the DxeImageVerificationHandler() function, the "VerifyStatus" variable can only contain one of two values: EFI_SUCCESS and EFI_ACCESS_DENIED. Furthermore, the variable is only consumed with EFI_ERROR(). Therefore, using the EFI_STATUS type for the variable is unnecessary. Worse, given the complex meanings of the function's return values, using EFI_STATUS for "VerifyStatus" is actively confusing. Rename the variable to "IsVerified", and make it a simple BOOLEAN. This patch is a no-op, regarding behavior. Cc: Chao Zhang <chao.b.zhang@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200116190705.18816-2-lersek@redhat.com> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> [lersek@redhat.com: push with Mike's R-b due to Chinese New Year Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid <d3fbb76dabed4e1987c512c328c82810@intel.com>]
…reak
In the code structure
if (condition) {
//
// block1
//
return;
} else {
//
// block2
//
}
nesting "block2" in an "else" branch is superfluous, and harms
readability. It can be transformed to:
if (condition) {
//
// block1
//
return;
}
//
// block2
//
with identical behavior, and improved readability (less nesting).
The same applies to "break" (instead of "return") in a loop body.
Perform these transformations on DxeImageVerificationHandler().
This patch is a no-op for behavior. Use
git show -b -W
for reviewing it more easily.
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Message-Id: <20200116190705.18816-3-lersek@redhat.com>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
[lersek@redhat.com: push with Mike's R-b due to Chinese New Year
Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
<d3fbb76dabed4e1987c512c328c82810@intel.com>]
…ernal The PeCoffLoaderGetImageInfo() function may return various error codes, such as RETURN_INVALID_PARAMETER and RETURN_UNSUPPORTED. Such error values should not be assigned to our "Status" variable in the DxeImageVerificationHandler() function, because "Status" generally stands for the main exit value of the function. And SECURITY2_FILE_AUTHENTICATION_HANDLER functions are expected to return one of EFI_SUCCESS, EFI_SECURITY_VIOLATION, and EFI_ACCESS_DENIED only. Introduce the "PeCoffStatus" helper variable for keeping the return value of PeCoffLoaderGetImageInfo() internal to the function. If PeCoffLoaderGetImageInfo() fails, we'll jump to the "Done" label with "Status" being EFI_ACCESS_DENIED, inherited from the top of the function. Note that this is consistent with the subsequent PE/COFF Signature check, where we jump to the "Done" label with "Status" having been re-set to EFI_ACCESS_DENIED. As a consequence, we can at once remove the Status = EFI_ACCESS_DENIED; assignment right after the "PeCoffStatus" check. This patch does not change the control flow in the function, it only changes the "Status" outcome from API-incompatible error codes to EFI_ACCESS_DENIED, under some circumstances. Cc: Chao Zhang <chao.b.zhang@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200116190705.18816-4-lersek@redhat.com> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> [lersek@redhat.com: push with Mike's R-b due to Chinese New Year Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid <d3fbb76dabed4e1987c512c328c82810@intel.com>]
Inside the "for" loop that scans the signatures of the image, we call HashPeImageByType(), and assign its return value to "Status". Beyond the immediate retval check, this assignment is useless (never consumed). That's because a subsequent access to "Status" may only be one of the following: - the "Status" assignment when we call HashPeImageByType() in the next iteration of the loop, - the "Status = EFI_ACCESS_DENIED" assignment right after the final "IsVerified" check. To make it clear that the assignment is only useful for the immediate HashPeImageByType() retval check, introduce a specific helper variable, called "HashStatus". This patch is a no-op, functionally. Cc: Chao Zhang <chao.b.zhang@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200116190705.18816-5-lersek@redhat.com> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> [lersek@redhat.com: push with Mike's R-b due to Chinese New Year Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid <d3fbb76dabed4e1987c512c328c82810@intel.com>]
A SECURITY2_FILE_AUTHENTICATION_HANDLER function is not expected to return EFI_OUT_OF_RESOURCES. We should only return EFI_SUCCESS, EFI_SECURITY_VIOLATION, or EFI_ACCESS_DENIED. In case we run out of memory while preparing "SignatureList" for AddImageExeInfo(), we should simply stick with the EFI_ACCESS_DENIED value that is already in "Status" -- from just before the "Action" condition --, and not suppress it with EFI_OUT_OF_RESOURCES. This patch does not change the control flow in the function, it only changes the "Status" outcome from API-incompatible error codes to EFI_ACCESS_DENIED, under some circumstances. Cc: Chao Zhang <chao.b.zhang@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129 Fixes: 570b3d1 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200116190705.18816-6-lersek@redhat.com> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> [lersek@redhat.com: push with Mike's R-b due to Chinese New Year Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid <d3fbb76dabed4e1987c512c328c82810@intel.com>]
…tting After the final "IsVerified" check, we set "Status" to EFI_ACCESS_DENIED. This is superfluous, as "Status" already carries EFI_ACCESS_DENIED value there, from the top of the function. Remove the assignment. Functionally, this change is a no-op. Cc: Chao Zhang <chao.b.zhang@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200116190705.18816-7-lersek@redhat.com> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> [lersek@redhat.com: push with Mike's R-b due to Chinese New Year Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid <d3fbb76dabed4e1987c512c328c82810@intel.com>]
Before the "Done" label at the end of DxeImageVerificationHandler(), we now have a single access to "Status": we set "Status" to EFI_ACCESS_DENIED at the top of the function. Therefore, the (Status != EFI_SUCCESS) condition is always true under the "Done" label. Accordingly, unnest the AddImageExeInfo() call dependent on that condition, remove the condition, and also rename the "Done" label to "Failed". Functionally, this patch is a no-op. It's easier to review with: git show -b -W Cc: Chao Zhang <chao.b.zhang@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200116190705.18816-8-lersek@redhat.com> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> [lersek@redhat.com: replace EFI_D_INFO w/ DEBUG_INFO for PatchCheck.py] [lersek@redhat.com: push with Mike's R-b due to Chinese New Year Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid <d3fbb76dabed4e1987c512c328c82810@intel.com>]
The "Status" variable is set to EFI_ACCESS_DENIED at the top of the function. Then it is overwritten with EFI_SECURITY_VIOLATION under the "Failed" (earlier: "Done") label. We finally return "Status". The above covers the complete usage of "Status" in DxeImageVerificationHandler(). Remove the variable, and simply return EFI_SECURITY_VIOLATION in the end. This patch is a no-op, regarding behavior. Cc: Chao Zhang <chao.b.zhang@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200116190705.18816-9-lersek@redhat.com> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> [lersek@redhat.com: push with Mike's R-b due to Chinese New Year Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid <d3fbb76dabed4e1987c512c328c82810@intel.com>]
…NULL) "FileBuffer" is a non-optional input (pointer) parameter to DxeImageVerificationHandler(). Normally, when an edk2 function receives a NULL argument for such a parameter, we return EFI_INVALID_PARAMETER or RETURN_INVALID_PARAMETER. However, those don't conform to the SECURITY2_FILE_AUTHENTICATION_HANDLER prototype. Return EFI_ACCESS_DENIED when "FileBuffer" is NULL; it means that no image has been loaded. This patch does not change the control flow in the function, it only changes the "Status" outcome from API-incompatible error codes to EFI_ACCESS_DENIED, under some circumstances. Cc: Chao Zhang <chao.b.zhang@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129 Fixes: 570b3d1 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200116190705.18816-10-lersek@redhat.com> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> [lersek@redhat.com: push with Mike's R-b due to Chinese New Year Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid <d3fbb76dabed4e1987c512c328c82810@intel.com>]
… fail It makes no sense to call AddImageExeInfo() with (Signature == NULL) and (SignatureSize > 0). AddImageExeInfo() does not crash in such a case -- it avoids the CopyMem() call --, but it creates an invalid EFI_IMAGE_EXECUTION_INFO record. Namely, the "EFI_IMAGE_EXECUTION_INFO.InfoSize" field includes "SignatureSize", but the actual signature bytes are not filled in. Document and ASSERT() this condition in AddImageExeInfo(). In DxeImageVerificationHandler(), zero out "SignatureListSize" if we set "SignatureList" to NULL due to AllocateZeroPool() failure. (Another approach could be to avoid calling AddImageExeInfo() completely, in case AllocateZeroPool() fails. Unfortunately, the UEFI v2.8 spec does not seem to state clearly whether a signature is mandatory in EFI_IMAGE_EXECUTION_INFO, if the "Action" field is EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED or EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND. For now, the EFI_IMAGE_EXECUTION_INFO addition logic is not changed; we only make sure that the record we add is not malformed.) Cc: Chao Zhang <chao.b.zhang@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200116190705.18816-11-lersek@redhat.com> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> [lersek@redhat.com: push with Mike's R-b due to Chinese New Year Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid <d3fbb76dabed4e1987c512c328c82810@intel.com>]
In DxeImageVerificationHandler(), we should return EFI_SECURITY_VIOLATION for a rejected image only if the platform sets DEFER_EXECUTE_ON_SECURITY_VIOLATION as the policy for the image's source. Otherwise, EFI_ACCESS_DENIED must be returned. Right now, EFI_SECURITY_VIOLATION is returned for all rejected images, which is wrong -- it causes LoadImage() to hold on to rejected images (in untrusted state), for further platform actions. However, if a platform already set DENY_EXECUTE_ON_SECURITY_VIOLATION, the platform will not expect the rejected image to stick around in memory (regardless of its untrusted state). Therefore, adhere to the platform policy in the return value of the DxeImageVerificationHandler() function. Furthermore, according to "32.4.2 Image Execution Information Table" in the UEFI v2.8 spec, and considering that edk2 only supports (AuditMode==0) at the moment: > When AuditMode==0, if the image's signature is not found in the > authorized database, or is found in the forbidden database, the image > will not be started and instead, information about it will be placed in > this table. we have to store an EFI_IMAGE_EXECUTION_INFO record in both the "defer" case and the "deny" case. Thus, the AddImageExeInfo() call is not being made conditional on (Policy == DEFER_EXECUTE_ON_SECURITY_VIOLATION); the documentation is updated instead. Cc: Chao Zhang <chao.b.zhang@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129 Fixes: 5db28a6 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200116190705.18816-12-lersek@redhat.com> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> [lersek@redhat.com: push with Mike's R-b due to Chinese New Year Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid <d3fbb76dabed4e1987c512c328c82810@intel.com>]
|
Hmm, mergify seems to be stuck; the checks have passed but the PR is not being merged. (Despite my setting the "push" label very soon after opening the PR.) So I guess I'll close this and reopen it, based on: https://edk2.groups.io/g/devel/message/53372 |
|
reopening... |
VivianNK
pushed a commit
to VivianNK/edk2
that referenced
this pull request
Apr 5, 2023
## Description Fixes bug tianocore#324. There are two NVMe CQs that get updated by hardware. In these two files, we do not have a volatile keyword on these CQ structs, so when we run them in a loop to check if they have changed, we do not continue to fetch the HW updated queues. Adding the volatile keyword allows us to continue to check the queues during the timer period (otherwise the timer period elapses without being able to operate on queues with descriptors in them. I checked for other instances of this pattern in the NVMe code that are not reported in this bug, I only found the fixed instances in NvmExpressPassthru.c and NvmExpressPeiPassThru.c. There is a similar if statement in NvmExpress.c, but the CQ gets updated in that loop and this is just a timer callback, process what we found and start the timer again. This PR also updates SQs to have the volatile keyword, per PR feedback, to ensure that HW accesses are done for each read of the SQ. Current code patterns do not appear to have an issue with reading SQs, but adding the volatile keyword will help ensure that patterns are not introduced that have the same issue as the CQs, as well as maintaining the same pattern with respect to HW queues. - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... ## How This Was Tested Tested by bug submitter on an ARM64 platform. ## Integration Instructions N/A.
kuqin12
pushed a commit
to kuqin12/edk2
that referenced
this pull request
Oct 3, 2023
## Description Fixes bug tianocore#324. There are two NVMe CQs that get updated by hardware. In these two files, we do not have a volatile keyword on these CQ structs, so when we run them in a loop to check if they have changed, we do not continue to fetch the HW updated queues. Adding the volatile keyword allows us to continue to check the queues during the timer period (otherwise the timer period elapses without being able to operate on queues with descriptors in them. I checked for other instances of this pattern in the NVMe code that are not reported in this bug, I only found the fixed instances in NvmExpressPassthru.c and NvmExpressPeiPassThru.c. There is a similar if statement in NvmExpress.c, but the CQ gets updated in that loop and this is just a timer callback, process what we found and start the timer again. This PR also updates SQs to have the volatile keyword, per PR feedback, to ensure that HW accesses are done for each read of the SQ. Current code patterns do not appear to have an issue with reading SQs, but adding the volatile keyword will help ensure that patterns are not introduced that have the same issue as the CQs, as well as maintaining the same pattern with respect to HW queues. - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... ## How This Was Tested Tested by bug submitter on an ARM64 platform. ## Integration Instructions N/A.
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=2129
https://edk2.groups.io/g/devel/message/53314
http://mid.mail-archive.com/20200116190705.18816-1-lersek@redhat.com