libselinux: make threadsafe for discover_class_cache by purushottamc · Pull Request #336 · SELinuxProject/selinux

purushottamc · 2021-12-17T11:55:41Z

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 · 2021-12-17T14:14:28Z

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 · 2022-01-25T11:10:35Z

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 · 2022-01-25T15:32:14Z

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.

stephensmalley · 2026-06-12T13:04:37Z

Re-opening since this still seems to be an unresolved issue; may try to revisit this.

…s set The AVC has a set of locking calls but defaults to no locking, and only the deprecated avc_init() allowed applications to set their own locking callbacks. Since selinux_check_access() is now commonly used by applications implementing userspace SELinux permission checks and it uses avc_open() internally, such calls are currently not thread-safe. Provide fallbacks to using pthread mutexes by default. Single-threaded programs that do not link -lpthread will be unaffected due to the existing __pthread_mutex() helpers. Applications that explicitly specify locking callbacks via the deprecated avc_init() will also be unaffected. This is the first step toward a thread-safe selinux_check_access(); the class-string cache and status-page transition handling are addressed in follow-up changes. Fixes: SELinuxProject#287 SELinuxProject#335 SELinuxProject#336 Link: https://lore.kernel.org/selinux/CAJsHiNx1E7x1jaBkS0i4L1nBWXp8YXLHRWcCaDaH4LOn=zm+Zw@mail.gmail.com/ Link: https://lore.kernel.org/selinux/20220120073329.15234-1-purushottamchoudhary29@gmail.com/ Reported-by: Seth Moore <sethmo@google.com> Reported-by: Purushottam Choudhary <purushottamchoudhary29@gmail.com> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

The discover_class_cache list is accessed without locking by the string_to_*() and *_to_string() functions in libselinux for translating between string names and class/perm values, and returns pointers to strings within this list to callers. selinux_flush_class_cache() frees the list upon a policy reload, called by avc_process_policyload() from selinux_status_updated() and avc_netlink_process(). Make this safe for multi-threaded users by introducing and taking a mutex around the list accesses, and by moving flushed nodes to a retired list rather than freeing them so that returned string pointers remain valid. Given the infrequency of policy reloads, the relative stability of the class/perm mapping even across policy reloads, and the small amount of memory required, this seems a worthwhile tradeoff. If it becomes an issue, there are several options: 1. Only load/refresh new/modified class/permission names and values upon policy reload rather than flushing and reloading them all, 2. Provide a way to drain the retired list safely when the application knows it is no longer using any returned strings. Fixes: SELinuxProject#287 SELinuxProject#335 SELinuxProject#336 Link: https://lore.kernel.org/selinux/CAJsHiNx1E7x1jaBkS0i4L1nBWXp8YXLHRWcCaDaH4LOn=zm+Zw@mail.gmail.com/ Link: https://lore.kernel.org/selinux/20220120073329.15234-1-purushottamchoudhary29@gmail.com/ Reported-by: Seth Moore <sethmo@google.com> Reported-by: Purushottam Choudhary <purushottamchoudhary29@gmail.com> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

When selinux_check_access() is used by a multi-threaded application and calls selinux_status_updated(), every thread that observes the same sequence number bump races to run the avc_process_*() functions, so avc_ss_reset() and selinux_flush_class_cache() fire once per thread instead of once per event, and writes to the last_seqno/policyload can tear. Introduce and take a status_lock mutex across the last_seqno compare-and-update and the update actions so that exactly one thread services each event. The kernel status page itself is a seqlock and is still read unlocked via the existing read_sequence() loop. Lock ordering: 1. status_lock is never taken when another libselinux lock is held. 2. Under status_lock, the code may take avc_lock (via avc_ss_reset) or discover_class_lock (via selinux_flush_class_cache), but neither of these ever nest back into status_lock. Fixes: SELinuxProject#287 SELinuxProject#335 SELinuxProject#336 Link: https://lore.kernel.org/selinux/CAJsHiNx1E7x1jaBkS0i4L1nBWXp8YXLHRWcCaDaH4LOn=zm+Zw@mail.gmail.com/ Link: https://lore.kernel.org/selinux/20220120073329.15234-1-purushottamchoudhary29@gmail.com/ Reported-by: Seth Moore <sethmo@google.com> Reported-by: Purushottam Choudhary <purushottamchoudhary29@gmail.com> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

stephensmalley · 2026-06-12T17:19:40Z

I know it's been a minute since this PR was opened but PTAL at this RFC series, https://lore.kernel.org/selinux/20260612170856.17904-1-stephen.smalley.work@gmail.com/T/#t

