Closed gcmoreira closed 1 month ago
Sorry for the last minute improvement. I realized that an extension object was more appropriate here, since it was already attached to the rb_root type via rb_node, rb_left, and rb_right.
It is now ready for review
Reading #1187, does this PR require symbols generated with a patched dwarf2json
? I think we have ways of checking the version of a symbol table, but I can't tell whether we need to for this PR or not?
@ikelos There's a bug in dwarf2json that prevents generating valid ISFs. Based on #63 and #57, it affects Linux kernel version 6.5+, which is pretty bad. Since this ticket addresses changes in the mount namespace for kernels 6.8+, it's indirectly affected.
One of the comments suggests a workaround that’s currently awaiting review. I'm not sure if it's the best way to fix it, but it worked for me.
Do you want to block this until they fix that bug in dwarf2json and make it dependent on the new version?
Well, what I'm trying to avoid is users using an old symbol table, hitting whatever the bug is and then filing a new bug for a known problem. The question is how best to warn about the bad table. We can either do it on load of the bad table ("warning, you need to regenerate this using dwarf2json at least x.x.x") or whether we only do the check in known fields. I don't think our cache database keeps the metadata (which perhaps it should, since it'll have read all the symbols at least once) so that we could do full sweeps of their stores (the warnings may get annoying if we do that on every run and they don't care though?). The other options are to only flag on particular plugins that are affected, or to just do nothing and have vol failed on bad symbol tables without much notice. I'd prefer to avoid that last one unless it's a vanishingly small percentage change of affecting users (and we're willing to handle bugs people bring up about it).
I think we've already got some code somewhere that checked for a symbol table's version, I think that was a per plugin check, lemme see if I can find it... Yeah, it's the kmsg
plugin, so at the moment that's our template for dealing with it, I guess?
So we've got a self.context.symbol_space.verify_table_versions
method for checking both creator and version of the symbol table. I guess we stick that on plugins that may be affected by it? Does that sound a reasonable solution @gcmoreira ?
Yeah, the dwarf2json issue hasn't been fixed yet, so there's no specific version to use for that check. There's no guarantee that it will be fixed in the next version, so using something like version > current dwarf2json version
might not work either.
Also, this issue isn't limited to this plugin or the filesystem subsystem, so checking that here won't prevent users from filing new bug reports. As a proof of this, see for instance this note in Abyss-W4tcher's volatility3-symbols
Resolved conflict following the recent merge of Linux Page Cache and IDR
@ikelos https://github.com/volatilityfoundation/dwarf2json/pull/66 and https://github.com/volatilityfoundation/dwarf2json/issues/57 are done. Regarding our last comments, you want me to lock this to the dwarf2json 0.9.0 version?
Yeah, just wondering if it needs to be across all plugins or just specific ones? I made PR #1305 without remembering about the function I already added, but I dunno whether it should be applied in a plug-in but phone basis (plus my PR breaks tests so I need to figure that out first...)
The dwarf2json issue is not limited to this plugin, it could happen with any type that has a binding in Rust. If we want to add this check, it needs to be across all plugins.
For instance:
$ pahole ./vmlinux-6.8.0-41-generic -C sockaddr
struct sockaddr {
sa_family_t sa_family; /* 0 2 */
union {
char sa_data_min[14]; /* 2 14 */
struct {
struct {
} __empty_sa_data; /* 2 0 */
char sa_data[0]; /* 2 0 */
}; /* 2 0 */
}; /* 2 14 */
/* size: 16, cachelines: 1, members: 2 */
/* last cacheline: 16 bytes */
};
$ pahole ./vmlinux-6.8.0-41-generic --lang rust -C sockaddr
struct sockaddr {
u16 sa_family __attribute__((__aligned__(2))); /* 0 2 */
struct sockaddr__bindgen_ty_1 __bindgen_anon_1 __attribute__((__aligned__(1))); /* 2 14 */
/* size: 16, cachelines: 1, members: 2 */
/* forced alignments: 2 */
/* last cacheline: 16 bytes */
} __attribute__((__aligned__(2)));
As you can see above, the Rust bindings don't have all members. Another example:
$ pahole ./vmlinux-6.8.0-41-generic -C vm_area_struct | grep vm_start | wc -l
1
$ pahole ./vmlinux-6.8.0-41-generic --lang rust -C vm_area_struct | grep vm_start | wc -l
0
If dwarf2json
ends up using one of those bindings instead of the original kernel type to generate the ISF, Volatility3 will eventually fail at some point.
This is dependent on #1305 then. Looks like we've got a pretty good workable solution, we just need to get it developed and merged in...
I agree it could be better if #1305 goes first.. but technically it doesn't depend on that one. I wonder if it's possible to merge this as is? It’s been around for a while now, pretty much every distro currently has kernels >= 6.8 and there's another bug report on the way that depends on this merge to be triggered
In the context of volatilityfoundation/volatility3#1187, this PR adds support for the mount namespace in kernels 6.8+. A basic Red-Black tree abstraction was also introduced to enable reuse in other parts of the framework if necessary.