Add systemmode hw_breakpoint libafl set/remove fns #93
Conversation
|
@rmalmain this should be ready |
libafl/system.c
Outdated
| } | ||
|
|
||
| // let's add/remove the breakpoint on every CPU | ||
| CPU_FOREACH(cs) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Why? :)
Most likely we are going to have just one cpu anyway, so most of the times the parameter would just be annoying ?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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_FOREACHmacro? 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.
There was a problem hiding this comment.
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
| @@ -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); | |||
There was a problem hiding this comment.
i didn't check yet, can we use normal hw breakpoints if this gets removed?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
reserve hw breakpoints for LibAFL, using them with GDB will return a graceful error. So far they are implemented only for kvm.