Skip to content

Bleeding Edge Kernel versions drop module_sect_attr #1761

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.

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

Open
atcuno opened this issue Apr 8, 2025 · 11 comments · May be fixed by #1773
Open

Bleeding Edge Kernel versions drop module_sect_attr #1761

atcuno opened this issue Apr 8, 2025 · 11 comments · May be fixed by #1773
Assignees
Milestone

Comments

@atcuno
Copy link
Contributor

atcuno commented Apr 8, 2025

This is a struct we currently rely on to extract kernel modules. This structure has been there since the beginning of the kernel but was removed with this commit:

torvalds/linux@34f5ec0

A slack user reported the bug from a preview/bleeding edge Ubuntu kernel, so this will hit mainstream kernels soon enough. I will investigate this week so its fixed for parity.

@atcuno atcuno self-assigned this Apr 8, 2025
@ikelos
Copy link
Member

ikelos commented Apr 8, 2025

I hate to say it, but this is a new feature ("support recent kernels") rather than a fix for something that currently works incorrectly, and I don't think it's enough to break the code freeze. It doesn't yet affect mainstream kernels (you said it was bleeding edge), the existing code isn't broken it just can't handle all cases at the moment (which will always be the case in a moving world) and we have to cut at some point. Feel free to get it fixed in develop, and it'll get released in July or August, whenever our next release is due to be rolled.

@ikelos ikelos added this to the 2.26.1 milestone Apr 8, 2025
@ikelos
Copy link
Member

ikelos commented Apr 8, 2025

It's not a parity release issue since I'm pretty sure volatility 2 will not handle this case.

@atcuno
Copy link
Contributor Author

atcuno commented Apr 8, 2025

The updated method for module attributes (the reason for this ticket) is in released Ubuntu preview kernels now, which means over the next couple weeks they will go into stable kernels being used in production. It is also not a silent issue, it fully backtraces all of Vol3 for any Linux analysis as we set the extension for module_sect_attr on load. So even running something like linux.pslist on the kernel (unrelated to module extraction) on an affected symbol table will backtrace the whole framework. This is a bug fix type ticket which is supposed to be the purpose of the gap time between now and the parity release. It really does need to be fixed for parity.

@ikelos
Copy link
Member

ikelos commented Apr 8, 2025

Well, I'll take a look at the PR once it comes out to see how invasive it is, but targetting a fix for something that comes out in two weeks, is still pre-empting the future, so I'm absolutely middle of the line on this and it could go either way...

@ikelos
Copy link
Member

ikelos commented Apr 8, 2025

(It also proves we need to be careful about whether we add class overrides or optional class overrides when we add extensions).

@atcuno
Copy link
Contributor Author

atcuno commented Apr 8, 2025

Yes, but two weeks from now is during the bug release period. Also, that would put 2-3 months of Linux analysis on up to date kernels backtracking and we just be telling people to use devleop for 2 months instead of the release.

Also, we do use the optional classes correctly, and it is good we didn't use it here as its a core data structure that hasn't changed since ~15 years - but of course it gets changed right before release. It would be like EPROCESS suddenly disappearing on Windows.

@ikelos
Copy link
Member

ikelos commented Apr 8, 2025

Ok, I'm glad we chose the right type for the structure, and yes, it's very disappointing they decided to make that change now.

As I said, I take a look and see how risky a change it is, but we have to balance it against putting in the change and us getting it wrong or missing something and breaking volatility (perhaps for every linux image, not just new ones) which is why I'm not giving you a guaranteed yes, or a guaranteed no now, but we need to see what the patch looks like before we decide whether it's better to include it now or not. The fact that it breaks all linux analysis rather than just one plugin does swing things in favour of including it.

@Abyss-W4tcher
Copy link
Contributor

Here are my researches towards a fix, if it can help you determine which way to go.

Doc.

At first glance, the only place where we require "module_sect_attr" is in get_sections (which by the way doesn't seem referenced anywhere?):

arr = self._context.object(
symbol_table_name + constants.BANG + "array",
layer_name=self.vol.layer_name,
offset=self.sect_attrs.attrs.vol.offset,
subtype=self._context.symbol_space.get_type(
symbol_table_name + constants.BANG + "module_sect_attr"
),
count=self.number_of_sections,
)

sect_attrs is of type module_sect_attrs

Diff is here:

// https://elixir.bootlin.com/linux/v6.13/source/kernel/module/sysfs.c#L27
struct module_sect_attrs {
	struct attribute_group grp;
	unsigned int nsections;
	struct module_sect_attr attrs[];
};

->

// https://elixir.bootlin.com/linux/v6.14-rc6/source/kernel/module/sysfs.c#L22
struct module_sect_attrs {
	struct attribute_group grp;
	struct bin_attribute attrs[];
};

And the now removed module_sect_attr was:

struct module_sect_attr {
	struct bin_attribute battr;
	unsigned long address;
};

module_sect_attr was removed because it was indeed only a proxy to a bin_attribute structure through its battr member.

Fix

  • Make module_sect_attr optional
diff --git a/volatility3/framework/symbols/linux/extensions/__init__.py b/volatility3/framework/symbols/linux/extensions/__init__.py
index b4bde1ab..74ad205b 100644
--- a/volatility3/framework/symbols/linux/extensions/__init__.py
+++ b/volatility3/framework/symbols/linux/extensions/__init__.py
@@ -212,9 +212,7 @@ class module(generic.GenericIntelProcess):
             symbol_table_name + constants.BANG + "array",
             layer_name=self.vol.layer_name,
             offset=self.sect_attrs.attrs.vol.offset,
-            subtype=self._context.symbol_space.get_type(
-                symbol_table_name + constants.BANG + "module_sect_attr"
-            ),
+            subtype=self.sect_attrs.attrs.vol.subtype,
             count=self.number_of_sections,
         )

Then add an optional "bin_attribute" class override with a "get_name" method that should basically only need to mimic

return utility.pointer_to_string(self.battr.attr.name, count=32)

@Abyss-W4tcher
Copy link
Contributor

I'll potentially update the previous comment if I find anything else, so consider this WiP for today.

@ikelos
Copy link
Member

ikelos commented Apr 8, 2025

So it sounds like the fix would be to make a module_sect_attrs class override, that determines whether it contains the needed classes, and then either processes attrs to yields each of the bin_attributes out of the module_sect_attrs or yields through a direct array of bin_attributes?

@Abyss-W4tcher
Copy link
Contributor

Abyss-W4tcher commented Apr 9, 2025

I don't think there is a need to tamper with module_sect_attrs, as the attrs array subtype class override (bin_attribute or module_sect_attr) will already handle members checking. This makes it transparent to get_name() callers, like here (because this one does not hardcode module_sect_attr in the subtype):

attribute_type = module.sect_attrs.attrs.vol.subtype
sect_array = vmlinux.object(
object_type="array",
subtype=attribute_type,
offset=module.sect_attrs.attrs.vol.offset,
count=num_sections,
absolute=True,
)
sections: Dict[int, str] = {}
# for each section, gather its name and address
for index, section in enumerate(sect_array):
name = section.get_name()

I can make a PR with how I think it could be resolved, so that it would a bit clearer of what would move and if it's too risky or not before 2.26.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants