22 Jul, 2020
1 commit
-
commit c27c6bd2c4d6b6bb779f9b722d5607993e1d5e5c upstream.
Currently it is possible to specify a state machine table with 0 length,
this is not valid as optional tables are specified by not defining
the table as present. Further this allows by-passing the base tables
range check against the next/check tables.Fixes: d901d6a298dc ("apparmor: dfa split verification of table headers")
Reported-by: Mike Salvatore
Signed-off-by: John Johansen
Signed-off-by: Greg Kroah-Hartman
24 Jun, 2020
4 commits
-
commit 65de50969a77509452ae590e9449b70a22b923bb upstream.
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 0Cc: stable@vger.kernel.org
Signed-off-by: Tom Rix
Acked-by: Stephen Smalley
Signed-off-by: Paul Moore
Signed-off-by: Greg Kroah-Hartman -
[ Upstream commit 3ed4aaa94fc07db3cd0c91be95e3e1b9782a2710 ]
The subset test is not taking into account the unconfined exception
which will cause profile transitions in the stacked confinement
case to fail when no_new_privs is applied.This fixes a regression introduced in the fix for
https://bugs.launchpad.net/bugs/1839037BugLink: https://bugs.launchpad.net/bugs/1844186
Signed-off-by: John Johansen
Signed-off-by: Sasha Levin -
[ Upstream commit 3b646abc5bc6c0df649daea4c2c976bd4d47e4c8 ]
Currently apparmor_sk_clone_security() does not check for existing
label/peer in the 'new' struct sock; it just overwrites it, if any
(with another reference to the label of the source sock.)static void apparmor_sk_clone_security(const struct sock *sk,
struct sock *newsk)
{
struct aa_sk_ctx *ctx = SK_CTX(sk);
struct aa_sk_ctx *new = SK_CTX(newsk);new->label = aa_get_label(ctx->label);
new->peer = aa_get_label(ctx->peer);
}This might leak label references, which might overflow under load.
Thus, check for and put labels, to prevent such errors.Note this is similarly done on:
static int apparmor_socket_post_create(struct socket *sock, ...)
...
if (sock->sk) {
struct aa_sk_ctx *ctx = SK_CTX(sock->sk);aa_put_label(ctx->label);
ctx->label = aa_get_label(label);
}
...Context:
-------The label reference count leak is observed if apparmor_sock_graft()
is called previously: this sets the 'ctx->label' field by getting
a reference to the current label (later overwritten, without put.)static void apparmor_sock_graft(struct sock *sk, ...)
{
struct aa_sk_ctx *ctx = SK_CTX(sk);if (!ctx->label)
ctx->label = aa_get_current_label();
}And that is the case on crypto/af_alg.c:af_alg_accept():
int af_alg_accept(struct sock *sk, struct socket *newsock, ...)
...
struct sock *sk2;
...
sk2 = sk_alloc(...);
...
security_sock_graft(sk2, newsock);
security_sk_clone(sk, sk2);
...Apparently both calls are done on their own right, especially for
other LSMs, being introduced in 2010/2014, before apparmor socket
mediation in 2017 (see commits [1,2,3,4]).So, it looks OK there! Let's fix the reference leak in apparmor.
Test-case:
---------Exercise that code path enough to overflow label reference count.
$ cat aa-refcnt-af_alg.c
#include
#include
#include
#include
#includeint main() {
int sockfd;
struct sockaddr_alg sa;/* Setup the crypto API socket */
sockfd = socket(AF_ALG, SOCK_SEQPACKET, 0);
if (sockfd < 0) {
perror("socket");
return 1;
}memset(&sa, 0, sizeof(sa));
sa.salg_family = AF_ALG;
strcpy((char *) sa.salg_type, "rng");
strcpy((char *) sa.salg_name, "stdrng");if (bind(sockfd, (struct sockaddr *) &sa, sizeof(sa)) < 0) {
perror("bind");
return 1;
}/* Accept a "connection" and close it; repeat. */
while (!close(accept(sockfd, NULL, 0)));return 0;
}$ gcc -o aa-refcnt-af_alg aa-refcnt-af_alg.c
[ 9928.475953] refcount_t overflow at apparmor_sk_clone_security+0x37/0x70 in aa-refcnt-af_alg[1322], uid/euid: 1000/1000
...
[ 9928.507443] RIP: 0010:apparmor_sk_clone_security+0x37/0x70
...
[ 9928.514286] security_sk_clone+0x33/0x50
[ 9928.514807] af_alg_accept+0x81/0x1c0 [af_alg]
[ 9928.516091] alg_accept+0x15/0x20 [af_alg]
[ 9928.516682] SYSC_accept4+0xff/0x210
[ 9928.519609] SyS_accept+0x10/0x20
[ 9928.520190] do_syscall_64+0x73/0x130
[ 9928.520808] entry_SYSCALL_64_after_hwframe+0x3d/0xa2Note that other messages may be seen, not just overflow, depending on
the value being incremented by kref_get(); on another run:[ 7273.182666] refcount_t: saturated; leaking memory.
...
[ 7273.185789] refcount_t: underflow; use-after-free.Kprobes:
-------Using kprobe events to monitor sk -> sk_security -> label -> count (kref):
Original v5.7 (one reference leak every iteration)
... (af_alg_accept+0x0/0x1c0) label=0xffff8a0f36c25eb0 label_refcnt=0x11fd2
... (af_alg_release_parent+0x0/0xd0) label=0xffff8a0f36c25eb0 label_refcnt=0x11fd4
... (af_alg_accept+0x0/0x1c0) label=0xffff8a0f36c25eb0 label_refcnt=0x11fd3
... (af_alg_release_parent+0x0/0xd0) label=0xffff8a0f36c25eb0 label_refcnt=0x11fd5
... (af_alg_accept+0x0/0x1c0) label=0xffff8a0f36c25eb0 label_refcnt=0x11fd4
... (af_alg_release_parent+0x0/0xd0) label=0xffff8a0f36c25eb0 label_refcnt=0x11fd6Patched v5.7 (zero reference leak per iteration)
... (af_alg_accept+0x0/0x1c0) label=0xffff9ff376c25eb0 label_refcnt=0x593
... (af_alg_release_parent+0x0/0xd0) label=0xffff9ff376c25eb0 label_refcnt=0x594
... (af_alg_accept+0x0/0x1c0) label=0xffff9ff376c25eb0 label_refcnt=0x593
... (af_alg_release_parent+0x0/0xd0) label=0xffff9ff376c25eb0 label_refcnt=0x594
... (af_alg_accept+0x0/0x1c0) label=0xffff9ff376c25eb0 label_refcnt=0x593
... (af_alg_release_parent+0x0/0xd0) label=0xffff9ff376c25eb0 label_refcnt=0x594Commits:
-------[1] commit 507cad355fc9 ("crypto: af_alg - Make sure sk_security is initialized on accept()ed sockets")
[2] commit 4c63f83c2c2e ("crypto: af_alg - properly label AF_ALG socket")
[3] commit 2acce6aa9f65 ("Networking") a.k.a ("crypto: af_alg - Avoid sock_graft call warning)
[4] commit 56974a6fcfef ("apparmor: add base infastructure for socket mediation")Fixes: 56974a6fcfef ("apparmor: add base infastructure for socket mediation")
Reported-by: Brian Moyles
Signed-off-by: Mauricio Faria de Oliveira
Signed-off-by: John Johansen
Signed-off-by: Sasha Levin -
[ Upstream commit dd2569fbb053719f7df7ef8fdbb45cf47156a701 ]
Fix two issues with introspecting the task mode.
1. If a task is attached to a unconfined profile that is not the
ns->unconfined profile then. Mode the mode is always reported
as -$ ps -Z
LABEL PID TTY TIME CMD
unconfined 1287 pts/0 00:00:01 bash
test (-) 1892 pts/0 00:00:00 psinstead of the correct value of (unconfined) as shown below
$ ps -Z
LABEL PID TTY TIME CMD
unconfined 2483 pts/0 00:00:01 bash
test (unconfined) 3591 pts/0 00:00:00 ps2. if a task is confined by a stack of profiles that are unconfined
the output of label mode is again the incorrect value of (-) like
above, instead of (unconfined). This is because the visibile
profile count increment is skipped by the special casing of
unconfined.Fixes: f1bd904175e8 ("apparmor: add the base fns() for domain labels")
Signed-off-by: John Johansen
Signed-off-by: Sasha Levin
22 Jun, 2020
11 commits
-
[ Upstream commit b59fda449cf07f2db3be3a67142e6c000f5e8d79 ]
After adding the new add_rule() function in commit c52657d93b05
("ima: refactor ima_init_policy()"), all appraisal flags are added to the
temp_ima_appraise variable. Revert to the previous behavior instead of
removing build_ima_appraise, to benefit from the protection offered by
__ro_after_init.The mentioned commit introduced a bug, as it makes all the flags
modifiable, while build_ima_appraise flags can be protected with
__ro_after_init.Cc: stable@vger.kernel.org # 5.0.x
Fixes: c52657d93b05 ("ima: refactor ima_init_policy()")
Co-developed-by: Roberto Sassu
Signed-off-by: Roberto Sassu
Signed-off-by: Krzysztof Struczynski
Signed-off-by: Mimi Zohar
Signed-off-by: Sasha Levin -
[ Upstream commit 6ee28442a465ab4c4be45e3b15015af24b1ba906 ]
Function ima_appraise_flag() returns the flag to be set in
temp_ima_appraise depending on the hook identifier passed as an argument.
It is not necessary to set the flag again for the POLICY_CHECK hook.Signed-off-by: Krzysztof Struczynski
Signed-off-by: Mimi Zohar
Signed-off-by: Sasha Levin -
commit 0c4395fb2aa77341269ea619c5419ea48171883f upstream.
Don't immediately return if the signature is portable and security.ima is
not present. Just set error so that memory allocated is freed before
returning from evm_calc_hmac_or_hash().Fixes: 50b977481fce9 ("EVM: Add support for portable signature format")
Signed-off-by: Roberto Sassu
Cc: stable@vger.kernel.org
Signed-off-by: Mimi Zohar
Signed-off-by: Greg Kroah-Hartman -
commit 8b8c704d913b0fe490af370631a4200e26334ec0 upstream.
Commit 6cc7c266e5b4 ("ima: Call ima_calc_boot_aggregate() in
ima_eventdigest_init()") added a call to ima_calc_boot_aggregate() so that
the digest can be recalculated for the boot_aggregate measurement entry if
the 'd' template field has been requested. For the 'd' field, only SHA1 and
MD5 digests are accepted.Given that ima_eventdigest_init() does not have the __init annotation, all
functions called should not have it. This patch removes __init from
ima_pcrread().Cc: stable@vger.kernel.org
Fixes: 6cc7c266e5b4 ("ima: Call ima_calc_boot_aggregate() in ima_eventdigest_init()")
Reported-by: Linus Torvalds
Signed-off-by: Roberto Sassu
Signed-off-by: Linus Torvalds
Signed-off-by: Greg Kroah-Hartman -
commit 6cc7c266e5b47d3cd2b5bb7fd3aac4e6bb2dd1d2 upstream.
If the template field 'd' is chosen and the digest to be added to the
measurement entry was not calculated with SHA1 or MD5, it is
recalculated with SHA1, by using the passed file descriptor. However, this
cannot be done for boot_aggregate, because there is no file descriptor.This patch adds a call to ima_calc_boot_aggregate() in
ima_eventdigest_init(), so that the digest can be recalculated also for the
boot_aggregate entry.Cc: stable@vger.kernel.org # 3.13.x
Fixes: 3ce1217d6cd5d ("ima: define template fields library and new helpers")
Reported-by: Takashi Iwai
Signed-off-by: Roberto Sassu
Signed-off-by: Mimi Zohar
Signed-off-by: Greg Kroah-Hartman -
commit 067a436b1b0aafa593344fddd711a755a58afb3b upstream.
This patch prevents the following oops:
[ 10.771813] BUG: kernel NULL pointer dereference, address: 0000000000000
[...]
[ 10.779790] RIP: 0010:ima_match_policy+0xf7/0xb80
[...]
[ 10.798576] Call Trace:
[ 10.798993] ? ima_lsm_policy_change+0x2b0/0x2b0
[ 10.799753] ? inode_init_owner+0x1a0/0x1a0
[ 10.800484] ? _raw_spin_lock+0x7a/0xd0
[ 10.801592] ima_must_appraise.part.0+0xb6/0xf0
[ 10.802313] ? ima_fix_xattr.isra.0+0xd0/0xd0
[ 10.803167] ima_must_appraise+0x4f/0x70
[ 10.804004] ima_post_path_mknod+0x2e/0x80
[ 10.804800] do_mknodat+0x396/0x3c0It occurs when there is a failure during IMA initialization, and
ima_init_policy() is not called. IMA hooks still call ima_match_policy()
but ima_rules is NULL. This patch prevents the crash by directly assigning
the ima_default_policy pointer to ima_rules when ima_rules is defined. This
wouldn't alter the existing behavior, as ima_rules is always set at the end
of ima_init_policy().Cc: stable@vger.kernel.org # 3.7.x
Fixes: 07f6a79415d7d ("ima: add appraise action keywords and default rules")
Reported-by: Takashi Iwai
Signed-off-by: Roberto Sassu
Signed-off-by: Mimi Zohar
Signed-off-by: Greg Kroah-Hartman -
commit e144d6b265415ddbdc54b3f17f4f95133effa5a8 upstream.
Evaluate error in init_ima() before register_blocking_lsm_notifier() and
return if not zero.Cc: stable@vger.kernel.org # 5.3.x
Fixes: b16942455193 ("ima: use the lsm policy update notifier")
Signed-off-by: Roberto Sassu
Reviewed-by: James Morris
Signed-off-by: Mimi Zohar
Signed-off-by: Greg Kroah-Hartman -
commit 6f1a1d103b48b1533a9c804e7a069e2c8e937ce7 upstream.
boot_aggregate is the first entry of IMA measurement list. Its purpose is
to link pre-boot measurements to IMA measurements. As IMA was designed to
work with a TPM 1.2, the SHA1 PCR bank was always selected even if a
TPM 2.0 with support for stronger hash algorithms is available.This patch first tries to find a PCR bank with the IMA default hash
algorithm. If it does not find it, it selects the SHA256 PCR bank for
TPM 2.0 and SHA1 for TPM 1.2. Ultimately, it selects SHA1 also for TPM 2.0
if the SHA256 PCR bank is not found.If none of the PCR banks above can be found, boot_aggregate file digest is
filled with zeros, as for TPM bypass, making it impossible to perform a
remote attestation of the system.Cc: stable@vger.kernel.org # 5.1.x
Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
Reported-by: Jerry Snitselaar
Suggested-by: James Bottomley
Signed-off-by: Roberto Sassu
Signed-off-by: Mimi Zohar
Signed-off-by: Greg Kroah-Hartman -
commit 1129d31b55d509f15e72dc68e4b5c3a4d7b4da8d upstream.
Function hash_long() accepts unsigned long, while currently only one byte
is passed from ima_hash_key(), which calculates a key for ima_htable.Given that hashing the digest does not give clear benefits compared to
using the digest itself, remove hash_long() and return the modulus
calculated on the first two bytes of the digest with the number of slots.
Also reduce the depth of the hash table by doubling the number of slots.Cc: stable@vger.kernel.org
Fixes: 3323eec921ef ("integrity: IMA as an integrity service provider")
Co-developed-by: Roberto Sassu
Signed-off-by: Roberto Sassu
Signed-off-by: Krzysztof Struczynski
Acked-by: David.Laight@aculab.com (big endian system concerns)
Signed-off-by: Mimi Zohar
Signed-off-by: Greg Kroah-Hartman -
[ Upstream commit 4c09f8b6913a779ca0c70ea8058bf21537eebb3b ]
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
Signed-off-by: Paul Moore
Signed-off-by: Sasha Levin -
[ Upstream commit 60cf7c5ed5f7087c4de87a7676b8c82d96fd166c ]
A number of userspace tools, such as systemtap, need a way to see the
current lockdown state so they can gracefully deal with the kernel being
locked down. The state is already exposed in
/sys/kernel/security/lockdown, but is only readable by root. Adjust the
permissions so unprivileged users can read the state.Fixes: 000d388ed3bb ("security: Add a static lockdown policy LSM")
Cc: Frank Ch. Eigler
Signed-off-by: Jeremy Cline
Signed-off-by: James Morris
Signed-off-by: Sasha Levin
17 Jun, 2020
3 commits
-
commit 84e99e58e8d1e26f04c097f4266e431a33987f36 upstream.
Add barrier to soob. Return -EOVERFLOW if the buffer
is exceeded.Suggested-by: Hillf Danton
Reported-by: syzbot+bfdd4a2f07be52351350@syzkaller.appspotmail.com
Signed-off-by: Casey Schaufler
Signed-off-by: Greg Kroah-Hartman -
[ Upstream commit d4eaa2837851db2bfed572898bfc17f9a9f9151e ]
For kvmalloc'ed data object that contains sensitive information like
cryptographic keys, we need to make sure that the buffer is always cleared
before freeing it. Using memset() alone for buffer clearing may not
provide certainty as the compiler may compile it away. To be sure, the
special memzero_explicit() has to be used.This patch introduces a new kvfree_sensitive() for freeing those sensitive
data objects allocated by kvmalloc(). The relevant places where
kvfree_sensitive() can be used are modified to use it.Fixes: 4f0882491a14 ("KEYS: Avoid false positive ENOMEM error on key read")
Suggested-by: Linus Torvalds
Signed-off-by: Waiman Long
Signed-off-by: Andrew Morton
Reviewed-by: Eric Biggers
Acked-by: David Howells
Cc: Jarkko Sakkinen
Cc: James Morris
Cc: "Serge E. Hallyn"
Cc: Joe Perches
Cc: Matthew Wilcox
Cc: David Rientjes
Cc: Uladzislau Rezki
Link: http://lkml.kernel.org/r/20200407200318.11711-1-longman@redhat.com
Signed-off-by: Linus Torvalds
Signed-off-by: Sasha Levin -
[ Upstream commit 00720f0e7f288d29681d265c23b22bb0f0f4e5b4 ]
The mix of IS_ENABLED() and #ifdef checks has left a combination
that causes a warning about an unused variable:security/smack/smack_lsm.c: In function 'smack_socket_connect':
security/smack/smack_lsm.c:2838:24: error: unused variable 'sip' [-Werror=unused-variable]
2838 | struct sockaddr_in6 *sip = (struct sockaddr_in6 *)sap;Change the code to use C-style checks consistently so the compiler
can handle it correctly.Fixes: 87fbfffcc89b ("broken ping to ipv6 linklocal addresses on debian buster")
Signed-off-by: Arnd Bergmann
Signed-off-by: Casey Schaufler
Signed-off-by: Sasha Levin
07 Jun, 2020
2 commits
-
[ Upstream commit 770f60586d2af0590be263f55fd079226313922c ]
This patch fixes the following warning and few other instances of
traversal of evm_config_xattrnames list:[ 32.848432] =============================
[ 32.848707] WARNING: suspicious RCU usage
[ 32.848966] 5.7.0-rc1-00006-ga8d5875ce5f0b #1 Not tainted
[ 32.849308] -----------------------------
[ 32.849567] security/integrity/evm/evm_main.c:231 RCU-list traversed in non-reader section!!Since entries are only added to the list and never deleted, use
list_for_each_entry_lockless() instead of list_for_each_entry_rcu for
traversing the list. Also, add a relevant comment in evm_secfs.c to
indicate this fact.Reported-by: kernel test robot
Suggested-by: Paul E. McKenney
Signed-off-by: Madhuparna Bhowmik
Acked-by: Paul E. McKenney (RCU viewpoint)
Signed-off-by: Mimi Zohar
Signed-off-by: Sasha Levin -
[ Upstream commit 69393cb03ccdf29f3b452d3482ef918469d1c098 ]
Xmon should be either fully or partially disabled depending on the
kernel lockdown state.Put xmon into read-only mode for lockdown=integrity and prevent user
entry into xmon when lockdown=confidentiality. Xmon checks the lockdown
state on every attempted entry:(1) during early xmon'ing
(2) when triggered via sysrq
(3) when toggled via debugfs
(4) when triggered via a previously enabled breakpoint
The following lockdown state transitions are handled:
(1) lockdown=none -> lockdown=integrity
set xmon read-only mode(2) lockdown=none -> lockdown=confidentiality
clear all breakpoints, set xmon read-only mode,
prevent user re-entry into xmon(3) lockdown=integrity -> lockdown=confidentiality
clear all breakpoints, set xmon read-only mode,
prevent user re-entry into xmonSuggested-by: Andrew Donnellan
Signed-off-by: Christopher M. Riedl
Signed-off-by: Michael Ellerman
Link: https://lore.kernel.org/r/20190907061124.1947-3-cmr@informatik.wtf
Signed-off-by: Sasha Levin
03 Jun, 2020
1 commit
-
[ Upstream commit a4ae32c71fe90794127b32d26d7ad795813b502e ]
An invariant of cap_bprm_set_creds is that every field in the new cred
structure that cap_bprm_set_creds might set, needs to be set every
time to ensure the fields does not get a stale value.The field cap_ambient is not set every time cap_bprm_set_creds is
called, which means that if there is a suid or sgid script with an
interpreter that has neither the suid nor the sgid bits set the
interpreter should be able to accept ambient credentials.
Unfortuantely because cap_ambient is not reset to it's original value
the interpreter can not accept ambient credentials.Given that the ambient capability set is expected to be controlled by
the caller, I don't think this is particularly serious. But it is
definitely worth fixing so the code works correctly.I have tested to verify my reading of the code is correct and the
interpreter of a sgid can receive ambient capabilities with this
change and cannot receive ambient capabilities without this change.Cc: stable@vger.kernel.org
Cc: Andy Lutomirski
Fixes: 58319057b784 ("capabilities: ambient capabilities")
Signed-off-by: "Eric W. Biederman"
Signed-off-by: Sasha Levin
27 May, 2020
7 commits
-
commit c6b39f070722ea9963ffe756bfe94e89218c5e63 upstream.
policy_update() invokes begin_current_label_crit_section(), which
returns a reference of the updated aa_label object to "label" with
increased refcount.When policy_update() returns, "label" becomes invalid, so the refcount
should be decreased to keep refcount balanced.The reference counting issue happens in one exception handling path of
policy_update(). When aa_may_manage_policy() returns not NULL, the
refcnt increased by begin_current_label_crit_section() is not decreased,
causing a refcnt leak.Fix this issue by jumping to "end_section" label when
aa_may_manage_policy() returns not NULL.Fixes: 5ac8c355ae00 ("apparmor: allow introspecting the loaded policy pre internal transform")
Signed-off-by: Xiyu Yang
Signed-off-by: Xin Tan
Signed-off-by: John Johansen
Signed-off-by: Greg Kroah-Hartman -
commit a0b845ffa0d91855532b50fc040aeb2d8338dca4 upstream.
aa_change_profile() invokes aa_get_current_label(), which returns
a reference of the current task's label.According to the comment of aa_get_current_label(), the returned
reference must be put with aa_put_label().
However, when the original object pointed by "label" becomes
unreachable because aa_change_profile() returns or a new object
is assigned to "label", reference count increased by
aa_get_current_label() is not decreased, causing a refcnt leak.Fix this by calling aa_put_label() before aa_change_profile() return
and dropping unnecessary aa_get_current_label().Fixes: 9fcf78cca198 ("apparmor: update domain transitions that are subsets of confinement at nnp")
Signed-off-by: Xiyu Yang
Signed-off-by: Xin Tan
Signed-off-by: John Johansen
Signed-off-by: Greg Kroah-Hartman -
commit c54d481d71c6849e044690d3960aaebc730224cc upstream.
In the implementation of aa_audit_rule_init(), when aa_label_parse()
fails the allocated memory for rule is released using
aa_audit_rule_free(). But after this release, the return statement
tries to access the label field of the rule which results in
use-after-free. Before releasing the rule, copy errNo and return it
after release.Fixes: 52e8c38001d8 ("apparmor: Fix memory leak of rule on error exit path")
Signed-off-by: Navid Emamdoost
Signed-off-by: John Johansen
Signed-off-by: Greg Kroah-Hartman -
[ Upstream commit 8433856947217ebb5697a8ff9c4c9cad4639a2cf ]
The IS_ERR_OR_NULL() function has two conditions and if we got really
unlucky we could hit a race where "ptr" started as an error pointer and
then was set to NULL. Both conditions would be false even though the
pointer at the end was NULL.This patch fixes the problem by ensuring that "*tfm" can only be NULL
or valid. I have introduced a "tmp_tfm" variable to make that work. I
also reversed a condition and pulled the code in one tab.Reported-by: Roberto Sassu
Fixes: 53de3b080d5e ("evm: Check also if *tfm is an error pointer in init_desc()")
Signed-off-by: Dan Carpenter
Acked-by: Roberto Sassu
Acked-by: Krzysztof Struczynski
Signed-off-by: Mimi Zohar
Signed-off-by: Sasha Levin -
[ Upstream commit 2e3a34e9f409ebe83d1af7cd2f49fca7af97dfac ]
This patch fixes the return value of ima_write_policy() when a new policy
is directly passed to IMA and the current policy requires appraisal of the
file containing the policy. Currently, if appraisal is not in ENFORCE mode,
ima_write_policy() returns 0 and leads user space applications to an
endless loop. Fix this issue by denying the operation regardless of the
appraisal mode.Cc: stable@vger.kernel.org # 4.10.x
Fixes: 19f8a84713edc ("ima: measure and appraise the IMA policy itself")
Signed-off-by: Roberto Sassu
Reviewed-by: Krzysztof Struczynski
Signed-off-by: Mimi Zohar
Signed-off-by: Sasha Levin -
[ Upstream commit 53de3b080d5eae31d0de219617155dcc34e7d698 ]
This patch avoids a kernel panic due to accessing an error pointer set by
crypto_alloc_shash(). It occurs especially when there are many files that
require an unsupported algorithm, as it would increase the likelihood of
the following race condition:Task A: *tfm = crypto_alloc_shash()
Signed-off-by: Krzysztof Struczynski
Signed-off-by: Roberto Sassu
Signed-off-by: Mimi Zohar
Signed-off-by: Sasha Levin -
[ Upstream commit 0014cc04e8ec077dc482f00c87dfd949cfe2b98f ]
Commit a408e4a86b36 ("ima: open a new file instance if no read
permissions") tries to create a new file descriptor to calculate a file
digest if the file has not been opened with O_RDONLY flag. However, if a
new file descriptor cannot be obtained, it sets the FMODE_READ flag to
file->f_flags instead of file->f_mode.This patch fixes this issue by replacing f_flags with f_mode as it was
before that commit.Cc: stable@vger.kernel.org # 4.20.x
Fixes: a408e4a86b36 ("ima: open a new file instance if no read permissions")
Signed-off-by: Roberto Sassu
Reviewed-by: Goldwyn Rodrigues
Signed-off-by: Mimi Zohar
Signed-off-by: Sasha Levin
06 May, 2020
1 commit
-
commit fb73974172ffaaf57a7c42f35424d9aece1a5af6 upstream.
Fix the SELinux netlink_send hook to properly handle multiple netlink
messages in a single sk_buff; each message is parsed and subject to
SELinux access control. Prior to this patch, SELinux only inspected
the first message in the sk_buff.Cc: stable@vger.kernel.org
Reported-by: Dmitry Vyukov
Reviewed-by: Stephen Smalley
Signed-off-by: Paul Moore
Signed-off-by: Greg Kroah-Hartman
29 Apr, 2020
1 commit
-
[ Upstream commit 4f0882491a148059a52480e753b7f07fc550e188 ]
By allocating a kernel buffer with a user-supplied buffer length, it
is possible that a false positive ENOMEM error may be returned because
the user-supplied length is just too large even if the system do have
enough memory to hold the actual key data.Moreover, if the buffer length is larger than the maximum amount of
memory that can be returned by kmalloc() (2^(MAX_ORDER-1) number of
pages), a warning message will also be printed.To reduce this possibility, we set a threshold (PAGE_SIZE) over which we
do check the actual key length first before allocating a buffer of the
right size to hold it. The threshold is arbitrary, it is just used to
trigger a buffer length check. It does not limit the actual key length
as long as there is enough memory to satisfy the memory request.To further avoid large buffer allocation failure due to page
fragmentation, kvmalloc() is used to allocate the buffer so that vmapped
pages can be used when there is not a large enough contiguous set of
pages available for allocation.In the extremely unlikely scenario that the key keeps on being changed
and made longer (still
Signed-off-by: David Howells
Signed-off-by: Sasha Levin
23 Apr, 2020
1 commit
-
commit d3ec10aa95819bff18a0d936b18884c7816d0914 upstream.
A lockdep circular locking dependency report was seen when running a
keyutils test:[12537.027242] ======================================================
[12537.059309] WARNING: possible circular locking dependency detected
[12537.088148] 4.18.0-147.7.1.el8_1.x86_64+debug #1 Tainted: G OE --------- - -
[12537.125253] ------------------------------------------------------
[12537.153189] keyctl/25598 is trying to acquire lock:
[12537.175087] 000000007c39f96c (&mm->mmap_sem){++++}, at: __might_fault+0xc4/0x1b0
[12537.208365]
[12537.208365] but task is already holding lock:
[12537.234507] 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
[12537.270476]
[12537.270476] which lock already depends on the new lock.
[12537.270476]
[12537.307209]
[12537.307209] the existing dependency chain (in reverse order) is:
[12537.340754]
[12537.340754] -> #3 (&type->lock_class){++++}:
[12537.367434] down_write+0x4d/0x110
[12537.385202] __key_link_begin+0x87/0x280
[12537.405232] request_key_and_link+0x483/0xf70
[12537.427221] request_key+0x3c/0x80
[12537.444839] dns_query+0x1db/0x5a5 [dns_resolver]
[12537.468445] dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
[12537.496731] cifs_reconnect+0xe04/0x2500 [cifs]
[12537.519418] cifs_readv_from_socket+0x461/0x690 [cifs]
[12537.546263] cifs_read_from_socket+0xa0/0xe0 [cifs]
[12537.573551] cifs_demultiplex_thread+0x311/0x2db0 [cifs]
[12537.601045] kthread+0x30c/0x3d0
[12537.617906] ret_from_fork+0x3a/0x50
[12537.636225]
[12537.636225] -> #2 (root_key_user.cons_lock){+.+.}:
[12537.664525] __mutex_lock+0x105/0x11f0
[12537.683734] request_key_and_link+0x35a/0xf70
[12537.705640] request_key+0x3c/0x80
[12537.723304] dns_query+0x1db/0x5a5 [dns_resolver]
[12537.746773] dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
[12537.775607] cifs_reconnect+0xe04/0x2500 [cifs]
[12537.798322] cifs_readv_from_socket+0x461/0x690 [cifs]
[12537.823369] cifs_read_from_socket+0xa0/0xe0 [cifs]
[12537.847262] cifs_demultiplex_thread+0x311/0x2db0 [cifs]
[12537.873477] kthread+0x30c/0x3d0
[12537.890281] ret_from_fork+0x3a/0x50
[12537.908649]
[12537.908649] -> #1 (&tcp_ses->srv_mutex){+.+.}:
[12537.935225] __mutex_lock+0x105/0x11f0
[12537.954450] cifs_call_async+0x102/0x7f0 [cifs]
[12537.977250] smb2_async_readv+0x6c3/0xc90 [cifs]
[12538.000659] cifs_readpages+0x120a/0x1e50 [cifs]
[12538.023920] read_pages+0xf5/0x560
[12538.041583] __do_page_cache_readahead+0x41d/0x4b0
[12538.067047] ondemand_readahead+0x44c/0xc10
[12538.092069] filemap_fault+0xec1/0x1830
[12538.111637] __do_fault+0x82/0x260
[12538.129216] do_fault+0x419/0xfb0
[12538.146390] __handle_mm_fault+0x862/0xdf0
[12538.167408] handle_mm_fault+0x154/0x550
[12538.187401] __do_page_fault+0x42f/0xa60
[12538.207395] do_page_fault+0x38/0x5e0
[12538.225777] page_fault+0x1e/0x30
[12538.243010]
[12538.243010] -> #0 (&mm->mmap_sem){++++}:
[12538.267875] lock_acquire+0x14c/0x420
[12538.286848] __might_fault+0x119/0x1b0
[12538.306006] keyring_read_iterator+0x7e/0x170
[12538.327936] assoc_array_subtree_iterate+0x97/0x280
[12538.352154] keyring_read+0xe9/0x110
[12538.370558] keyctl_read_key+0x1b9/0x220
[12538.391470] do_syscall_64+0xa5/0x4b0
[12538.410511] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
[12538.435535]
[12538.435535] other info that might help us debug this:
[12538.435535]
[12538.472829] Chain exists of:
[12538.472829] &mm->mmap_sem --> root_key_user.cons_lock --> &type->lock_class
[12538.472829]
[12538.524820] Possible unsafe locking scenario:
[12538.524820]
[12538.551431] CPU0 CPU1
[12538.572654] ---- ----
[12538.595865] lock(&type->lock_class);
[12538.613737] lock(root_key_user.cons_lock);
[12538.644234] lock(&type->lock_class);
[12538.672410] lock(&mm->mmap_sem);
[12538.687758]
[12538.687758] *** DEADLOCK ***
[12538.687758]
[12538.714455] 1 lock held by keyctl/25598:
[12538.732097] #0: 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
[12538.770573]
[12538.770573] stack backtrace:
[12538.790136] CPU: 2 PID: 25598 Comm: keyctl Kdump: loaded Tainted: G
[12538.844855] Hardware name: HP ProLiant DL360 Gen9/ProLiant DL360 Gen9, BIOS P89 12/27/2015
[12538.881963] Call Trace:
[12538.892897] dump_stack+0x9a/0xf0
[12538.907908] print_circular_bug.isra.25.cold.50+0x1bc/0x279
[12538.932891] ? save_trace+0xd6/0x250
[12538.948979] check_prev_add.constprop.32+0xc36/0x14f0
[12538.971643] ? keyring_compare_object+0x104/0x190
[12538.992738] ? check_usage+0x550/0x550
[12539.009845] ? sched_clock+0x5/0x10
[12539.025484] ? sched_clock_cpu+0x18/0x1e0
[12539.043555] __lock_acquire+0x1f12/0x38d0
[12539.061551] ? trace_hardirqs_on+0x10/0x10
[12539.080554] lock_acquire+0x14c/0x420
[12539.100330] ? __might_fault+0xc4/0x1b0
[12539.119079] __might_fault+0x119/0x1b0
[12539.135869] ? __might_fault+0xc4/0x1b0
[12539.153234] keyring_read_iterator+0x7e/0x170
[12539.172787] ? keyring_read+0x110/0x110
[12539.190059] assoc_array_subtree_iterate+0x97/0x280
[12539.211526] keyring_read+0xe9/0x110
[12539.227561] ? keyring_gc_check_iterator+0xc0/0xc0
[12539.249076] keyctl_read_key+0x1b9/0x220
[12539.266660] do_syscall_64+0xa5/0x4b0
[12539.283091] entry_SYSCALL_64_after_hwframe+0x6a/0xdfOne way to prevent this deadlock scenario from happening is to not
allow writing to userspace while holding the key semaphore. Instead,
an internal buffer is allocated for getting the keys out from the
read method first before copying them out to userspace without holding
the lock.That requires taking out the __user modifier from all the relevant
read methods as well as additional changes to not use any userspace
write helpers. That is,1) The put_user() call is replaced by a direct copy.
2) The copy_to_user() call is replaced by memcpy().
3) All the fault handling code is removed.Compiling on a x86-64 system, the size of the rxrpc_read() function is
reduced from 3795 bytes to 2384 bytes with this patch.Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
Reviewed-by: Jarkko Sakkinen
Signed-off-by: Waiman Long
Signed-off-by: David Howells
Signed-off-by: Greg Kroah-Hartman
21 Apr, 2020
1 commit
-
commit 86d32f9a7c54ad74f4514d7fef7c847883207291 upstream.
If seq_file .next function does not change position index,
read after some lseek can generate unexpected output:$ dd if=/proc/keys bs=1 # full usual output
0f6bfdf5 I--Q--- 2 perm 3f010000 1000 1000 user 4af2f79ab8848d0a: 740
1fb91b32 I--Q--- 3 perm 1f3f0000 1000 65534 keyring _uid.1000: 2
27589480 I--Q--- 1 perm 0b0b0000 0 0 user invocation_id: 16
2f33ab67 I--Q--- 152 perm 3f030000 0 0 keyring _ses: 2
33f1d8fa I--Q--- 4 perm 3f030000 1000 1000 keyring _ses: 1
3d427fda I--Q--- 2 perm 3f010000 1000 1000 user 69ec44aec7678e5a: 740
3ead4096 I--Q--- 1 perm 1f3f0000 1000 65534 keyring _uid_ses.1000: 1
521+0 records in
521+0 records out
521 bytes copied, 0,00123769 s, 421 kB/sBut a read after lseek in middle of last line results in the partial
last line and then a repeat of the final line:$ dd if=/proc/keys bs=500 skip=1
dd: /proc/keys: cannot skip to specified offset
g _uid_ses.1000: 1
3ead4096 I--Q--- 1 perm 1f3f0000 1000 65534 keyring _uid_ses.1000: 1
0+1 records in
0+1 records out
97 bytes copied, 0,000135035 s, 718 kB/sand a read after lseek beyond end of file results in the last line being
shown:$ dd if=/proc/keys bs=1000 skip=1 # read after lseek beyond end of file
dd: /proc/keys: cannot skip to specified offset
3ead4096 I--Q--- 1 perm 1f3f0000 1000 65534 keyring _uid_ses.1000: 1
0+1 records in
0+1 records out
76 bytes copied, 0,000119981 s, 633 kB/sSee https://bugzilla.kernel.org/show_bug.cgi?id=206283
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
Signed-off-by: Vasily Averin
Signed-off-by: David Howells
Reviewed-by: Jarkko Sakkinen
Cc: stable@vger.kernel.org
Signed-off-by: Linus Torvalds
Signed-off-by: Greg Kroah-Hartman
17 Apr, 2020
1 commit
-
commit 2e356101e72ab1361821b3af024d64877d9a798d upstream.
Currently, when we add a new user key, the calltrace as below:
add_key()
key_create_or_update()
key_alloc()
__key_instantiate_and_link
generic_key_instantiate
key_payload_reserve
......Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
we can reach max bytes/keys in key_alloc, but we forget to remove this
limit when we reserver space for payload in key_payload_reserve. So we
can only reach max keys but not max bytes when having delta between plen
and type->def_datalen. Remove this limit when instantiating the key, so we
can keep consistent with key_alloc.Also, fix the similar problem in keyctl_chown_key().
Fixes: 0b77f5bfb45c ("keys: make the keyring quotas controllable through /proc/sys")
Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
Cc: stable@vger.kernel.org # 5.0.x
Cc: Eric Biggers
Signed-off-by: Yang Xu
Reviewed-by: Jarkko Sakkinen
Reviewed-by: Eric Biggers
Signed-off-by: Jarkko Sakkinen
Signed-off-by: Greg Kroah-Hartman
12 Mar, 2020
1 commit
-
[ Upstream commit 3be54d558c75562e42bc83d665df024bd79d399b ]
If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
from the db, dbx and MokListRT EFI variables into the appropriate keyrings.But it just assumes that the variables will be present and prints an error
if the certs can't be loaded, even when is possible that the variables may
not exist. For example the MokListRT variable will only be present if shim
is used.So only print an error message about failing to get the certs list from an
EFI variable if this is found. Otherwise these printed errors just pollute
the kernel log ring buffer with confusing messages like the following:[ 5.427251] Couldn't get size: 0x800000000000000e
[ 5.427261] MODSIGN: Couldn't get UEFI db list
[ 5.428012] Couldn't get size: 0x800000000000000e
[ 5.428023] Couldn't get UEFI MokListRTReported-by: Hans de Goede
Signed-off-by: Javier Martinez Canillas
Tested-by: Hans de Goede
Acked-by: Ard Biesheuvel
Signed-off-by: Mimi Zohar
Signed-off-by: Sasha Levin
05 Mar, 2020
1 commit
-
commit 483ec26eed42bf050931d9a5c5f9f0b5f2ad5f3b upstream.
Keep the ima policy rules around from the beginning even if they appear
invalid at the time of loading, as they may become active after an lsm
policy load. However, loading a custom IMA policy with unknown LSM
labels is only safe after we have transitioned from the "built-in"
policy rules to a custom IMA policy.Patch also fixes the rule re-use during the lsm policy reload and makes
some prints a bit more human readable.Changelog:
v4:
- Do not allow the initial policy load refer to non-existing lsm rules.
v3:
- Fix too wide policy rule matching for non-initialized LSMs
v2:
- Fix log printsFixes: b16942455193 ("ima: use the lsm policy update notifier")
Cc: Casey Schaufler
Reported-by: Mimi Zohar
Signed-off-by: Janne Karhunen
Signed-off-by: Konsta Karsisto
Signed-off-by: Mimi Zohar
Signed-off-by: Greg Kroah-Hartman
24 Feb, 2020
2 commits
-
[ Upstream commit 030b995ad9ece9fa2d218af4429c1c78c2342096 ]
In AVC update we don't call avc_node_kill() when avc_xperms_populate()
fails, resulting in the avc->avc_cache.active_nodes counter having a
false value. In last patch this changes was missed , so correcting it.Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls")
Signed-off-by: Jaihind Yadav
Signed-off-by: Ravi Kumar Siddojigari
[PM: merge fuzz, minor description cleanup]
Signed-off-by: Paul Moore
Signed-off-by: Sasha Levin -
[ Upstream commit d8db60cb23e49a92cf8cada3297395c7fa50fdf8 ]
Fix avc_insert() to call avc_node_kill() if we've already allocated
an AVC node and the code fails to insert the node in the cache.Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls")
Reported-by: rsiddoji@codeaurora.org
Suggested-by: Stephen Smalley
Acked-by: Stephen Smalley
Signed-off-by: Paul Moore
Signed-off-by: Sasha Levin
15 Feb, 2020
2 commits
-
commit 0188d5c025ca8fe756ba3193bd7d150139af5a88 upstream.
commit bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
passed down the rcu flag to the SELinux AVC, but failed to adjust the
test in slow_avc_audit() to also return -ECHILD on LSM_AUDIT_DATA_DENTRY.
Previously, we only returned -ECHILD if generating an audit record with
LSM_AUDIT_DATA_INODE since this was only relevant from inode_permission.
Move the handling of MAY_NOT_BLOCK to avc_audit() and its inlined
equivalent in selinux_inode_permission() immediately after we determine
that audit is required, and always fall back to ref-walk in this case.Fixes: bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
Reported-by: Will Deacon
Suggested-by: Al Viro
Signed-off-by: Stephen Smalley
Signed-off-by: Paul Moore
Signed-off-by: Greg Kroah-Hartman -
commit 98aa00345de54b8340dc2ddcd87f446d33387b5e upstream.
commit 2db154b3ea8e ("vfs: syscall: Add move_mount(2) to move mounts around")
introduced a new move_mount(2) system call and a corresponding new LSM
security_move_mount hook but did not implement this hook for any existing
LSM. This creates a regression for SELinux with respect to consistent
checking of mounts; the existing selinux_mount hook checks mounton
permission to the mount point path. Provide a SELinux hook
implementation for move_mount that applies this same check for
consistency. In the future we may wish to add a new move_mount
filesystem permission and check as well, but this addresses
the immediate regression.Fixes: 2db154b3ea8e ("vfs: syscall: Add move_mount(2) to move mounts around")
Signed-off-by: Stephen Smalley
Reviewed-by: Ondrej Mosnacek
Signed-off-by: Paul Moore
Signed-off-by: Greg Kroah-Hartman
