search

SELinuxProject / selinux

This is the upstream repository for the Security Enhanced Linux (SELinux) userland libraries and tools. The software provided by this project complements the SELinux features integrated into the Linux kernel and is used by Linux distributions. All bugs and patches should be submitted to selinux@vger.kernel.org
Other
1.35k stars 360 forks source link

libselinux: make threadsafe for discover_class_cache #336

Open purushottamc opened 2 years ago

purushottamc commented 2 years ago

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 commented 2 years ago

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 selinux@vger.kernel.org.

purushottamc commented 2 years ago

Hi @cgzones, Sent patch to selinux@vger.kernel.org Did changes as per above review comments but changes are not working as expected with our build system: https://lore.kernel.org/selinux/20220120073329.15234-1-purushottamchoudhary29@gmail.com/ Could you please have a look into it?

cgzones commented 2 years ago

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.