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]: [GenFw] FindPrmHandler in Elf64Convert.c assumes symbol value is file offset #550

Closed
1 task done
kurtjd opened this issue Sep 1, 2023 · 2 comments
Closed
1 task done
Labels
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 type:bug Something isn't working urgency:low Little to no impact

Comments

@kurtjd
Copy link

kurtjd commented Sep 1, 2023

Is there an existing issue for this?

  • I have searched existing issues

Current Behavior

When GenFw is ran with the --prm option, it will attempt to locate the PrmModuleExportDescriptor symbol as part of the ScanSections64 function call. Once found, it will pass its 'st_value' field value into FindPrmHandler as an offset to find the location of PRM handler symbols in the file, as seen here:

FindPrmHandler(Sym->st_value);

However, st_value does not necessarily represent the offset of a symbol in the file. It appears to actually represent the VMA of the symbol. For example, on my build, PrmModuleExportDescriptor is placed in the .data section, which has a VMA of 0x6000, but the .data section is actually at file offset 0x7000. So, when st_value is passed into FindPrmHandler, the function begins looking at offset 0x6000 for PRM handlers rather than 0x7000.

A potential fix (which works on my build) is to calculate the symbol offset by first finding the section header it belongs to using the symbol's st_shndx field. Then, subtract the section's address from the symbol's address before finally adding the section offset, like this:

STATIC
Elf64_Addr
GetSymValueOffset (
  Elf_Sym* Sym
  )
{
    Elf_Shdr* shdr = GetShdrByIndex(Sym->st_shndx);
    return (Sym->st_value - shdr->sh_addr) + shdr->sh_offset;
}

Thus, anytime a symbol's value is treated as an offset, this function should be used instead.

Expected Behavior

I expect GenFw, when ran with the --prm option, to locate PRM-related symbols in an ELF file regardless of what their VMA value is.

Steps To Reproduce

  1. Write a sample PRM module, and pass the --prm option to GenFw in the sample module's INF file
  2. Build UEFI FW using the CLANG100WIN/ARM toolchain
  3. View the build log, and observe that GenFw will likely error out as it attempts to read garbage (in my case, it prints error message "FindPrmHandler: Number 255 is too high." as it thought there were over 25,000 PRM Handlers)

Build Environment

- OS(s): Windows 11
- Tool Chain(s): CLANG100WIN, arm-linker
- Targets Impacted: AARCH64 DEBUG (only target tested)

Version Information

Latest

Urgency

Low

Are you going to fix this?

I will fix it

Do you need maintainer feedback?

Maintainer feedback requested

Anything else?

This shows .data has a VMA that is not equal to its file offset:
prm_shdrs

This shows PrmModuleExportDescriptor has a symbol value of 0x6000 (since it's the first symbol in .data):
prm_sym

And this shows PrmModuleExportDescriptor actually has a file offset of 0x7000 (thus the symbol's value is not its offset):
prm_hex

@kurtjd kurtjd added state:needs-triage Needs to triaged to determine next steps type:bug Something isn't working labels Sep 1, 2023
@github-actions github-actions bot added state:needs-maintainer-feedback Needs more information from a maintainer to determine next steps urgency:low Little to no impact labels Sep 1, 2023
@github-actions
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 Oct 16, 2023
@github-actions
Copy link

This issue has been automatically been closed because it did not have any activity in 45 days and no follow up within 7 days after being marked stale. Thank you for your contributions.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 type:bug Something isn't working urgency:low Little to no impact
Projects
None yet
Development

No branches or pull requests

1 participant