| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
There are a few bugs in the error handling for security_load_policy().
1) If the newpolicy->sidtab allocation fails then it leads to a NULL
dereference. Also the error code was not set to -ENOMEM on that
path.
2) If policydb_read() failed then we call policydb_destroy() twice
which meands we call kvfree(p->sym_val_to_name[i]) twice.
3) If policydb_load_isids() failed then we call sidtab_destroy() twice
and that results in a double free in the sidtab_destroy_tree()
function because entry.ptr_inner and entry.ptr_leaf are not set to
NULL.
One thing that makes this code nice to deal with is that none of the
functions return partially allocated data. In other words, the
policydb_read() either allocates everything successfully or it frees
all the data it allocates. It never returns a mix of allocated and
not allocated data.
I re-wrote this to only free the successfully allocated data which
avoids the double frees. I also re-ordered selinux_policy_free() so
it's in the reverse order of the allocation function.
Fixes: c7c556f1e81b ("selinux: refactor changing booleans")
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
[PM: partially merged by hand due to merge fuzz]
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Convert the policy read-write lock to RCU. This is significantly
simplified by the earlier work to encapsulate the policy data
structures and refactor the policy load and boolean setting logic.
Move the latest_granting sequence number into the selinux_policy
structure so that it can be updated atomically with the policy.
Since removing the policy rwlock and moving latest_granting reduces
the selinux_ss structure to nothing more than a wrapper around the
selinux_policy pointer, get rid of the extra layer of indirection.
At present this change merely passes a hardcoded 1 to
rcu_dereference_check() in the cases where we know we do not need to
take rcu_read_lock(), with the preceding comment explaining why.
Alternatively we could pass fsi->mutex down from selinuxfs and
apply a lockdep check on it instead.
Based in part on earlier attempts to convert the policy rwlock
to RCU by Kaigai Kohei [1] and by Peter Enderborg [2].
[1] https://lore.kernel.org/selinux/6e2f9128-e191-ebb3-0e87-74bfccb0767f@tycho.nsa.gov/
[2] https://lore.kernel.org/selinux/20180530141104.28569-1-peter.enderborg@sony.com/
Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Replace the existing /* fall through */ comments and its variants with
the new pseudo-keyword macro fallthrough[1]. Also, remove unnecessary
fall-through markings when it is the case.
[1] https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
|
|
Use kmemdup rather than duplicating its implementation
Generated by: scripts/coccinelle/api/memdup.cocci
Fixes: c7c556f1e81b ("selinux: refactor changing booleans")
CC: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: kernel test robot <lkp@intel.com>
Signed-off-by: Julia Lawall <julia.lawall@inria.fr>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Certain SELinux security server functions (e.g. security_port_sid,
called during bind) were not explicitly testing to see if SELinux
has been initialized (i.e. initial policy loaded) and handling
the no-policy-loaded case. In the past this happened to work
because the policydb was statically allocated and could always
be accessed, but with the recent encapsulation of policy state
and conversion to dynamic allocation, we can no longer access
the policy state prior to initialization. Add a test of
!selinux_initialized(state) to all of the exported functions that
were missing them and handle appropriately.
Fixes: 461698026ffa ("selinux: encapsulate policy state, refactor policy load")
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Tested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
The allocation check of newpolicy->sidtab is null checking if
newpolicy is null and not newpolicy->sidtab. Fix this.
Addresses-Coverity: ("Logically dead code")
Fixes: c7c556f1e81b ("selinux: refactor changing booleans")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Refactor the logic for changing SELinux policy booleans in a similar
manner to the refactoring of policy load, thereby reducing the
size of the critical section when the policy write-lock is held
and making it easier to convert the policy rwlock to RCU in the
future. Instead of directly modifying the policydb in place, modify
a copy and then swap it into place through a single pointer update.
Only fully copy the portions of the policydb that are affected by
boolean changes to avoid the full cost of a deep policydb copy.
Introduce another level of indirection for the sidtab since changing
booleans does not require updating the sidtab, unlike policy load.
While we are here, create a common helper for notifying
other kernel components and userspace of a policy change and call it
from both security_set_bools() and selinux_policy_commit().
Based on an old (2004) patch by Kaigai Kohei [1] to convert the policy
rwlock to RCU that was deferred at the time since it did not
significantly improve performance and introduced complexity. Peter
Enderborg later submitted a patch series to convert to RCU [2] that
would have made changing booleans a much more expensive operation
by requiring a full policydb_write();policydb_read(); sequence to
deep copy the entire policydb and also had concerns regarding
atomic allocations.
This change is now simplified by the earlier work to encapsulate
policy state in the selinux_policy struct and to refactor
policy load. After this change, the last major obstacle to
converting the policy rwlock to RCU is likely the sidtab live
convert support.
[1] https://lore.kernel.org/selinux/6e2f9128-e191-ebb3-0e87-74bfccb0767f@tycho.nsa.gov/
[2] https://lore.kernel.org/selinux/20180530141104.28569-1-peter.enderborg@sony.com/
Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
With the refactoring of the policy load logic in the security
server from the previous change, it is now possible to split out
the committing of the new policy from security_load_policy() and
perform it only after successful updating of selinuxfs. Change
security_load_policy() to return the newly populated policy
data structures to the caller, export selinux_policy_commit()
for external callers, and introduce selinux_policy_cancel() to
provide a way to cancel the policy load in the event of an error
during updating of the selinuxfs directory tree. Further, rework
the interfaces used by selinuxfs to get information from the policy
when creating the new directory tree to take and act upon the
new policy data structure rather than the current/active policy.
Update selinuxfs to use these updated and new interfaces. While
we are here, stop re-creating the policy_capabilities directory
on each policy load since it does not depend on the policy, and
stop trying to create the booleans and classes directories during
the initial creation of selinuxfs since no information is available
until first policy load.
After this change, a failure while updating the booleans and class
directories will cause the entire policy load to be canceled, leaving
the original policy intact, and policy load notifications to userspace
will only happen after a successful completion of updating those
directories. This does not (yet) provide full atomicity with respect
to the updating of the directory trees themselves.
Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Encapsulate the policy state in its own structure (struct
selinux_policy) that is separately allocated but referenced from the
selinux_ss structure. The policy state includes the SID table
(particularly the context structures), the policy database, and the
mapping between the kernel classes/permissions and the policy values.
Refactor the security server portion of the policy load logic to
cleanly separate loading of the new structures from committing the new
policy. Unify the initial policy load and reload code paths as much
as possible, avoiding duplicated code. Make sure we are taking the
policy read-lock prior to any dereferencing of the policy. Move the
copying of the policy capability booleans into the state structure
outside of the policy write-lock because they are separate from the
policy and are read outside of any policy lock; possibly they should
be using at least READ_ONCE/WRITE_ONCE or smp_load_acquire/store_release.
These changes simplify the policy loading logic, reduce the size of
the critical section while holding the policy write-lock, and should
facilitate future changes to e.g. refactor the entire policy reload
logic including the selinuxfs code to make the updating of the policy
and the selinuxfs directory tree atomic and/or to convert the policy
read-write lock to RCU.
Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Presently mdp does not enable any SELinux policy capabilities
in the dummy policy it generates. Thus, policies derived from
it will by default lack various features commonly used in modern
policies such as open permission, extended socket classes, network
peer controls, etc. Split the policy capability definitions out into
their own headers so that we can include them into mdp without pulling in
other kernel headers and extend mdp generate policycap statements for the
policy capabilities known to the kernel. Policy authors may wish to
selectively remove some of these from the generated policy.
Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Pull selinux updates from Paul Moore:
"Beyond the usual smattering of bug fixes, we've got three small
improvements worth highlighting:
- improved SELinux policy symbol table performance due to a reworking
of the insert and search functions
- allow reading of SELinux labels before the policy is loaded,
allowing for some more "exotic" initramfs approaches
- improved checking an error reporting about process
class/permissions during SELinux policy load"
* tag 'selinux-pr-20200803' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux:
selinux: complete the inlining of hashtab functions
selinux: prepare for inlining of hashtab functions
selinux: specialize symtab insert and search functions
selinux: Fix spelling mistakes in the comments
selinux: fixed a checkpatch warning with the sizeof macro
selinux: log error messages on required process class / permissions
scripts/selinux/mdp: fix initial SID handling
selinux: allow reading labels before policy is loaded
|
|
Move (most of) the definitions of hashtab_search() and hashtab_insert()
to the header file. In combination with the previous patch, this avoids
calling the callbacks indirectly by function pointers and allows for
better optimization, leading to a drastic performance improvement of
these operations.
With this patch, I measured a speed up in the following areas (measured
on x86_64 F32 VM with 4 CPUs):
1. Policy load (`load_policy`) - takes ~150 ms instead of ~230 ms.
2. `chcon -R unconfined_u:object_r:user_tmp_t:s0:c381,c519 /tmp/linux-src`
where /tmp/linux-src is an extracted linux-5.7 source tarball -
takes ~522 ms instead of ~576 ms. This is because of many
symtab_search() calls in string_to_context_struct() when there are
many categories specified in the context.
3. `stress-ng --msg 1 --msg-ops 10000000` - takes 12.41 s instead of
13.95 s (consumes 18.6 s of kernel CPU time instead of 21.6 s).
This is thanks to security_transition_sid() being ~43% faster after
this patch.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Refactor searching and inserting into hashtabs to pave the way for
converting hashtab_search() and hashtab_insert() to inline functions in
the next patch. This will avoid indirect calls and allow the compiler to
better optimize individual callers, leading to a significant performance
improvement.
In order to avoid the indirect calls, the key hashing and comparison
callbacks need to be extracted from the hashtab struct and passed
directly to hashtab_search()/_insert() by the callers so that the
callback address is always known at compile time. The kernel's
rhashtable library (<linux/rhashtable*.h>) does the same thing.
This of course makes the hashtab functions slightly easier to misuse by
passing a wrong callback set, but unfortunately there is no better way
to implement a hash table that is both generic and efficient in C. This
patch tries to somewhat mitigate this by only calling the hashtab
functions in the same file where the corresponding callbacks are
defined (wrapping them into more specialized functions as needed).
Note that this patch doesn't bring any benefit without also moving the
definitions of hashtab_search() and -_insert() to the header file, which
is done in a follow-up patch for easier review of the hashtab.c changes
in this patch.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
This encapsulates symtab a little better and will help with further
refactoring later.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
`sizeof buf` changed to `sizeof(buf)`
Signed-off-by: Ethan Edwards <ethancarteredwards@gmail.com>
[PM: rewrote the subject line]
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
In general SELinux no longer treats undefined object classes or permissions
in the policy as a fatal error, instead handling them in accordance with
handle_unknown. However, the process class and process transition and
dyntransition permissions are still required to be defined due to
dependencies on these definitions for default labeling behaviors,
role and range transitions in older policy versions that lack an explicit
class field, and role allow checking. Log error messages in these cases
since otherwise the policy load will fail silently with no indication
to the user as to the underlying cause. While here, fix the checking for
process transition / dyntransition so that omitting either permission is
handled as an error; both are needed in order to ensure that role allow
checking is consistently applied.
Reported-by: bauen1 <j2468h@googlemail.com>
Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Pull SELinux fixes from Paul Moore:
"Three small patches to fix problems in the SELinux code, all found via
clang.
Two patches fix potential double-free conditions and one fixes an
undefined return value"
* tag 'selinux-pr-20200621' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux:
selinux: fix undefined return of cond_evaluate_expr
selinux: fix a double free in cond_read_node()/cond_read_list()
selinux: fix double free
|
|
clang static analysis reports an undefined return
security/selinux/ss/conditional.c:79:2: warning: Undefined or garbage value returned to caller [core.uninitialized.UndefReturn]
return s[0];
^~~~~~~~~~~
static int cond_evaluate_expr( ...
{
u32 i;
int s[COND_EXPR_MAXDEPTH];
for (i = 0; i < expr->len; i++)
...
return s[0];
When expr->len is 0, the loop which sets s[0] never runs.
So return -1 if the loop never runs.
Cc: stable@vger.kernel.org
Signed-off-by: Tom Rix <trix@redhat.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Clang static analysis reports this double free error
security/selinux/ss/conditional.c:139:2: warning: Attempt to free released memory [unix.Malloc]
kfree(node->expr.nodes);
^~~~~~~~~~~~~~~~~~~~~~~
When cond_read_node fails, it calls cond_node_destroy which frees the
node but does not poison the entry in the node list. So when it
returns to its caller cond_read_list, cond_read_list deletes the
partial list. The latest entry in the list will be deleted twice.
So instead of freeing the node in cond_read_node, let list freeing in
code_read_list handle the freeing the problem node along with all of the
earlier nodes.
Because cond_read_node no longer does any error handling, the goto's
the error case are redundant. Instead just return the error code.
Cc: stable@vger.kernel.org
Fixes: 60abd3181db2 ("selinux: convert cond_list to array")
Signed-off-by: Tom Rix <trix@redhat.com>
Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
[PM: subject line tweaks]
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Clang's static analysis tool reports these double free memory errors.
security/selinux/ss/services.c:2987:4: warning: Attempt to free released memory [unix.Malloc]
kfree(bnames[i]);
^~~~~~~~~~~~~~~~
security/selinux/ss/services.c:2990:2: warning: Attempt to free released memory [unix.Malloc]
kfree(bvalues);
^~~~~~~~~~~~~~
So improve the security_get_bools error handling by freeing these variables
and setting their return pointers to NULL and the return len to 0
Cc: stable@vger.kernel.org
Signed-off-by: Tom Rix <trix@redhat.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Pull SELinux updates from Paul Moore:
"The highlights:
- A number of improvements to various SELinux internal data
structures to help improve performance. We move the role
transitions into a hash table. In the content structure we shift
from hashing the content string (aka SELinux label) to the
structure itself, when it is valid. This last change not only
offers a speedup, but it helps us simplify the code some as well.
- Add a new SELinux policy version which allows for a more space
efficient way of storing the filename transitions in the binary
policy. Given the default Fedora SELinux policy with the unconfined
module enabled, this change drops the policy size from ~7.6MB to
~3.3MB. The kernel policy load time dropped as well.
- Some fixes to the error handling code in the policy parser to
properly return error codes when things go wrong"
* tag 'selinux-pr-20200601' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux:
selinux: netlabel: Remove unused inline function
selinux: do not allocate hashtabs dynamically
selinux: fix return value on error in policydb_read()
selinux: simplify range_write()
selinux: fix error return code in policydb_read()
selinux: don't produce incorrect filename_trans_count
selinux: implement new format of filename transitions
selinux: move context hashing under sidtab
selinux: hash context structure directly
selinux: store role transitions in a hash table
selinux: drop unnecessary smp_load_acquire() call
selinux: fix warning Comparison to bool
|
|
It is simpler to allocate them statically in the corresponding
structure, avoiding unnecessary kmalloc() calls and pointer
dereferencing.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
[PM: manual merging required in policydb.c]
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
The value of rc is still zero from the last assignment when the error
path is taken. Fix it by setting it to -ENOMEM before the
hashtab_create() call.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: e67b2ec9f617 ("selinux: store role transitions in a hash table")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
No need to traverse the hashtab to count its elements, hashtab already
tracks it for us.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Fix to return negative error code -ENOMEM from the kvcalloc() error
handling case instead of 0, as done elsewhere in this function.
Fixes: acdf52d97f82 ("selinux: convert to kvmalloc")
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Pull SELinux fixes from Paul Moore:
"Two more SELinux patches to fix problems in the v5.7-rcX releases.
Wei Yongjun's patch fixes a return code in an error path, and my patch
fixes a problem where we were not correctly applying access controls
to all of the netlink messages in the netlink_send LSM hook"
* tag 'selinux-pr-20200430' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux:
selinux: properly handle multiple messages in selinux_netlink_send()
selinux: fix error return code in cond_read_list()
|
|
Fix to return negative error code -ENOMEM from the error handling
case instead of 0, as done elsewhere in this function.
Fixes: 60abd3181db2 ("selinux: convert cond_list to array")
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
I thought I fixed the counting in filename_trans_read_helper() to count
the compat rule count correctly in the final version, but it's still
wrong. To really count the same thing as in the compat path, we'd need
to add up the cardinalities of stype bitmaps of all datums.
Since the kernel currently doesn't implement an ebitmap_cardinality()
function (and computing the proper count would just waste CPU cycles
anyway), just document that we use the field only in case of the old
format and stop updating it in filename_trans_read_helper().
Fixes: 430059024389 ("selinux: implement new format of filename transitions")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Implement a new, more space-efficient way of storing filename
transitions in the binary policy. The internal structures have already
been converted to this new representation; this patch just implements
reading/writing an equivalent represntation from/to the binary policy.
This new format reduces the size of Fedora policy from 7.6 MB to only
3.3 MB (with policy optimization enabled in both cases). With the
unconfined module disabled, the size is reduced from 3.3 MB to 2.4 MB.
The time to load policy into kernel is also shorter with the new format.
On Fedora Rawhide x86_64 it dropped from 157 ms to 106 ms; without the
unconfined module from 115 ms to 105 ms.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Now that context hash computation no longer depends on policydb, we can
simplify things by moving the context hashing completely under sidtab.
The hash is still cached in sidtab entries, but not for the in-flight
context structures.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Always hashing the string representation is inefficient. Just hash the
contents of the structure directly (using jhash). If the context is
invalid (str & len are set), then hash the string as before, otherwise
hash the structured data.
Since the context hashing function is now faster (about 10 times), this
patch decreases the overhead of security_transition_sid(), which is
called from many hooks.
The jhash function seemed as a good choice, since it is used as the
default hashing algorithm in rhashtable.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Jeff Vander Stoep <jeffv@google.com>
Tested-by: Jeff Vander Stoep <jeffv@google.com>
[PM: fixed some spelling errors in the comments pointed out by JVS]
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Currently, they are stored in a linked list, which adds significant
overhead to security_transition_sid(). On Fedora, with 428 role
transitions in policy, converting this list to a hash table cuts down
its run time by about 50%. This was measured by running 'stress-ng --msg
1 --msg-ops 100000' under perf with and without this patch.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Pull SELinux fix from Paul Moore:
"One small SELinux fix to ensure we cleanup properly on an error
condition"
* tag 'selinux-pr-20200416' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux:
selinux: free str on error in str_read()
|
|
In commit 66f8e2f03c02 ("selinux: sidtab reverse lookup hash table") the
corresponding load is moved under the spin lock, so there is no race
possible and we can read the count directly. The smp_store_release() is
still needed to avoid racing with the lock-free readers.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
In [see "Fixes:"] I missed the fact that str_read() may give back an
allocated pointer even if it returns an error, causing a potential
memory leak in filename_trans_read_one(). Fix this by making the
function free the allocated string whenever it returns a non-zero value,
which also makes its behavior more obvious and prevents repeating the
same mistake in the future.
Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1461665 ("Resource leaks")
Fixes: c3a276111ea2 ("selinux: optimize storage of filename transitions")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
fix below warnings reported by coccicheck
security/selinux/ss/mls.c:539:39-43: WARNING: Comparison to bool
security/selinux/ss/services.c:1815:46-50: WARNING: Comparison to bool
security/selinux/ss/services.c:1827:46-50: WARNING: Comparison to bool
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Zou Wei <zou_wei@huawei.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Pull SELinux updates from Paul Moore:
"We've got twenty SELinux patches for the v5.7 merge window, the
highlights are below:
- Deprecate setting /sys/fs/selinux/checkreqprot to 1.
This flag was originally created to deal with legacy userspace and
the READ_IMPLIES_EXEC personality flag. We changed the default from
1 to 0 back in Linux v4.4 and now we are taking the next step of
deprecating it, at some point in the future we will take the final
step of rejecting 1.
- Allow kernfs symlinks to inherit the SELinux label of the parent
directory. In order to preserve backwards compatibility this is
protected by the genfs_seclabel_symlinks SELinux policy capability.
- Optimize how we store filename transitions in the kernel, resulting
in some significant improvements to policy load times.
- Do a better job calculating our internal hash table sizes which
resulted in additional policy load improvements and likely general
SELinux performance improvements as well.
- Remove the unused initial SIDs (labels) and improve how we handle
initial SIDs.
- Enable per-file labeling for the bpf filesystem.
- Ensure that we properly label NFS v4.2 filesystems to avoid a
temporary unlabeled condition.
- Add some missing XFS quota command types to the SELinux quota
access controls.
- Fix a problem where we were not updating the seq_file position
index correctly in selinuxfs.
- We consolidate some duplicated code into helper functions.
- A number of list to array conversions.
- Update Stephen Smalley's email address in MAINTAINERS"
* tag 'selinux-pr-20200330' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux:
selinux: clean up indentation issue with assignment statement
NFS: Ensure security label is set for root inode
MAINTAINERS: Update my email address
selinux: avtab_init() and cond_policydb_init() return void
selinux: clean up error path in policydb_init()
selinux: remove unused initial SIDs and improve handling
selinux: reduce the use of hard-coded hash sizes
selinux: Add xfs quota command types
selinux: optimize storage of filename transitions
selinux: factor out loop body from filename_trans_read()
security: selinux: allow per-file labeling for bpffs
selinux: generalize evaluate_cond_node()
selinux: convert cond_expr to array
selinux: convert cond_av_list to array
selinux: convert cond_list to array
selinux: sel_avc_get_stat_idx should increase position index
selinux: allow kernfs symlinks to inherit parent directory context
selinux: simplify evaluate_cond_node()
Documentation,selinux: deprecate setting checkreqprot to 1
selinux: move status variables out of selinux_ss
|
|
The assignment of e->type_names is indented one level too deep,
clean this up by removing the extraneous tab.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
The avtab_init() and cond_policydb_init() functions always return
zero so mark them as returning void and update the callers not to
check for a return value.
Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Commit e0ac568de1fa ("selinux: reduce the use of hard-coded hash sizes")
moved symtab initialization out of policydb_init(), but left the cleanup
of symtabs from the error path. This patch fixes the oversight.
Suggested-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Remove initial SIDs that have never been used or are no longer used by
the kernel from its string table, which is also used to generate the
SECINITSID_* symbols referenced in code. Update the code to
gracefully handle the fact that these can now be NULL. Stop treating
it as an error if a policy defines additional initial SIDs unknown to
the kernel. Do not load unused initial SID contexts into the sidtab.
Fix the incorrect usage of the name from the ocontext in error
messages when loading initial SIDs since these are not presently
written to the kernel policy and are therefore always NULL.
After this change, it is possible to safely reclaim and reuse some of
the unused initial SIDs without compatibility issues. Specifically,
unused initial SIDs that were being assigned the same context as the
unlabeled initial SID in policies can be reclaimed and reused for
another purpose, with existing policies still treating them as having
the unlabeled context and future policies having the option of mapping
them to a more specific context. For example, this could have been
used when the infiniband labeling support was introduced to define
initial SIDs for the default pkey and endport SIDs similar to the
handling of port/netif/node SIDs rather than always using
SECINITSID_UNLABELED as the default.
The set of safely reclaimable unused initial SIDs across all known
policies is igmp_packet (13), icmp_socket (14), tcp_socket (15), kmod
(24), policy (25), and scmp_packet (26); these initial SIDs were
assigned the same context as unlabeled in all known policies including
mls. If only considering non-mls policies (i.e. assuming that mls
users always upgrade policy with their kernels), the set of safely
reclaimable unused initial SIDs further includes file_labels (6), init
(7), sysctl_modprobe (16), and sysctl_fs (18) through sysctl_dev (23).
Adding new initial SIDs beyond SECINITSID_NUM to policy unfortunately
became a fatal error in commit 24ed7fdae669 ("selinux: use separate
table for initial SID lookup") and even before that it could cause
problems on a policy reload (collision between the new initial SID and
one allocated at runtime) ever since commit 42596eafdd75 ("selinux:
load the initial SIDs upon every policy load") so we cannot safely
start adding new initial SIDs to policies beyond SECINITSID_NUM (27)
until such a time as all such kernels do not need to be supported and
only those that include this commit are relevant. That is not a big
deal since we haven't added a new initial SID since 2004 (v2.6.7) and
we have plenty of unused ones we can reclaim if we truly need one.
If we want to avoid the wasted storage in initial_sid_to_string[]
and/or sidtab->isids[] for the unused initial SIDs, we could introduce
an indirection between the kernel initial SID values and the policy
initial SID values and just map the policy SID values in the ocontexts
to the kernel values during policy_load_isids(). Originally I thought
we'd do this by preserving the initial SID names in the kernel policy
and creating a mapping at load time like we do for the security
classes and permissions but that would require a new kernel policy
format version and associated changes to libsepol/checkpolicy and I'm
not sure it is justified. Simpler approach is just to create a fixed
mapping table in the kernel from the existing fixed policy values to
the kernel values. Less flexible but probably sufficient.
A separate selinux userspace change was applied in
https://github.com/SELinuxProject/selinux/commit/8677ce5e8f592950ae6f14cea1b68a20ddc1ac25
to enable removal of most of the unused initial SID contexts from
policies, but there is no dependency between that change and this one.
That change permits removing all of the unused initial SID contexts
from policy except for the fs and sysctl SID contexts. The initial
SID declarations themselves would remain in policy to preserve the
values of subsequent ones but the contexts can be dropped. If/when
the kernel decides to reuse one of them, future policies can change
the name and start assigning a context again without breaking
compatibility.
Here is how I would envision staging changes to the initial SIDs in a
compatible manner after this commit is applied:
1. At any time after this commit is applied, the kernel could choose
to reclaim one of the safely reclaimable unused initial SIDs listed
above for a new purpose (i.e. replace its NULL entry in the
initial_sid_to_string[] table with a new name and start using the
newly generated SECINITSID_name symbol in code), and refpolicy could
at that time rename its declaration of that initial SID to reflect its
new purpose and start assigning it a context going
forward. Existing/old policies would map the reclaimed initial SID to
the unlabeled context, so that would be the initial default behavior
until policies are updated. This doesn't depend on the selinux
userspace change; it will work with existing policies and userspace.
2. In 6 months or so we'll have another SELinux userspace release that
will include the libsepol/checkpolicy support for omitting unused
initial SID contexts.
3. At any time after that release, refpolicy can make that release its
minimum build requirement and drop the sid context statements (but not
the sid declarations) for all of the unused initial SIDs except for
fs and sysctl, which must remain for compatibility on policy
reload with old kernels and for compatibility with kernels that were
still using SECINITSID_SYSCTL (< 2.6.39). This doesn't depend on this
kernel commit; it will work with previous kernels as well.
4. After N years for some value of N, refpolicy decides that it no
longer cares about policy reload compatibility for kernels that
predate this kernel commit, and refpolicy drops the fs and sysctl
SID contexts from policy too (but retains the declarations).
5. After M years for some value of M, the kernel decides that it no
longer cares about compatibility with refpolicies that predate step 4
(dropping the fs and sysctl SIDs), and those two SIDs also become
safely reclaimable. This step is optional and need not ever occur unless
we decide that the need to reclaim those two SIDs outweighs the
compatibility cost.
6. After O years for some value of O, refpolicy decides that it no
longer cares about policy load (not just reload) compatibility for
kernels that predate this kernel commit, and both kernel and refpolicy
can then start adding and using new initial SIDs beyond 27. This does
not depend on the previous change (step 5) and can occur independent
of it.
Fixes: https://github.com/SELinuxProject/selinux-kernel/issues/12
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Instead allocate hash tables with just the right size based on the
actual number of elements (which is almost always known beforehand, we
just need to defer the hashtab allocation to the right time). The only
case when we don't know the size (with the current policy format) is the
new filename transitions hashtable. Here I just left the existing value.
After this patch, the time to load Fedora policy on x86_64 decreases
from 790 ms to 167 ms. If the unconfined module is removed, it decreases
from 750 ms to 122 ms. It is also likely that other operations are going
to be faster, mainly string_to_context_struct() or mls_compute_sid(),
but I didn't try to quantify that.
The memory usage of all hash table arrays increases from ~58 KB to
~163 KB (with Fedora policy on x86_64).
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
In these rules, each rule with the same (target type, target class,
filename) values is (in practice) always mapped to the same result type.
Therefore, it is much more efficient to group the rules by (ttype,
tclass, filename).
Thus, this patch drops the stype field from the key and changes the
datum to be a linked list of one or more structures that contain a
result type and an ebitmap of source types that map the given target to
the given result type under the given filename. The size of the hash
table is also incremented to 2048 to be more optimal for Fedora policy
(which currently has ~2500 unique (ttype, tclass, filename) tuples,
regardless of whether the 'unconfined' module is enabled).
Not only does this dramtically reduce memory usage when the policy
contains a lot of unconfined domains (ergo a lot of filename based
transitions), but it also slightly reduces memory usage of strongly
confined policies (modeled on Fedora policy with 'unconfined' module
disabled) and significantly reduces lookup times of these rules on
Fedora (roughly matches the performance of the rhashtable conversion
patch [1] posted recently to selinux@vger.kernel.org).
An obvious next step is to change binary policy format to match this
layout, so that disk space is also saved. However, since that requires
more work (including matching userspace changes) and this patch is
already beneficial on its own, I'm posting it separately.
Performance/memory usage comparison:
Kernel | Policy load | Policy load | Mem usage | Mem usage | openbench
| | (-unconfined) | | (-unconfined) | (createfiles)
-----------------|-------------|---------------|-----------|---------------|--------------
reference | 1,30s | 0,91s | 90MB | 77MB | 55 us/file
rhashtable patch | 0.98s | 0,85s | 85MB | 75MB | 38 us/file
this patch | 0,95s | 0,87s | 75MB | 75MB | 40 us/file
(Memory usage is measured after boot. With SELinux disabled the memory
usage was ~60MB on the same system.)
[1] https://lore.kernel.org/selinux/20200116213937.77795-1-dev@lynxeye.de/T/
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
It simplifies cleanup in the error path. This will be extra useful in
later patch.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Both callers iterate the cond_list and call it for each node - turn it
into evaluate_cond_nodes(), which does the iteration for them.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Since it is fixed-size after allocation and we know the size beforehand,
using a plain old array is simpler and more efficient.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Since it is fixed-size after allocation and we know the size beforehand,
using a plain old array is simpler and more efficient.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Since it is fixed-size after allocation and we know the size beforehand,
using a plain old array is simpler and more efficient.
While there, also fix signedness of some related variables/parameters.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
Currently symlinks on kernel filesystems, like sysfs, are labeled on
creation with the parent filesystem root sid.
Allow symlinks to inherit the parent directory context, so fine-grained
kernfs labeling can be applied to symlinks too and checking contexts
doesn't complain about them.
For backward-compatibility this behavior is contained in a new policy
capability: genfs_seclabel_symlinks
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
|
It never fails, so it can just return void.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
|
Dan Carpenter
Stephen Smalley
Gustavo A. R. Silva
kernel test robot
Colin Ian King
Linus Torvalds
Ondrej Mosnacek
Ethan Edwards
Tom Rix
Wei Yongjun
Zou Wei
Paul Moore
Stephen Smalley
Christian Göttsche