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

Add ARM64 exceptions #1902

Closed
wants to merge 4 commits into from
Closed

Conversation

hmartinez82
Copy link
Contributor

@hmartinez82 hmartinez82 commented Nov 16, 2023

Initial discussion for something better than a blank page :)
Is this acceptable?

image

@hmartinez82
Copy link
Contributor Author

Also, a very newbie question: How do I read the .xdata section? I've been scouring the internet, but I can't for the life of me find how to read it!

@jxy-s
Copy link
Member

jxy-s commented Nov 16, 2023

Does ARM64EC have a table we need to locate and extract alternate exception info from?

Also, a very newbie question: How do I read the .xdata section? I've been scouring the internet, but I can't for the life of me find how to read it!

UnwindData I believe is an RVA, you can use PhMappedImageRvaToSection to locate the section. The RVA to a virtual address with PhMappedImageRvaToVa.

@hmartinez82
Copy link
Contributor Author

hmartinez82 commented Nov 16, 2023

@jxy-s I don't fully understand how to use PhMappedImageRvaToSection() in this case. If I understand correctly I would need the RVA of where .xdata starts and use PhMappedImageRvaToSection(), right? But I don't know that value wither. The UnwindData field in each exception entry is the RVA inside .xdata. At least that's what I understood from:

UnwindData is the Exception Information RVA: The address of the variable-length exception information structure, stored in the .xdata section. This data must be 4-byte aligned.

I don't know how to find where .xdata starts

@hmartinez82 hmartinez82 reopened this Nov 16, 2023
@jxy-s
Copy link
Member

jxy-s commented Nov 16, 2023

UnwindData is the Exception Information RVA

PhMappedImageRvaToSection takes an RVA and returns the IMAGE_SECTION_HEADER for which that RVA resides. If you want the information about that section you can use PhMappedImageRvaToSection to get it. You have an RVA, it's UnwindData. That should get you to the .xdata IMAGE_SECTION_HEADER. But, if you want to just read what information is at UnwindData you can use PhMappedImageRvaToVa which will look up the section in which it resides and do the calculation to give you a pointer to the data in the section mapped via the PH_MAPPED_IMAGE structure.

So, since I think your goal is to read the exception information stored at the UnwindData RVA, you should just PhMappedImageRvaToVa to get the VA into the section mapping.

@hmartinez82
Copy link
Contributor Author

hmartinez82 commented Nov 16, 2023

You are right. Turns out I don't need to know where .xdata starts. PhMappedImageRvaToVa() alone did the trick:
image

I'm using the output from dumpbin /unwindinfo to check my calculations.

@hmartinez82 hmartinez82 changed the title [draft] Adding ARM64 exceptions Add ARM64 exceptions Nov 16, 2023
@hmartinez82 hmartinez82 marked this pull request as ready for review November 16, 2023 07:59
@hmartinez82
Copy link
Contributor Author

hmartinez82 commented Nov 16, 2023

Regarding ARM64EC. I have no idea. I can work on that in a next PR 😅

@jxy-s
Copy link
Member

jxy-s commented Nov 16, 2023

Some of these ARM64 items were left unimplemented simply because I had lost the energy to keep working on it - ARM64EC is a beast... we have support for the it in most places. In general, I was looking for time to find the right way to approach some of outstanding ARM64 items. I appreciate your time and effort helping to move this along ❤️

@jxy-s jxy-s self-requested a review November 20, 2023 23:41
@jxy-s jxy-s self-assigned this Nov 20, 2023
@DavidXanatos
Copy link
Contributor

for ARM64EC you may want to review this project: https://github.com/FFRI/ProjectChameleon

Copy link
Member

@jxy-s jxy-s left a comment

Choose a reason for hiding this comment

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

Apologies on the delay here. I just got around to testing the changes myself. I've added some feedback to incorporate.

