kernel: make sepolicy manipulation follows rcu#37
Conversation
5ec1cff
force-pushed
the
main
branch
2 times, most recently
from
f203c12 to
67e782c
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates KernelSU’s SELinux policy manipulation to follow an RCU-style “duplicate → swap pointer → wait grace period → free old” model, aiming to avoid in-place mutation of the active policy and make policy updates safer under concurrent readers.
Changes:
- Adds
ksu_dup_sepolicy()/ksu_destroy_sepolicy()helpers to duplicate and partially free astruct selinux_policy. - Updates rule application and sepolicy handling paths to lock
selinux_state.policy_mutex, swapselinux_state.policyviarcu_assign_pointer(), andsynchronize_rcu()before freeing the old policy. - Keeps AVC cache reset after policy modifications so updated rules take effect immediately.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
kernel/selinux/sepolicy.h |
Exposes new policy duplication/destruction APIs. |
kernel/selinux/sepolicy.c |
Implements partial policy duplication and teardown helpers. |
kernel/selinux/rules.c |
Switches policy update paths to mutex + RCU pointer swap + old-policy destruction, and centralizes AVC reset. |
Comments suppressed due to low confidence (1)
kernel/selinux/rules.c:198
copy_from_user(&data, ...)failure returns immediately whileselinux_state.policy_mutexis held and after a new policy may have been allocated. This can deadlock future callers and leakpol. Setret, free/destroypolif allocated, andgoto out_unlockinstead of returning here.
struct sepol_data data;
if (copy_from_user(&data, arg4, sizeof(struct sepol_data))) {
pr_err("sepol: copy sepol_data failed.\n");
return -EINVAL;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| struct selinux_policy *pol, *old_pol = selinux_state.policy; | ||
| struct policydb *db; |
There was a problem hiding this comment.
old_pol is read from selinux_state.policy before policy_mutex is taken. If the policy pointer can change concurrently (e.g., policy reload), this cached pointer can become stale before the later rcu_dereference_protected(old_pol, ...) and ksu_destroy_sepolicy(old_pol) calls. Consider reading selinux_state.policy only after acquiring selinux_state.policy_mutex (via rcu_dereference_protected(selinux_state.policy, ...)) and keep old_pol scoped to the locked section.
|
|
||
| out: | ||
| kfree(new_pol); | ||
| return NULL; |
There was a problem hiding this comment.
ksu_dup_sepolicy error path (out:) only kfree(new_pol). If any copy step fails after earlier ones succeeded, those earlier allocations are leaked. Consider unwinding via ksu_destroy_sepolicy(new_pol) (or a stepwise cleanup) before returning NULL.
| static void free_filename_trans(struct policydb *db) | ||
| { | ||
| hashtab_destroy(&db->filename_trans); | ||
| } |
There was a problem hiding this comment.
free_filename_trans() only calls hashtab_destroy(&db->filename_trans), but add_filename_trans() allocates filename_trans_key/filename_trans_datum (plus kstrdup name) and inserts them into this table. Without a hashtab_map() + free callback (or equivalent), these per-rule allocations will leak on each policy replacement.
| kvfree(db->sym_val_to_name[SYM_TYPES]); | ||
| } | ||
|
|
||
| hashtab_destroy(&db->p_types.table); | ||
| } |
There was a problem hiding this comment.
free_type_datum_partially() destroys db->p_types.table via hashtab_destroy() but never frees per-entry key/datum. Since KSU can allocate new types/attrs at runtime (see add_type() using kzalloc + kstrdup + symtab_insert()), those allocations will leak when a duplicated policy is destroyed. Consider iterating the table with a free callback for entries owned by this policy (or avoid allocating new type entries in a policy that uses shallow-copied symtabs).
| rcu_assign_pointer(selinux_state.policy, pol); | ||
| synchronize_rcu(); | ||
| ksu_destroy_sepolicy(old_pol); | ||
|
|
There was a problem hiding this comment.
apply_kernelsu_rules() replaces selinux_state.policy and then calls ksu_destroy_sepolicy(old_pol). ksu_destroy_sepolicy() is a partial free (it doesn’t release many policydb-owned allocations / symtabs), so if old_pol is the original SELinux-loaded policy, this will leak a large amount of memory. Consider only calling ksu_destroy_sepolicy() for policies created by ksu_dup_sepolicy(), and use the proper SELinux policy free path (or keep the original policy alive) for the initial policy object.
No description provided.