The discover_class_cache list is accessed without locking by the string_to_*() and *_to_string() functions in libselinux for translating between string names and class/perm values, and returns pointers to strings within this list to callers. selinux_flush_class_cache() frees the list upon a policy reload, called by avc_process_policyload() from selinux_status_updated() and avc_netlink_process(). Make this safe for multi-threaded users by introducing and taking a mutex around the list accesses, and by moving flushed nodes to a retired list rather than freeing them so that returned string pointers remain valid. Given the infrequency of policy reloads, the relative stability of the class/perm mapping even across policy reloads, and the small amount of memory required, this seems a worthwhile tradeoff. If it becomes an issue, there are several options: 1. Only load/refresh new/modified class/permission names and values upon policy reload rather than flushing and reloading them all, 2. Provide a way to drain the retired list safely when the application knows it is no longer using any returned strings. Fixes: SELinuxProject#287 SELinuxProject#335 SELinuxProject#336 Link: https://lore.kernel.org/selinux/CAJsHiNx1E7x1jaBkS0i4L1nBWXp8YXLHRWcCaDaH4LOn=zm+Zw@mail.gmail.com/ Link: https://lore.kernel.org/selinux/20220120073329.15234-1-purushottamchoudhary29@gmail.com/ Reported-by: Seth Moore <sethmo@google.com> Reported-by: Purushottam Choudhary <purushottamchoudhary29@gmail.com> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

When selinux_check_access() is used by a multi-threaded application and calls selinux_status_updated(), every thread that observes the same sequence number bump races to run the avc_process_*() functions, so avc_ss_reset() and selinux_flush_class_cache() fire once per thread instead of once per event, and writes to the last_seqno/policyload can tear. Introduce and take a status_lock mutex across the last_seqno compare-and-update and the update actions so that exactly one thread services each event. The kernel status page itself is a seqlock and is still read unlocked via the existing read_sequence() loop. Lock ordering: 1. status_lock is never taken when another libselinux lock is held. 2. Under status_lock, the code may take avc_lock (via avc_ss_reset) or discover_class_lock (via selinux_flush_class_cache), but neither of these ever nest back into status_lock. Fixes: SELinuxProject#287 SELinuxProject#335 SELinuxProject#336 Link: https://lore.kernel.org/selinux/CAJsHiNx1E7x1jaBkS0i4L1nBWXp8YXLHRWcCaDaH4LOn=zm+Zw@mail.gmail.com/ Link: https://lore.kernel.org/selinux/20220120073329.15234-1-purushottamchoudhary29@gmail.com/ Reported-by: Seth Moore <sethmo@google.com> Reported-by: Purushottam Choudhary <purushottamchoudhary29@gmail.com> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

The discover_class_cache list is accessed without locking by the string_to_*() and *_to_string() functions in libselinux for translating between string names and class/perm values, and returns pointers to strings within this list to callers. selinux_flush_class_cache() frees the list upon a policy reload, called by avc_process_policyload() from selinux_status_updated() and avc_netlink_process(). Make this safe for multi-threaded users by introducing and taking a mutex around the list accesses, and by moving flushed nodes to a retired list rather than freeing them so that returned string pointers remain valid. Given the infrequency of policy reloads, the relative stability of the class/perm mapping even across policy reloads, and the small amount of memory required, this seems a worthwhile tradeoff. If it becomes an issue, there are several options: 1. Only load/refresh new/modified class/permission names and values upon policy reload rather than flushing and reloading them all, 2. Provide a way to drain the retired list safely when the application knows it is no longer using any returned strings. Fixes: SELinuxProject#287 SELinuxProject#335 SELinuxProject#336 Link: https://lore.kernel.org/selinux/CAJsHiNx1E7x1jaBkS0i4L1nBWXp8YXLHRWcCaDaH4LOn=zm+Zw@mail.gmail.com/ Link: https://lore.kernel.org/selinux/20220120073329.15234-1-purushottamchoudhary29@gmail.com/ Reported-by: Seth Moore <sethmo@google.com> Reported-by: Purushottam Choudhary <purushottamchoudhary29@gmail.com> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

When selinux_check_access() is used by a multi-threaded application and calls selinux_status_updated(), every thread that observes the same sequence number bump races to run the avc_process_*() functions, so avc_ss_reset() and selinux_flush_class_cache() fire once per thread instead of once per event, and writes to the last_seqno/policyload can tear. Introduce and take a status_lock mutex across the last_seqno compare-and-update and the update actions so that exactly one thread services each event. The kernel status page itself is a seqlock and is still read unlocked via the existing read_sequence() loop. Lock ordering: 1. status_lock is never taken when another libselinux lock is held. 2. Under status_lock, the code may take avc_lock (via avc_ss_reset) or discover_class_lock (via selinux_flush_class_cache), but neither of these ever nest back into status_lock. Fixes: SELinuxProject#287 SELinuxProject#335 SELinuxProject#336 Link: https://lore.kernel.org/selinux/CAJsHiNx1E7x1jaBkS0i4L1nBWXp8YXLHRWcCaDaH4LOn=zm+Zw@mail.gmail.com/ Link: https://lore.kernel.org/selinux/20220120073329.15234-1-purushottamchoudhary29@gmail.com/ Reported-by: Seth Moore <sethmo@google.com> Reported-by: Purushottam Choudhary <purushottamchoudhary29@gmail.com> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

