Vector35 / binaryninja-api

Public API, examples, documentation and issues for Binary Ninja
https://binary.ninja/
MIT License
948 stars 213 forks source link

Binja doesn't properly set function parameters in the ILs #3910

Closed hjlbs closed 1 year ago

hjlbs commented 1 year ago

Version and Platform (required):

Bug Description: When doing the analysis for the *IL stuff, even though the function disassembly shows the correct number of arguments that is not propagated up to the caller.

Steps To Reproduce: Open the .efi file I provide in the tarball and look at _start. The first function called is sub_418 but there are no arguments indicated. However if you look at the function sub_418, the proper arguments are shown.

Expected Behavior: It would be nice if the proper arguments propagated up to the caller.

Screenshots: If applicable, please add screenshots here to help explain your problem.

Additional Information: Please add any other context about the problem here. Screenshot from 2023-02-17 10-54-55 Screenshot from 2023-02-17 10-55-14 PlatformSetup.tar.gz

psifertex commented 1 year ago

Start has no arguments pushed anywhere or initialized -- where are the arguments supposed to actually come from? I don't know much about how EFI works, is there an initialization structure being passed here that's missing? Does it use a different calling convention?

You can fix this temporarily by right-clicking on the call to 418 inside of start and selecting "override call type" and just hitting enter to force the instance of the call to be fixed with the expected number of parameters but the issue here is you have basically two conflicting sets of information and BN has to pick one. It's up to you the user to identify which one is correct if it gets it wrong. The question is whether the detected behavior in sub418 is correct (it looks like it has two arguments) or the detected behavior in _start correct where it looks like no arguments are being passed?

hjlbs commented 1 year ago

These are drivers that get two arguments to the entry function. The detected behavior in sub418 is correct. You can see that there are arguments used within the function and you mark the prototype correctly but that doesn't show in the caller at 0x408. I have tried annotating it manually then having binja update and reanalyze but unfortunately is doesn't populate.

The GUI won't let me overwrite the call type of _start manually but I can via the api: entryFunction.function_type = binaryninja.Type.function(binaryninja.Type.int(8), params=[binaryninja.Type.int(8), binaryninja.Type.int(8)] )

You can see that the function prototype of _start changes but it doesn't appear to propagate that knowledge. Screenshot from 2023-02-17 12-52-02

hjlbs commented 1 year ago

The calling convention is standard win64, rcx, rdx, etc. Is there a way to "attach" arg1 to rcx and arg2 to rdx and continue with analysis?

psifertex commented 1 year ago

To automate overriding the detected convention/arguments, you could use something like this.

fn = bv.get_function_at(0x418)
for site in fn.caller_sites:
    site.function.set_call_type_adjustment(site.address, fn.type)
psifertex commented 1 year ago

FWIW -- if you're doing it in the python console there's basically an explicit update_analysis_and_wait() every time you hit enter so those last two lines in your screenshot shouldn't ever do anything different (if that's not true, it's a bug, let us know!)

hjlbs commented 1 year ago

That works. I had to combine it with some other code but thanks! You want fn.function_type.

Ended up with this code:

entryFunction.apply_auto_discovered_type( binaryninja.Type.function(binaryninja.Type.int(8), params=[binaryninja.Type.int(8), binaryninja.Type.int(8)] ) )
binaryView.update_analysis_and_wait()
entryFunction.merge_vars(entryFunction.get_variable_by_name('arg1'), entryFunction.get_variable_by_name('rcx'))
entryFunction.merge_vars(entryFunction.get_variable_by_name('arg2'), entryFunction.get_variable_by_name('rdx'))
binaryView.update_analysis_and_wait()

## Now, loop through all the instructions in the _start function and any calls need to be updated
for callee in entryFunction.callees:
    print('Callee', callee)
    for site in callee.caller_sites:
        site.function.set_call_type_adjustment(site.address, callee.function_type)
binaryView.update_analysis_and_wait()

After this, the analysis using mlil refs lets me follow the _start parameters. Thanks again. You can close this if you like. I don't know if you consider it a bug or not.

Also, the Typing stuff you have on your page: https://docs.binary.ninja/guide/type.html#function-types

Type.function(params=[(Type.int(4), 'arg1')])

Results in a thrown error. the params arg doesn't seem to take tuples anymore.

>>> Type.function(params=[(Type.int(4), 'arg1')])
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/home/hj/binaryninja/plugins/../python/binaryninja/types.py", line 2083, in function
    return FunctionType.create(ret, params, calling_convention, variable_arguments, stack_adjust)
  File "/home/hj/binaryninja/plugins/../python/binaryninja/types.py", line 2566, in create
    param_buf = FunctionBuilder._to_core_struct(params)
  File "/home/hj/binaryninja/plugins/../python/binaryninja/types.py", line 1121, in _to_core_struct
    raise ValueError(f"Conversion from unsupported function parameter type {type(param)}")
ValueError: Conversion from unsupported function parameter type <class 'tuple'>
psifertex commented 1 year ago

Oh, .type works on latest dev, I changed that a little while ago and we're marking .function_type as deprecated in an upcoming release since it was rather redundant.

psifertex commented 1 year ago

It takes tuples, but apparently they're just backward: Type.function(params=[('arg1', Type.int(4))]) works

I'll update the docs, thanks.

psifertex commented 1 year ago

FYI -- the docs are updated, I'm only leaving this open while we investigate whether there's any other automated improvements we can make. The original blocker was removed just seeing if there's anything else we can do to fix the ambiguity of the original function (there might not be!) before closing...

psifertex commented 1 year ago

We've added UEFI support which should resolve all related issues here.