Additionally there is AMD64 in the ARM64EC binary (you will see then when you use dumpbin.exe). I haven't located where they are. If I had to guess the dynamic relocations fix up the IMAGE_DIRECTORY_ENTRY_EXCEPTION directory in the headers to point to the alternative directory. The loader will do this for ARM64EC in the case of the export directory, see PhpFixupExportDirectoryForARM64EC for the hoops necessary to locate the alternate directory.

{
PhPrintPointer(value, UlongToPtr(entry->UnwindData));
PhSetListViewSubItem(ListViewHandle, lvItemIndex, 2, value);
PhSetListViewSubItem(ListViewHandle, lvItemIndex, 3, L"✔️");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
PhSetListViewSubItem(ListViewHandle, lvItemIndex, 3, L"✔️");
PhSetListViewSubItem(ListViewHandle, lvItemIndex, 3, L"Yes");

I appreciate this visually but we have a precedence for using "Yes" instead of extended Unicode characters.

image

We've had issues the past with editors and GitHub itself screwing this up. At very least, for the sake of consistency - would you please change this to L"Yes" instead?

Comment on lines +194 to +195
exceptionData = (IMAGE_ARM64_RUNTIME_FUNCTION_ENTRY_XDATA*)PhMappedImageRvaToVa(&PvMappedImage, entry->UnwindData, NULL);
functionLength = exceptionData->FunctionLength << 2;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
exceptionData = (IMAGE_ARM64_RUNTIME_FUNCTION_ENTRY_XDATA*)PhMappedImageRvaToVa(&PvMappedImage, entry->UnwindData, NULL);
functionLength = exceptionData->FunctionLength << 2;
exceptionData = (IMAGE_ARM64_RUNTIME_FUNCTION_ENTRY_XDATA*)PhMappedImageRvaToVa(&PvMappedImage, entry->UnwindData, NULL);
if (exceptionData)
functionLength = exceptionData->FunctionLength << 2;
else
functionLength = 0;

Please add a null pointer check. While unlikely, in an odd/corrupt case the RVA could not land within a section, causing the parser here to crash.

}
}

printf("Flag: %u\n", entry->Flag);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
printf("Flag: %u\n", entry->Flag);

Please remove this.

PhPrintPointer(value, UlongToPtr(entry->BeginAddress));
PhSetListViewSubItem(ListViewHandle, lvItemIndex, 1, value);

ULONG functionLength = entry->FunctionLength << 2;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should move accessing the packed FunctionLength into the case where we know the entry is in the packed form. Especially since I'm asking for a null pointer check elsewhere.