…s set The AVC has a set of locking calls but defaults to no locking, and only the deprecated avc_init() allowed applications to set their own locking callbacks. Since selinux_check_access() is now commonly used by applications implementing userspace SELinux permission checks and it uses avc_open() internally, such calls are currently not thread-safe. Provide fallbacks to using pthread mutexes by default. Single-threaded programs that do not link -lpthread will be unaffected due to the existing __pthread_mutex() helpers. Applications that explicitly specify locking callbacks via the deprecated avc_init() will also be unaffected. This is the first step toward a thread-safe selinux_check_access(); the class-string cache and status-page transition handling are addressed in follow-up changes. Fixes: SELinuxProject#287 SELinuxProject#335 SELinuxProject#336 Link: https://lore.kernel.org/selinux/CAJsHiNx1E7x1jaBkS0i4L1nBWXp8YXLHRWcCaDaH4LOn=zm+Zw@mail.gmail.com/ Link: https://lore.kernel.org/selinux/20220120073329.15234-1-purushottamchoudhary29@gmail.com/ Reported-by: Seth Moore <sethmo@google.com> Reported-by: Purushottam Choudhary <purushottamchoudhary29@gmail.com> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

The discover_class_cache list is accessed without locking by the string_to_*() and *_to_string() functions in libselinux for translating between string names and class/perm values, and returns pointers to strings within this list to callers. selinux_flush_class_cache() frees the list upon a policy reload, called by avc_process_policyload() from selinux_status_updated() and avc_netlink_process(). Make this safe for multi-threaded users by introducing and taking a mutex around the list accesses, and by moving flushed nodes to a retired list rather than freeing them so that returned string pointers remain valid. Given the infrequency of policy reloads, the relative stability of the class/perm mapping even across policy reloads, and the small amount of memory required, this seems a worthwhile tradeoff. If it becomes an issue, there are several options: 1. Only load/refresh new/modified class/permission names and values upon policy reload rather than flushing and reloading them all, 2. Provide a way to drain the retired list safely when the application knows it is no longer using any returned strings. Fixes: SELinuxProject#287 SELinuxProject#335 SELinuxProject#336 Link: https://lore.kernel.org/selinux/CAJsHiNx1E7x1jaBkS0i4L1nBWXp8YXLHRWcCaDaH4LOn=zm+Zw@mail.gmail.com/ Link: https://lore.kernel.org/selinux/20220120073329.15234-1-purushottamchoudhary29@gmail.com/ Reported-by: Seth Moore <sethmo@google.com> Reported-by: Purushottam Choudhary <purushottamchoudhary29@gmail.com> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

When selinux_check_access() is used by a multi-threaded application and calls selinux_status_updated(), every thread that observes the same sequence number bump races to run the avc_process_*() functions, so avc_ss_reset() and selinux_flush_class_cache() fire once per thread instead of once per event, and writes to the last_seqno/policyload can tear. Introduce and take a status_lock mutex across the last_seqno compare-and-update and the update actions so that exactly one thread services each event. The kernel status page itself is a seqlock and is still read unlocked via the existing read_sequence() loop. Lock ordering: 1. status_lock is never taken when another libselinux lock is held. 2. Under status_lock, the code may take avc_lock (via avc_ss_reset) or discover_class_lock (via selinux_flush_class_cache), but neither of these ever nest back into status_lock. Fixes: SELinuxProject#287 SELinuxProject#335 SELinuxProject#336 Link: https://lore.kernel.org/selinux/CAJsHiNx1E7x1jaBkS0i4L1nBWXp8YXLHRWcCaDaH4LOn=zm+Zw@mail.gmail.com/ Link: https://lore.kernel.org/selinux/20220120073329.15234-1-purushottamchoudhary29@gmail.com/ Reported-by: Seth Moore <sethmo@google.com> Reported-by: Purushottam Choudhary <purushottamchoudhary29@gmail.com> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

stephensmalley · 2026-06-12T19:22:01Z

See https://lore.kernel.org/selinux/20260612191146.20122-1-stephen.smalley.work@gmail.com/T/#t

