Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AdvLoggerPkg: Switch to using AsciiStrnLenS #293

Merged
merged 2 commits into from
Aug 23, 2023
Merged

AdvLoggerPkg: Switch to using AsciiStrnLenS #293

merged 2 commits into from
Aug 23, 2023

Conversation

apop5
Copy link
Contributor

@apop5 apop5 commented Aug 23, 2023 •
edited by makubacki
Loading

Description

A compounds assert was found in a platform. An assert was triggered, and when attempting to generate the assert messages, print lib triggered an assert as well. The system was found to have reached the PcdMaximumAsciiStringLength referenced in AsciiStrLen.

To combat this, the AsciiStrLen calls in AdvancedLogger are being switched to use the safe version, with the associated max lengths being the stack buffers that are used in the functions.

This change allowed assert messages to be displayed.

Fixes #292

  • 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 on a platform that was triggering asserts.
Tested in CI

Integration Instructions

N/A - Changes should be

@apop5 apop5 assigned cfernald, os-d and apop5 and unassigned cfernald and os-d Aug 23, 2023
@apop5 apop5 requested review from cfernald, makubacki and os-d August 23, 2023 19:24
@apop5 apop5 added the complexity:easy Requires minimal background information and effort to accomplish label Aug 23, 2023
os-d
os-d approved these changes Aug 23, 2023
View reviewed changes
Copy link
Contributor

@os-d os-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Michael's comment, LGTM.

@makubacki makubacki changed the title Switch to using AsciiStrnLenS. AdvLoggerPkg: Switch to using AsciiStrnLenS Aug 23, 2023
@apop5 apop5 force-pushed the switch_to_safestring branch from 4eae31d to 5ddd99d Compare August 23, 2023 19:50
@apop5 apop5 merged commit 2304375 into microsoft:release/202302 Aug 23, 2023
@ProjectMuBot ProjectMuBot mentioned this pull request Aug 24, 2023
Merged
ProjectMuBot referenced this pull request in microsoft/mu_tiano_platforms Aug 24, 2023
@ProjectMuBot
Bumps Common/MU from 2023020001.1.1 to 2023020001.1.2
a9cdf84 
Introduces 3 new commits in [Common/MU](https://github.com/microsoft/mu_plus.git).

<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/microsoft/mu_plus/commit/26580b3cac7a2e7e33d299d836a52034d5d52c67">26580b</a> pip: bump edk2-pytool-library from 0.16.1 to 0.17.0 (<a href="https://github.com/microsoft/mu_plus/pull/288">#288</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/c0d44c4584d2d95bfd309a60c076a28bbe3c23ce">c0d44c</a> Update UXN/PXN Parsing and Fix Filters in Paging Audit (<a href="https://github.com/microsoft/mu_plus/pull/290">#290</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/23043756b9d76f0c6fe7bdd8231727c3690d1e16">230437</a> AdvLoggerPkg: Switch to using AsciiStrnLenS (<a href="https://github.com/microsoft/mu_plus/pull/293">#293</a>)</li>
</ul>
</details>

Signed-off-by: Project Mu Bot <[email protected]>
makubacki referenced this pull request in microsoft/mu_tiano_platforms Aug 24, 2023
@ProjectMuBot @makubacki
Bumps Common/MU from 2023020001.1.1 to 2023020001.1.2
620046e 
Introduces 3 new commits in [Common/MU](https://github.com/microsoft/mu_plus.git).

<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/microsoft/mu_plus/commit/26580b3cac7a2e7e33d299d836a52034d5d52c67">26580b</a> pip: bump edk2-pytool-library from 0.16.1 to 0.17.0 (<a href="https://github.com/microsoft/mu_plus/pull/288">#288</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/c0d44c4584d2d95bfd309a60c076a28bbe3c23ce">c0d44c</a> Update UXN/PXN Parsing and Fix Filters in Paging Audit (<a href="https://github.com/microsoft/mu_plus/pull/290">#290</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/23043756b9d76f0c6fe7bdd8231727c3690d1e16">230437</a> AdvLoggerPkg: Switch to using AsciiStrnLenS (<a href="https://github.com/microsoft/mu_plus/pull/293">#293</a>)</li>
</ul>
</details>

Signed-off-by: Project Mu Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfernald cfernald cfernald approved these changes

@makubacki makubacki makubacki approved these changes

@os-d os-d os-d approved these changes

Assignees

@apop5 apop5

Labels
complexity:easy Requires minimal background information and effort to accomplish
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature]: Switch AdvancedLogger to use Safe Strings.
4 participants
@apop5 @cfernald @makubacki @os-d