Skip to content

ensure Nyx command handler support is more CPU agnostic (#3153) #3167

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

ensure Nyx command handler support is more CPU agnostic (#3153) #3167

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions libafl_qemu/src/command/nyx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ macro_rules! define_nyx_command_manager {
#[deny(unreachable_patterns)]
fn parse(&self, qemu: Qemu) -> Result<Self::Commands, CommandError> {
let arch_regs_map: &'static EnumMap<ExitArgs, Regs> = get_exit_arch_regs();
let nyx_backdoor = qemu.read_reg(Regs::Rax)? as c_uint;
let cmd_id = qemu.read_reg(Regs::Rbx)? as c_uint;
let nyx_backdoor = qemu.read_reg(arch_regs_map[ExitArgs::Ret])? as c_uint;
let cmd_id = qemu.read_reg(arch_regs_map[ExitArgs::Cmd])? as c_uint;
Copy link
Member

Choose a reason for hiding this comment

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

LibAFL/libafl_qemu/src/arch/x86_64.rs

Line 45 in f16ddf2

ExitArgs::Cmd => Regs::Rax,

Cmd is not Rbx (should it be?)

Copy link
Author

Choose a reason for hiding this comment

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

Good call out!

Short answer for Nyx; yes. There are other changes needed though; e.g. depend upon Nyx support actually being built (disabled by default in CI/Docker builds, etc already). I no longer think this is the best abstraction to rely upon.

I confirmed that this should be more agnostic; hypercall macros defined in the Nyx packer differentiate between x86 and x86_64 (and of course, emitted code for guests). If I understand correctly, EAX/RAX will vary based on usage of kAFL vs others with and without Intel-PT but changes when trying to avoid collisions with VMware, etc. The Nyx packer code handles more and the code I patched would really be the most common config. The "subcommand" relevant to Nyx appears to be in EBX/RBX and is just a byte, based on the GCC asm modifiers.

I've produced a full Nyx test environment by dual-booting an Intel Core i7 10th Gen. This gets me everything I need; Intel-PT, etc. Everything builds. IIRC I encountered some test failures in other places but had to context switch.

This week, I will prioritize revisiting this PR and can expand further.

Copy link
Member

Choose a reason for hiding this comment

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

I consider this PR to be fix to #3153
for that purpose this PR is ready to merge?

Copy link
Member

Choose a reason for hiding this comment

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

this was written with only the x64 intel pt usecase indeed, so i'd expect to see issues in another scenario.
the nyx packer always uses [er]ax to detect a nyx hypercall and [er]bx / [er]cx for the arguments.
i don't mind supporting more things that the x64 intel pt mode, but i think it's important we keep the same behavior as the original nyx API, otherwise it makes the main use of this command handler useless.
the original intention was to be able to support nyx targets without having to touch them.
if you have time to revisit the pr it would be appreciated

Copy link
Member

Choose a reason for hiding this comment

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

then this file should be disabled for i386 right?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe use the fix for i386 for people that want/need it, and use the current register assignments for 64 bit?

Copy link
Member

@tokatoka tokatoka Jun 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

but romain said it's gonna work only with amd64

Copy link
Member

Choose a reason for hiding this comment

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

btw now without the restriction of CI runner. i386, arm can be added to CI


// Check nyx backdoor correctness
debug_assert_eq!(nyx_backdoor, 0x1f);
Expand Down
Loading