fluxchief / binaryninja_avr

Binaryninja AVR architecture plugin with lifting
MIT License
41 stars 10 forks source link

removed from plugin manager #15

Open psifertex opened 1 year ago

psifertex commented 1 year ago

Apologies for the late notice, we removed this from the plugin manager some time last year due to:

https://github.com/fluxchief/binaryninja_avr/blob/master/__init__.py#L416-L418

I'd love to re-add it back once that is resolved. Very sorry for not notifying you directly earlier (don't remember if we talked via another mechanism or not)

fluxchief commented 1 year ago

You actually mentioned this in issue #14, so this was no surprise. As explained in the bug, fixing this block is actually a bit challenging there are no magic bytes which can be used to differentiate between a valid AVR binary or just a random blob. It might be possible to work around this with some heuristics, however that's where the second issue comes into play: different AVR chips have different amounts of ISRs and rom sizes. A heuristic that checks that ISRs actually point into the image needs to know how many ISRs exists for the specific chip. A better solution would be to have to explicitly open the bin with the architecture plugin and having to select the corresponding chip. Realistically I still don't see myself fixing this anytime soon, haven't been doing lots of RE work lately (and don't even have a up2date BN)

Closing this as dupe of #14

fluxchief commented 1 year ago

On a second thought let's keep it open so that it's clear to others why this is not part of the plugin repo

fluxchief commented 1 year ago

With the "linear sweep" pass added to BN, creating the ISR routines is not as important anymore - that code is currently disabled. The "apply BV during open-with-options" would work, but ideally we'd figure out why it is locking up to begin with.

psifertex commented 1 year ago

So I think the link I had above is incorrect due to changes in the file. The problem isn't adding the ISR functions but the is_valid always returning true which means this view gets called every time and not only slows down analysis for all other files but means any bug in the lifting or analysis can cause problems even with files that aren't AVR. That looks to still be true and would have to change before it can be re-added.

You can also register as an ELF handler even so that elf based on AVR will still automatically load like: https://github.com/Vector35/arch-mips/blob/master/arch_mips.cpp#L2136-L2139

Basically, just call register_arch with the appropriate ident.

That said, yeah, I don't think we have a good way to let the user override which BinaryViews are loaded for a given file, but it sounds like something that would be a good idea and easy enough to do. Lemme talk to the team and see if there's any concerns.

psifertex commented 1 week ago

I believe this to be unblocked now with the resolution of #3721.

Happy to submit a PR, but the TL;DR is implement is_force_loadable and have it return true in your BinaryView. This will prevent it from applying to all files but let the user specify they'd like to load it using "open with options".

fluxchief commented 1 week ago

Awesome, thanks for letting me know. I'll have a look but probably won't be able to do fix this in the next week. Happy to accept your PR if you feel like sending one before that.

psifertex commented 1 week ago

Totally fine, no rush on that.