ULONG functionLength = entry->FunctionLength << 2;
if (entry->Flag != 0) //Packed Unwind Data
{
PhPrintPointer(value, UlongToPtr(entry->UnwindData));
Copy link
Member

Choose a reason for hiding this comment

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

Accessing UnwindData here is incorrect. This is an erroneous value when Flags != 0.

Copy link
Member

@jxy-s jxy-s Dec 2, 2023

Choose a reason for hiding this comment

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

It's probably best to instead show a representation of these bits here:

            ULONG RegF : 3;
            ULONG RegI : 4;
            ULONG H : 1;
            ULONG CR : 2;
            ULONG FrameSize : 9

Described here: https://learn.microsoft.com/en-us/cpp/build/arm64-exception-handling?view=msvc-170#packed-unwind-data

PhSetListViewSubItem(ListViewHandle, lvItemIndex, 1, value);

ULONG functionLength = entry->FunctionLength << 2;
if (entry->Flag != 0) //Packed Unwind Data
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth showing the type of packing here (instead of yes/no), the bits are described here:
https://learn.microsoft.com/en-us/cpp/build/arm64-exception-handling?view=msvc-170#packed-unwind-data

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, instead of showing L"Yes" for "Packed", we could instead show something more descriptive:

    00 = packed unwind data not used; remaining bits point to an .xdata record
    01 = packed unwind data used with a single prolog and epilog at the beginning and end of the scope
    10 = packed unwind data used for code without any prolog and epilog. Useful for describing separated function segments
    11 = reserved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any idea on column names and what text for each case?

@jxy-s
Copy link
Member

jxy-s commented Dec 2, 2023

Yep the loader does the same thing with the exception directory that it does with the export directory, see this:
image

We're looking at the headers of the ARM64EC ntdll.dll. On the left is a native ARM64 binary. On the right is an AMD64 binary running in emulation mode.

ARM64:

  32A8A0 [   15BC9] address [size] of Export Directory
       0 [       0] address [size] of Import Directory
  38C000 [   75070] address [size] of Resource Directory
  352000 [   12D18] address [size] of Exception Directory
  3EE600 [    B4F0] address [size] of Security Directory
  402000 [     E70] address [size] of Base Relocation Directory
  2CAAD0 [      54] address [size] of Debug Directory
       0 [       0] address [size] of Description Directory
       0 [       0] address [size] of Special Directory
       0 [       0] address [size] of Thread Storage Directory
  2CA990 [     140] address [size] of Load Configuration Directory
       0 [       0] address [size] of Bound Import Directory
       0 [       0] address [size] of Import Address Table Directory
       0 [       0] address [size] of Delay Import Directory
       0 [       0] address [size] of COR20 Header Directory
       0 [       0] address [size] of Reserved Directory

x64:

  314C30 [   15C70] address [size] of Export Directory
       0 [       0] address [size] of Import Directory
  38C000 [   75070] address [size] of Resource Directory
  364D18 [    177C] address [size] of Exception Directory
  3EE600 [    B4F0] address [size] of Security Directory
  402000 [     E70] address [size] of Base Relocation Directory
  2CAAD0 [      54] address [size] of Debug Directory
       0 [       0] address [size] of Description Directory
       0 [       0] address [size] of Special Directory
       0 [       0] address [size] of Thread Storage Directory
  2CA7D0 [     140] address [size] of Load Configuration Directory
       0 [       0] address [size] of Bound Import Directory
       0 [       0] address [size] of Import Address Table Directory
       0 [       0] address [size] of Delay Import Directory
       0 [       0] address [size] of COR20 Header Directory
       0 [       0] address [size] of Reserved Directory

So, to support showing the AMD64 exception information in an ARM64EC binary (as we do for the exports already). You will want to do something similar to PhpFixupExportDirectoryForARM64EC for fixing up the exception directory.

/* Todo */
for (ULONG i = 0; i < exceptions.NumberOfEntries; i++)
{
PPH_IMAGE_RUNTIME_FUNCTION_ENTRY_ARM64 entry = PTR_ADD_OFFSET(exceptions.ExceptionDirectory, i * sizeof(PH_IMAGE_RUNTIME_FUNCTION_ENTRY_ARM64));
Copy link
Member

@jxy-s jxy-s Dec 3, 2023

Choose a reason for hiding this comment

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

IMAGE_ARM64_RUNTIME_FUNCTION_ENTRY is in the SDK headers now, I think we can eliminate the PPH_IMAGE_RUNTIME_FUNCTION_ENTRY_ARM64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok. I thought that there was a reason all these entry types were being redeclared. I did see it in the SDK headers, but just to keep it conformance with the others ones I ended up redeclaring. I can work on this once I'm back.

@hmartinez82
Copy link
Contributor Author

Tank you for the many change requests. I'll work on the ones that are up to my league once I'm back after December, 11th

@jxy-s
Copy link
Member

jxy-s commented Feb 8, 2024

@hmartinez82 I'm currently working on a set of changes specific to ARM to provide better support and visibility. I wanted to ask if you would like me to incorporate your changes here to that set of changes, or if you plan to continue this PR?

@hmartinez82
Copy link
Contributor Author

hmartinez82 commented Feb 8, 2024

Sure, you can incorporate these changes in your branch and I'll close this one :)

@hmartinez82 hmartinez82 closed this Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants