Skip to content
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

libselinux: make threadsafe for discover_class_cache #336

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

purushottamc
Copy link

Crash is observed in process dbus-daemon while accessing name
from discover_class_cache structure variable,
discover_class_cache->name variable found NULL
during backtrace analysis.
Add mutex lock for the discover_class_cache to handle multiple
threads for the function which uses discover_class_cache
This avoids variable corruption during parallel access
in the multiple thread environment.

Crash is observed in process dbus-daemon while accessing name
from discover_class_cache structure variable,
discover_class_cache->name variable found NULL
during backtrace analysis.
Add mutex lock for the discover_class_cache to handle multiple
threads for the function which uses discover_class_cache
This avoids variable corruption during parallel access
in the multiple thread environment.
@cgzones
Copy link
Contributor

cgzones commented Dec 17, 2021

I think there might still be an issue left:
Consider thread A calling string_to_security_class() calling get_class_cache_entry_name(), acquiring the mutex, getting the static discover_class_cache pointer, searching the required node, releasing the mutex and returning the node pointer back to string_to_security_class().
Now while operating in string_to_security_class() on the returned node pointer thread B calls selinux_flush_class_cache(), which invalidates all node pointers behind the static discover_class_cache and thus thread A will access free'd memory.

Also as by https://github.com/SELinuxProject/selinux/blob/master/CONTRIBUTING.md#contributing-code all patches require a Signed-off-by and needs to be send to [email protected].

@purushottamc
Copy link
Author

Hi @cgzones,
Sent patch to [email protected]
Did changes as per above review comments but changes are not working as expected with our build system:
https://lore.kernel.org/selinux/[email protected]/
Could you please have a look into it?

@cgzones
Copy link
Contributor

cgzones commented Jan 25, 2022

but changes are not working as expected with our build system

Does it not build, are there issues in single thread mode, are there issues with multiple threads?

For the multiple threads case:
security_av_perm_to_string() might still return an invalidated pointer and calls to (un)?map_(class|perm) might still be racy due to the process global variable current_mapping in mapping.c.

Another note on the sent patch: returning a copy from security_class_to_string() should probably communicated to all (also third-party, fortunately there seem to be not much (https://codesearch.debian.net/search?q=security_class_to_string&literal=1)) callers to avoid memory leaks and the type should by changed to char* to allow free'ing the memory without a dangerous const-cast.

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