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.

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

Add systemmode hw_breakpoint libafl set/remove fns #93

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add systemmode hw_breakpoint libafl set/remove fns #93

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3fd9610
Add systemmode hw_breakpoint libafl set/remove fns
Marcondiro Dec 9, 2024
379824c
very bad kvm breakpoint hook yolo
Marcondiro Dec 11, 2024
fc8dcf4
cleanup
Marcondiro Dec 17, 2024
5baf91b
Prevent GDB from using HW breakpoints
Marcondiro Dec 18, 2024
f53a020
fix: hw breakpoint add/rm no loop over CPUs
Marcondiro Jan 7, 2025
f122e3c
Merge remote-tracking branch 'upstream/main'
Marcondiro Jan 8, 2025
00e1d7a
Merge remote-tracking branch 'upstream/main'
Marcondiro Jan 14, 2025
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
6 changes: 5 additions & 1 deletion accel/kvm/kvm-accel-ops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,11 @@ static void *kvm_vcpu_thread_fn(void *arg)
if (cpu_can_run(cpu)) {
r = kvm_cpu_exec(cpu);
if (r == EXCP_DEBUG) {
cpu_handle_guest_debug(cpu);
//// --- Begin LibAFL code ---
// cpu_handle_guest_debug(cpu);
Copy link
Member

Choose a reason for hiding this comment

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

i didn't check yet, can we use normal hw breakpoints if this gets removed?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I understand what you mean by normal hw breakpoints, through gdb? If so, no, with this pr the hw breakpoints are reserved to libafl internal usage

Copy link
Member

Choose a reason for hiding this comment

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

do we really need to make it impossible to use gdb hardware breakpoints?
i think we can add a very simple allocation system to differentiate libafl from normal breakpoints, with a bitmap or something similar.
in general, i try to avoid breaking QEMU features as much as possible to prevent future problems.

Copy link
Author

Choose a reason for hiding this comment

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

do we really need to make it impossible to use gdb hardware breakpoints?

Nah, but when I looked at it, it didn't look trivial to implement the breakpoint provenance tagging properly.

The question is more if it is worth it to keep compatibility and if it is a priority. For me no, considering that afaik gdb + libafl_qemu is already quite broken and imho if you want to use gdb is more convenient to do it with vanilla qemu and not with libafl_qemu.

i try to avoid breaking QEMU features as much as possible to prevent future problems.

In this case I think that implementing the breakpoint provenance tagging would increase the probability of having future problems since this PR's modifications are quite simple and not deep in QEMU's internals and convoluted as the tagging would be

Copy link
Member

Choose a reason for hiding this comment

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

Nah, but when I looked at it, it didn't look trivial to implement the breakpoint provenance tagging properly.

The question is more if it is worth it to keep compatibility and if it is a priority. For me no, considering that afaik gdb + libafl_qemu is already quite broken and imho if you want to use gdb is more convenient to do it with vanilla qemu and not with libafl_qemu.

there are some problems, true. i still use gdb with libafl qemu often to debug fuzzers. apart from breakpoints, i didn't encounter too many issues, and most of the common features seemed to work as expected.

In this case I think that implementing the breakpoint provenance tagging would increase the probability of having future problems since this PR's modifications are quite simple and not deep in QEMU's internals and convoluted as the tagging would be

in that case, i'd at least issue an error or print something in gdbstub for incoming hw breakpoints.
it would reduce the confusion when using guest gdb in the future.
we could put this somewhere around there, after checking we are handling a hardware bp.

Copy link
Author

Choose a reason for hiding this comment

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

Yepp, it's already there :)

https://github.com/Marcondiro/qemu-libafl-bridge/blob/f53a020dc09236cd174357ea9ac2911a249d5edd/gdbstub/system.c#L659-L664

cpu->stopped = true;
libafl_qemu_trigger_breakpoint(cpu);
//// --- End LibAFL code ---
}
}
qemu_wait_io_event(cpu);
Expand Down
7 changes: 7 additions & 0 deletions gdbstub/system.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

//// --- Begin LibAFL code ---
#include "libafl/gdb.h"
#include "gdbstub/enums.h"
//// --- End LibAFL code ---

