Second round of usermode snapshot fixes

[cube0x8]

• Added brk callback into trace_mmap_snapshot: now we hook brk and change the mappings in the interval tree based on the brk return values
• Added IntervalSnapshotFilter for QemuSnapshotHelper: we can create DenyList and AllowList with memory address ranges to respectively filter out, or include, memory areas in the snapshot. For example, if you want to exclude the mutated input buffer from the snapshot or include specific memory areas that are not being traced by read/write callbacks etc.

[rmalmain]

I'm not sure if this is something we should expose like this.
It's great to have this around for debugging or dirty fix as last resort, but I'm scared people rely too much on this instead of reporting bugs in the snapshot systems.
Also, it would be nice to have this work generically with the IsSnapshotManager maybe? I didn't implement the trait for usermode snapshot yet, but I guess it could be the right moment to do so.

[rmalmain]

Or maybe we could emit a warning when using this asking to issue a bug report or something similar?

[domenukk]

You could add a #[deprecated] flag that will show at compile time? (But feels a bit funny to add a new API as deprecated)

[rmalmain]

I doesn't seem there is another way to issue warnings at compile time in stable, I guess it's a good alternative

[cube0x8]

Sorry for being late but I couldn't get on it last week. Btw, what if we just keep the deny list and that's it? Or we could just pass a simple vector of GuestAddr without using filters at all.

…

On Sun, 12 May 2024, 16:53 Romain Malmain, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In libafl_qemu/src/helpers/snapshot.rs <#2140 (comment)>: > @@ -73,6 +73,12 @@ pub struct MappingInfo { pub size: usize, } +pub enum IntervalSnapshotFilter { I doesn't seem there is another way to issue warnings at compile time in stable, I guess it's a good alternative — Reply to this email directly, view it on GitHub <#2140 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGW4SJWQ3MNN3A3ZUAFYUTZB5X6JAVCNFSM6AAAAABHGWCZ5SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJRGM4DEOBSG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[domenukk]

@rmalmain merge?

[cube0x8]

To be more clear, these are two alternatives:

1. We don't use filters at all. We rename fn with_filters in fn with_exclude_list and we pass as argument a simple vector of GuestAddr. In this way, we just keep a list of ranges to exclude, and we cannot force snapshot on certain area.
2. We keep it as it is. If you look at the code, I added the possibility to have an AllowList but there is no logic that forces those allowed areas to be snapshotted. In other words, pageflags_root is the only thing that is going to be snapshotted and that AllowList won't change its behaviour. This said, that AllowList was there only as an idea for the future. I don't think we actually need it, because we still have paranoid_debug to identify bugs at snapshot time.

[rmalmain]

Let's keep it as it is. I'm not sure it's good practice to use this filter still, but most likely if you use it in the first place means you know what you're doing. We can always add a warning or only enable this in debug mode in the future if the feature is abused later on.

[domenukk]

Just add a comment to the snapshot filter that bugs should be reported?

[cube0x8]

Good idea

…

On Tue, 21 May 2024, 12:27 Dominik Maier, ***@***.***> wrote: Just add a comment to the snapshot filter that bugs should be reported? — Reply to this email directly, view it on GitHub <#2140 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGW4SL36W54ERO6PGJT42LZDMHQBAVCNFSM6AAAAABHGWCZ5SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRSGE4DENJRGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[rmalmain]

Sounds good