…s set The AVC has a set of locking calls but defaults to no locking, and only the deprecated avc_init() allowed applications to set their own locking callbacks. Since selinux_check_access() is now commonly used by applications implementing userspace SELinux permission checks and it uses avc_open() internally, such calls are currently not thread-safe. Provide fallbacks to using pthread mutexes by default. Single-threaded programs that do not link -lpthread will be unaffected due to the existing __pthread_mutex() helpers. Applications that explicitly specify locking callbacks via the deprecated avc_init() will also be unaffected. This is the first step toward a thread-safe selinux_check_access(); the class-string cache and status-page transition handling are addressed in follow-up changes. Fixes: SELinuxProject#287 SELinuxProject#335 SELinuxProject#336 Link: https://lore.kernel.org/selinux/CAJsHiNx1E7x1jaBkS0i4L1nBWXp8YXLHRWcCaDaH4LOn=zm+Zw@mail.gmail.com/ Link: https://lore.kernel.org/selinux/20220120073329.15234-1-purushottamchoudhary29@gmail.com/ Reported-by: Seth Moore <sethmo@google.com> Reported-by: Purushottam Choudhary <purushottamchoudhary29@gmail.com> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

The discover_class_cache list is accessed without locking by the string_to_*() and *_to_string() functions in libselinux for translating between string names and class/perm values, and returns pointers to strings within this list to callers. selinux_flush_class_cache() frees the list upon a policy reload, called by avc_process_policyload() from selinux_status_updated() and avc_netlink_process(). Make this safe for multi-threaded users by introducing and taking a mutex around the list accesses, and by moving flushed nodes to a retired list rather than freeing them so that returned string pointers remain valid. Given the infrequency of policy reloads, the relative stability of the class/perm mapping even across policy reloads, and the small amount of memory required, this seems a worthwhile tradeoff. If it becomes an issue, there are several options: 1. Only load/refresh new/modified class/permission names and values upon policy reload rather than flushing and reloading them all, 2. Provide a way to drain the retired list safely when the application knows it is no longer using any returned strings. Fixes: SELinuxProject#287 SELinuxProject#335 SELinuxProject#336 Link: https://lore.kernel.org/selinux/CAJsHiNx1E7x1jaBkS0i4L1nBWXp8YXLHRWcCaDaH4LOn=zm+Zw@mail.gmail.com/ Link: https://lore.kernel.org/selinux/20220120073329.15234-1-purushottamchoudhary29@gmail.com/ Reported-by: Seth Moore <sethmo@google.com> Reported-by: Purushottam Choudhary <purushottamchoudhary29@gmail.com> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

When selinux_check_access() is used by a multi-threaded application and calls selinux_status_updated(), every thread that observes the same sequence number bump races to run the avc_process_*() functions, so avc_ss_reset() and selinux_flush_class_cache() fire once per thread instead of once per event, and writes to the last_seqno/policyload can tear. Introduce and take a status_lock mutex across the last_seqno compare-and-update and the update actions so that exactly one thread services each event. The kernel status page itself is a seqlock and is still read unlocked via the existing read_sequence() loop. Lock ordering: 1. status_lock is never taken when another libselinux lock is held. 2. Under status_lock, the code may take avc_lock (via avc_ss_reset) or discover_class_lock (via selinux_flush_class_cache), but neither of these ever nest back into status_lock. Fixes: SELinuxProject#287 SELinuxProject#335 SELinuxProject#336 Link: https://lore.kernel.org/selinux/CAJsHiNx1E7x1jaBkS0i4L1nBWXp8YXLHRWcCaDaH4LOn=zm+Zw@mail.gmail.com/ Link: https://lore.kernel.org/selinux/20220120073329.15234-1-purushottamchoudhary29@gmail.com/ Reported-by: Seth Moore <sethmo@google.com> Reported-by: Purushottam Choudhary <purushottamchoudhary29@gmail.com> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

purushottamc mentioned this pull request Dec 17, 2021

discover_class_cache is not threadsafe #335

Open

bachradsusi closed this May 29, 2026

stephensmalley reopened this Jun 12, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

libselinux: make threadsafe for discover_class_cache#336

libselinux: make threadsafe for discover_class_cache#336
purushottamc wants to merge 1 commit into
SELinuxProject:masterfrom
purushottamc:threadsafe_discover_class_cache

purushottamc commented Dec 17, 2021

Uh oh!

cgzones commented Dec 17, 2021

Uh oh!

purushottamc commented Jan 25, 2022

Uh oh!

cgzones commented Jan 25, 2022

Uh oh!

stephensmalley commented Jun 12, 2026

Uh oh!

stephensmalley commented Jun 12, 2026

Uh oh!

stephensmalley commented Jun 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

purushottamc commented Dec 17, 2021

Uh oh!

cgzones commented Dec 17, 2021

Uh oh!

purushottamc commented Jan 25, 2022

Uh oh!

cgzones commented Jan 25, 2022

Uh oh!

stephensmalley commented Jun 12, 2026

Uh oh!

stephensmalley commented Jun 12, 2026

Uh oh!

stephensmalley commented Jun 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants