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.

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

[Bug]: Math in OnScreenKeyboardDxe can result in dimensions that overrun max resolution. #426

Open
1 task done
rogurr opened this issue Jan 30, 2024 · 1 comment
Open
1 task done
Assignees
Labels
state:backlog In the backlog state:needs-maintainer-feedback Needs more information from a maintainer to determine next steps state:needs-triage Needs to triaged to determine next steps state:stale Has not been updated in a long time urgency:medium Important with a moderate impact

Comments

@rogurr
Copy link
Contributor

rogurr commented Jan 30, 2024

Is there an existing issue for this?

  • I have searched existing issues

Current Behavior

The transforms used in the OnScreenKeyboardDxe driver in the DisplayTransform.c file all use floats to perform calculations for rotation, positioning, scaling, etc. When doing the math for putting the keyboard in the lower right corner of the screen on certain resolutions results in a 1 pixel overrun of either the X or Y max resolution. This incorrect X or Y point eventually makes it's way to the RenderingEngineDxe driver that makes a mParentGop->Blt() call into the GOP with the 1 pixel overrun. Depending on the silicon vendor's GOP, that can have differing results from being ignored to on screen corruption.

Expected Behavior

Three areas need to be addressed:

  1. The OnScreenKeyboardDriver.c driver file uses that math for several functions to scale, rotate, and position the keyboard. They all have access to a member GOP pointer, so logic should probably be added to those functions to guarantee any point in the keyboard does not go over the expected resolution.
  2. The RenderingEngine.c file in the RenderingEngineDxe driver publishes a protocol that the keyboard driver uses. Since it is a public API, the driver should probably validate the inputs are acceptable and either return an error code, or debug print and clip the overrun.
  3. The RenderingEngine.c file in the RenderingEngineDxe driver makes multiple calls to mParentGop->Blt() which is using the GOP protocol without checking the return status code. Again, we probably need to check the return code and pass up to the caller of the rendering protocol user.

Steps To Reproduce

Due to this being an error handler issue and needing a vendor specific GOP with specific keyboard size and specific screen resolution to get the proper rounding error in the float math, it is very difficult to reproduce. Easiest way to see the error is to modify the SREActivateSurface function call in the RenderingEngine.c file to increase the FrameWidth parameter in the mParentGop->Blt calls to be 1 pixel past the X resolution.

Build Environment

- OS(s): Windows
- Tool Chain(s): VS2022
- Targets Impacted: All

Version Information

url = https://github.com/microsoft/mu_plus
branch = release/202302
SHA = 69d1c094ca61f6d12e8c8e172b07576120e6d259

Urgency

Medium

Are you going to fix this?

Someone else needs to fix it

Do you need maintainer feedback?

Maintainer feedback requested

Anything else?

Changes have been made to the current platform in development to implement a wrapper around each of the BLT calls to clip the offending pixel, but I would assume the MU team would prefer a better fix than a patch. Please contact me for the workaround if interested.

@rogurr rogurr added state:needs-triage Needs to triaged to determine next steps type:bug Something isn't working labels Jan 30, 2024
@github-actions github-actions bot added state:needs-maintainer-feedback Needs more information from a maintainer to determine next steps state:needs-owner Needs an issue owner to be assigned urgency:medium Important with a moderate impact labels Jan 30, 2024
@TaylorBeebe TaylorBeebe self-assigned this Jan 31, 2024
@github-actions github-actions bot removed the state:needs-owner Needs an issue owner to be assigned label Jan 31, 2024
Copy link

This issue has been automatically marked as stale because it has not had activity in 45 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

@github-actions github-actions bot added the state:stale Has not been updated in a long time label Mar 16, 2024
@TaylorBeebe TaylorBeebe added state:backlog In the backlog and removed type:bug Something isn't working labels Mar 17, 2024
@TaylorBeebe TaylorBeebe assigned spbrogan and unassigned TaylorBeebe May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:backlog In the backlog state:needs-maintainer-feedback Needs more information from a maintainer to determine next steps state:needs-triage Needs to triaged to determine next steps state:stale Has not been updated in a long time urgency:medium Important with a moderate impact
Projects
None yet
Development

No branches or pull requests

3 participants