Skip to content

Linux: enhance VMA enumeration smearing protection #1814

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
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Abyss-W4tcher
Copy link
Contributor

Hi,

During mass testing, we observed many VMA entries being broken due to smearing. This PR introduces sanity checks to ensure that the extracted values (start, end, length...) are coherent.

Here are a few examples of broken entries that are now filtered out:

{"PID":1057,"Process":"python","Start":9223372037176023143,"End":9223372037176047719,"Flags":"rwx","PgOff":37778931864109161279488,"Major":0,"Minor":0,"Inode":0,"File Path":null,"File output":"Disabled"}
{"End": 140210914017280, "File Path": "/usr/lib/x86_64-linux-gnu/libsmime3.so", "File output": "Disabled", "Flags": "r-x", "Inode": 396487, "Major": 8, "Minor": 1, "PID": 2710, "PgOff": 0, "Process": "thunderbird", "Start": 649032930659012321, "__children": []}

Copy link
Member

@ikelos ikelos left a comment

Choose a reason for hiding this comment

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

So... I'm ok with the code and the addition, I'd just take the opportunity for a version bump to move the changed code out into a versionable component itself...

@@ -1,7 +1,7 @@
# We use the SemVer 2.0.0 versioning scheme
VERSION_MAJOR = 2 # Number of releases of the library with a breaking change
VERSION_MINOR = 26 # Number of changes that only add to the interface
VERSION_PATCH = 2 # Number of changes that do not change the interface
VERSION_MINOR = 27 # Number of changes that only add to the interface
Copy link
Member

Choose a reason for hiding this comment

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

Given this is a MINOR version bump, did you want to take the opportunity to slice off the methods that are changing into their own module (similar to the tainting stuff)? I'm just keen that rather than needing framework version bumps, we get to a place where we can just make component version bumps...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving the vm_area_struct methods into a versioned utility like the tainting module would remove all the object overloading features, and require to call an external function with the object as a parameter.

Versioning type overloading modules is not something that we currently do anyways?

Copy link
Member

Choose a reason for hiding this comment

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

Ugh, you're right but we should probably start planning out a way to allow such changes to be versioned. For now, I don't think adding the _is_valid method is necessarily all that useful (particularly if the only consider is internal) so perhaps we should make it a non-exposed internal function, and then we can expose it later if we feels it's necessary/valuable? That lets us delay the decision and figure out how we want to handle versioning type overrides in the future...

Copy link
Contributor Author

@Abyss-W4tcher Abyss-W4tcher May 22, 2025

Choose a reason for hiding this comment

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

Technically this method is public, as it is called by the mm_struct module which is outside of vm_area_struct?

Versioning type overrides might be tricky, as it'll most likely require any user of the overrides to specify the version requirement somewhere. This could result in a lot of internal cross requirements, extended get_requirements() lists etc. Importing those functions can be done silently (e.g. my_vma_object.is_valid(), no python header import), making it hard to track.

Relying on the framework version requires to bump it more frequently, but I guess that suits this use case well?

@ikelos
Copy link
Member

ikelos commented May 22, 2025

Oh, I missed that, I thought it was all part of the same struct.

Yeah, it will be tricky, but otherwise we need to be really careful about changes (even additions) because any changes down the line will be with us for a couple of years at a minimum and are tricky to clear up. is_valid seems reasonable and not too complex, but it also isn't clear how useful it would be to many others.

I'm not sure how we cope with this? We could do it this way, or could just put the changes into the only consumer of the code at the moment. We probably can't avoid endless bumps, but there's got to be a better way for something as trivial as adding an is_valid method... 5:S

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 this pull request may close these issues.

2 participants