/* System emulation specific state */
Expand Down Expand Up @@ -655,6 +656,12 @@ bool gdb_supports_guest_debug(void)
int gdb_breakpoint_insert(CPUState *cs, int type, vaddr addr, vaddr len)
{
const AccelOpsClass *ops = cpus_get_accel();
//// --- Begin LibAFL code ---
// HW breakpoints are reserved for LibAFL
if (type == GDB_BREAKPOINT_HW) {
return -ENOSYS;
}
//// --- End LibAFL code ---
if (ops->insert_breakpoint) {
return ops->insert_breakpoint(cs, type, addr, len);
}
Expand Down
8 changes: 8 additions & 0 deletions include/libafl/system.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
#pragma once

#include "hw/core/cpu.h"
#include "gdbstub/enums.h"
#include "sysemu/accel-ops.h"
#include "sysemu/cpus.h"

int libafl_qemu_set_hw_breakpoint(vaddr addr);
int libafl_qemu_remove_hw_breakpoint(vaddr addr);

void libafl_qemu_init(int argc, char** argv);
39 changes: 39 additions & 0 deletions libafl/system.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,43 @@

#include "libafl/system.h"

int libafl_qemu_toggle_hw_breakpoint(vaddr addr, bool set);

void libafl_qemu_init(int argc, char** argv) { qemu_init(argc, argv); }

int libafl_qemu_set_hw_breakpoint(vaddr addr)
{
return libafl_qemu_toggle_hw_breakpoint(addr, true);
}

int libafl_qemu_remove_hw_breakpoint(vaddr addr)
{
return libafl_qemu_toggle_hw_breakpoint(addr, false);
}

int libafl_qemu_toggle_hw_breakpoint(vaddr addr, bool set) {
const int type = GDB_BREAKPOINT_HW;
const vaddr len = 1;
const AccelOpsClass *ops = cpus_get_accel();

CPUState* cs;
int ret = 0;

if (!ops->insert_breakpoint) {
return -ENOSYS;
}

// let's add/remove the breakpoint on every CPU
CPU_FOREACH(cs) {
Copy link
Member

Choose a reason for hiding this comment

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

i don't think it's a good idea to insert the hw bp for each CPU. better to take CPUState* as parameter and use it.

Copy link
Author

Choose a reason for hiding this comment

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

Why? :)
Most likely we are going to have just one cpu anyway, so most of the times the parameter would just be annoying ?

Copy link
Member

Choose a reason for hiding this comment

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

multiple reasons:

  • most of our api does not make assumption about CPU, and takes a CPU as parameter when necessary
  • it's not so straightforward to say we will always have to handle a single CPU, there are more tricky cases that could require multiple CPUs for a single fuzzing instance (think of a system using a coprocessor for instance).

Copy link
Author

Choose a reason for hiding this comment

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

most of our api does not make assumption about CPU, and takes a CPU as parameter when necessary

It doesn't look like it is the case for sw breakpoints int libafl_qemu_set_breakpoint(target_ulong pc);, do we want to have different signatures between hw and sw breakpoints?

it's not so straightforward to say we will always have to handle a single CPU, there are more tricky cases that could require multiple CPUs for a single fuzzing instance (think of a system using a coprocessor for instance).

mmm, are you sure the coprocessor would be included by the CPU_FOREACH macro? it looked more like a main cpu(s) cores iterator to me

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like it is the case for sw breakpoints int libafl_qemu_set_breakpoint(target_ulong pc);, do we want to have different signatures between hw and sw breakpoints?

for sw breakpoints, it's a bit different. since we are not changing the architectural state of the CPU (we check if we triggered a breakpoint address or not independently of the core triggering the bp), we only need to call the bp callback whenever it gets hit (we give the CPUState of the core that triggered the bp there).

for hw breakpoint, we actually change the state of the CPU(s) to select when a breakpoint should be triggered.
so in theory, we could choose which core should stop when the breakpoint is reached.

nonetheless, qemu seems to inject the hw breakpoint to every running core as well. in that case, i guess we can keep the same behavior.

mmm, are you sure the coprocessor would be included by the CPU_FOREACH macro? it looked more like a main cpu(s) cores iterator to me

i think so, cpus are always added to the global list when they get realized. in any case, if qemu sets the hw breakpoints using CPU_FOREACH, it should be fine.

Copy link
Author

@Marcondiro Marcondiro Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

nonetheless, qemu seems to inject the hw breakpoint to every running core as well. in that case, i guess we can keep the same behavior.

Good catch, this can actually be problematic, I removed my loop

if (set) {
ret = ops->insert_breakpoint(cs, type, addr, len);
} else {
ret = ops->remove_breakpoint(cs, type, addr, len);
}
if (ret != 0) {
return ret;
}
}

return 0;
}