* [RFC PATCH 02/17] security: Add hooks to rule on setting a watch [ver #4]
From: David Howells @ 2020-03-09 12:17 UTC (permalink / raw)
To: torvalds, viro
Cc: Casey Schaufler, Stephen Smalley, linux-security-module, dhowells,
dhowells, casey, sds, nicolas.dichtel, raven, christian, andres,
jlayton, dray, kzak, keyrings, linux-api, linux-fsdevel,
linux-security-module, linux-kernel
In-Reply-To: <158375623086.334846.16121725232323108842.stgit@warthog.procyon.org.uk>
Add security hooks that will allow an LSM to rule on whether or not a watch
may be set. More than one hook is required as the watches watch different
types of object.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Casey Schaufler <casey@schaufler-ca.com>
cc: Stephen Smalley <sds@tycho.nsa.gov>
cc: linux-security-module@vger.kernel.org
---
include/linux/lsm_hooks.h | 24 ++++++++++++++++++++++++
include/linux/security.h | 17 +++++++++++++++++
security/security.c | 14 ++++++++++++++
3 files changed, 55 insertions(+)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 20d8cf194fb7..79d7c73676d7 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1416,6 +1416,18 @@
* @ctx is a pointer in which to place the allocated security context.
* @ctxlen points to the place to put the length of @ctx.
*
+ * Security hooks for the general notification queue:
+ *
+ * @watch_key:
+ * Check to see if a process is allowed to watch for event notifications
+ * from a key or keyring.
+ * @key: The key to watch.
+ *
+ * @watch_devices:
+ * Check to see if a process is allowed to watch for event notifications
+ * from devices (as a global set).
+ *
+ *
* Security hooks for using the eBPF maps and programs functionalities through
* eBPF syscalls.
*
@@ -1698,6 +1710,12 @@ union security_list_options {
int (*inode_notifysecctx)(struct inode *inode, void *ctx, u32 ctxlen);
int (*inode_setsecctx)(struct dentry *dentry, void *ctx, u32 ctxlen);
int (*inode_getsecctx)(struct inode *inode, void **ctx, u32 *ctxlen);
+#ifdef CONFIG_KEY_NOTIFICATIONS
+ int (*watch_key)(struct key *key);
+#endif
+#ifdef CONFIG_DEVICE_NOTIFICATIONS
+ int (*watch_devices)(void);
+#endif
#ifdef CONFIG_SECURITY_NETWORK
int (*unix_stream_connect)(struct sock *sock, struct sock *other,
@@ -1985,6 +2003,12 @@ struct security_hook_heads {
struct hlist_head inode_notifysecctx;
struct hlist_head inode_setsecctx;
struct hlist_head inode_getsecctx;
+#ifdef CONFIG_KEY_NOTIFICATIONS
+ struct hlist_head watch_key;
+#endif
+#ifdef CONFIG_DEVICE_NOTIFICATIONS
+ struct hlist_head watch_devices;
+#endif
#ifdef CONFIG_SECURITY_NETWORK
struct hlist_head unix_stream_connect;
struct hlist_head unix_may_send;
diff --git a/include/linux/security.h b/include/linux/security.h
index 64b19f050343..7a36064a64ea 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1276,6 +1276,23 @@ static inline int security_locked_down(enum lockdown_reason what)
}
#endif /* CONFIG_SECURITY */
+#if defined(CONFIG_SECURITY) && defined(CONFIG_KEY_NOTIFICATIONS)
+int security_watch_key(struct key *key);
+#else
+static inline int security_watch_key(struct key *key)
+{
+ return 0;
+}
+#endif
+#if defined(CONFIG_SECURITY) && defined(CONFIG_DEVICE_NOTIFICATIONS)
+int security_watch_devices(void);
+#else
+static inline int security_watch_devices(void)
+{
+ return 0;
+}
+#endif
+
#ifdef CONFIG_SECURITY_NETWORK
int security_unix_stream_connect(struct sock *sock, struct sock *other, struct sock *newsk);
diff --git a/security/security.c b/security/security.c
index 565bc9b67276..22877f47cf62 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1988,6 +1988,20 @@ int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
}
EXPORT_SYMBOL(security_inode_getsecctx);
+#ifdef CONFIG_KEY_NOTIFICATIONS
+int security_watch_key(struct key *key)
+{
+ return call_int_hook(watch_key, 0, key);
+}
+#endif
+
+#ifdef CONFIG_DEVICE_NOTIFICATIONS
+int security_watch_devices(void)
+{
+ return call_int_hook(watch_devices, 0);
+}
+#endif
+
#ifdef CONFIG_SECURITY_NETWORK
int security_unix_stream_connect(struct sock *sock, struct sock *other, struct sock *newsk)
^ permalink raw reply related
* [RFC PATCH 01/17] uapi: General notification queue definitions [ver #4]
From: David Howells @ 2020-03-09 12:17 UTC (permalink / raw)
To: torvalds, viro
Cc: dhowells, dhowells, casey, sds, nicolas.dichtel, raven, christian,
andres, jlayton, dray, kzak, keyrings, linux-api, linux-fsdevel,
linux-security-module, linux-kernel
In-Reply-To: <158375623086.334846.16121725232323108842.stgit@warthog.procyon.org.uk>
Add UAPI definitions for the general notification queue, including the
following pieces:
(*) struct watch_notification.
This is the metadata header for notification messages. It includes a
type and subtype that indicate the source of the message
(eg. WATCH_TYPE_MOUNT_NOTIFY) and the kind of the message
(eg. NOTIFY_MOUNT_NEW_MOUNT).
The header also contains an information field that conveys the
following information:
- WATCH_INFO_LENGTH. The size of the entry (entries are variable
length).
- WATCH_INFO_ID. The watch ID specified when the watchpoint was
set.
- WATCH_INFO_TYPE_INFO. (Sub)type-specific information.
- WATCH_INFO_FLAG_*. Flag bits overlain on the type-specific
information. For use by the type.
All the information in the header can be used in filtering messages at
the point of writing into the buffer.
(*) struct watch_notification_removal
This is an extended watch-removal notification record that includes an
'id' field that can indicate the identifier of the object being
removed if available (for instance, a keyring serial number).
Signed-off-by: David Howells <dhowells@redhat.com>
---
include/uapi/linux/watch_queue.h | 55 ++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
create mode 100644 include/uapi/linux/watch_queue.h
diff --git a/include/uapi/linux/watch_queue.h b/include/uapi/linux/watch_queue.h
new file mode 100644
index 000000000000..5f3d21e8a34b
--- /dev/null
+++ b/include/uapi/linux/watch_queue.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_LINUX_WATCH_QUEUE_H
+#define _UAPI_LINUX_WATCH_QUEUE_H
+
+#include <linux/types.h>
+
+enum watch_notification_type {
+ WATCH_TYPE_META = 0, /* Special record */
+ WATCH_TYPE__NR = 1
+};
+
+enum watch_meta_notification_subtype {
+ WATCH_META_REMOVAL_NOTIFICATION = 0, /* Watched object was removed */
+ WATCH_META_LOSS_NOTIFICATION = 1, /* Data loss occurred */
+};
+
+/*
+ * Notification record header. This is aligned to 64-bits so that subclasses
+ * can contain __u64 fields.
+ */
+struct watch_notification {
+ __u32 type:24; /* enum watch_notification_type */
+ __u32 subtype:8; /* Type-specific subtype (filterable) */
+ __u32 info;
+#define WATCH_INFO_LENGTH 0x0000007f /* Length of record */
+#define WATCH_INFO_LENGTH__SHIFT 0
+#define WATCH_INFO_ID 0x0000ff00 /* ID of watchpoint */
+#define WATCH_INFO_ID__SHIFT 8
+#define WATCH_INFO_TYPE_INFO 0xffff0000 /* Type-specific info */
+#define WATCH_INFO_TYPE_INFO__SHIFT 16
+#define WATCH_INFO_FLAG_0 0x00010000 /* Type-specific info, flag bit 0 */
+#define WATCH_INFO_FLAG_1 0x00020000 /* ... */
+#define WATCH_INFO_FLAG_2 0x00040000
+#define WATCH_INFO_FLAG_3 0x00080000
+#define WATCH_INFO_FLAG_4 0x00100000
+#define WATCH_INFO_FLAG_5 0x00200000
+#define WATCH_INFO_FLAG_6 0x00400000
+#define WATCH_INFO_FLAG_7 0x00800000
+};
+
+
+/*
+ * Extended watch removal notification. This is used optionally if the type
+ * wants to indicate an identifier for the object being watched, if there is
+ * such. This can be distinguished by the length.
+ *
+ * type -> WATCH_TYPE_META
+ * subtype -> WATCH_META_REMOVAL_NOTIFICATION
+ */
+struct watch_notification_removal {
+ struct watch_notification watch;
+ __u64 id; /* Type-dependent identifier */
+};
+
+#endif /* _UAPI_LINUX_WATCH_QUEUE_H */
^ permalink raw reply related
* [RFC PATCH 00/17] pipe: Keyrings, mount and superblock notifications [ver #4]
From: David Howells @ 2020-03-09 12:17 UTC (permalink / raw)
To: torvalds, viro
Cc: Casey Schaufler, Stephen Smalley, linux-security-module, dhowells,
dhowells, casey, sds, nicolas.dichtel, raven, christian, andres,
jlayton, dray, kzak, keyrings, linux-api, linux-fsdevel,
linux-security-module, linux-kernel
Here's a set of patches to add a general notification queue concept and to
add event sources such as:
(1) Keys/keyrings, such as linking and unlinking keys and changing their
attributes.
(2) Mount topology events, such as mounting, unmounting, mount expiry,
mount reconfiguration.
(3) Superblock events, such as R/W<->R/O changes, quota overrun and I/O
errors (not complete yet).
LSM hooks are included:
(1) A set of hooks are provided that allow an LSM to rule on whether or
not a watch may be set. Each of these hooks takes a different
"watched object" parameter, so they're not really shareable. The LSM
should use current's credentials. [Wanted by SELinux & Smack]
(2) A hook is provided to allow an LSM to rule on whether or not a
particular message may be posted to a particular queue. This is given
the credentials from the event generator (which may be the system) and
the watch setter. [Wanted by Smack]
I've provided SELinux and Smack with implementations of some of these hooks.
WHY
===
(1) Key/keyring notifications.
If you have your kerberos tickets in a file/directory, your gnome
desktop will monitor that using something like fanotify and tell you
if your credentials cache changes.
We also have the ability to cache your kerberos tickets in the
session, user or persistent keyring so that it isn't left around on
disk across a reboot or logout. Keyrings, however, cannot currently
be monitored asynchronously, so the desktop has to poll for it - not
so good on a laptop.
This source will allow the desktop to avoid the need to poll. Here's
a pull request for usage by gnome-online-accounts:
https://gitlab.gnome.org/GNOME/gnome-online-accounts/merge_requests/47
(2) Mount notifications.
This one is wanted to avoid repeated trawling of /proc/mounts or
similar to work out changes to the mount object attributes and mount
topology. I'm told that the proc file holding the namespace_sem is a
point of contention, especially as the process of generating the text
descriptions of the mounts/superblocks can be quite involved.
Whilst you can use poll() on /proc/mounts, it doesn't give you any
clues as to what changed. The notification generated here directly
indicates the mounts involved in any particular event and gives an
idea of what the change was.
This is combined with a new fsinfo() system call that allows, amongst
other things, the ability to retrieve in one go an { id,
change_counter } tuple from all the children of a specified mount,
allowing buffer overruns to be dealt with quickly.
This is of use to systemd to improve efficiency:
https://lore.kernel.org/linux-fsdevel/20200227151421.3u74ijhqt6ekbiss@ws.net.home/
And it's not just Red Hat that's potentially interested in this:
https://lore.kernel.org/linux-fsdevel/293c9bd3-f530-d75e-c353-ddeabac27cf6@6wind.com/
(3) Superblock notifications.
This one is provided to allow systemd or the desktop to more easily
detect events such as I/O errors and EDQUOT/ENOSPC. This would be of
interest to Postgres:
https://lore.kernel.org/linux-fsdevel/20200211005626.7yqjf5rbs3vbwagd@alap3.anarazel.de/
Design decisions:
(1) The notification queue is built on top of a standard pipe. Messages
are effectively spliced in. The pipe is opened with a special flag:
pipe2(fds, O_NOTIFICATION_PIPE);
The special flag has the same value as O_EXCL (which doesn't seem like
it will ever be applicable in this context)[?]. It is given up front
to make it a lot easier to prohibit splice and co. from accessing the
pipe.
[?] Should this be done some other way? I'd rather not use up a new
O_* flag if I can avoid it - should I add a pipe3() system call
instead?
The pipe is then configured::
ioctl(fds[1], IOC_WATCH_QUEUE_SET_SIZE, queue_depth);
ioctl(fds[1], IOC_WATCH_QUEUE_SET_FILTER, &filter);
Messages are then read out of the pipe using read().
(2) It should be possible to allow write() to insert data into the
notification pipes too, but this is currently disabled as the kernel
has to be able to insert messages into the pipe *without* holding
pipe->mutex and the code to make this work needs careful auditing.
(3) sendfile(), splice() and vmsplice() are disabled on notification pipes
because of the pipe->mutex issue and also because they sometimes want
to revert what they just did - but one or more notification messages
might've been interleaved in the ring.
(4) The kernel inserts messages with the wait queue spinlock held. This
means that pipe_read() and pipe_write() have to take the spinlock to
update the queue pointers.
(5) Records in the buffer are binary, typed and have a length so that they
can be of varying size.
This allows multiple heterogeneous sources to share a common buffer;
there are 16 million types available, of which I've used just a few,
so there is scope for others to be used. Tags may be specified when a
watchpoint is created to help distinguish the sources.
(6) Records are filterable as types have up to 256 subtypes that can be
individually filtered. Other filtration is also available.
(7) Notification pipes don't interfere with each other; each may be bound
to a different set of watches. Any particular notification will be
copied to all the queues that are currently watching for it - and only
those that are watching for it.
(8) When recording a notification, the kernel will not sleep, but will
rather mark a queue as having lost a message if there's insufficient
space. read() will fabricate a loss notification message at an
appropriate point later.
(9) The notification pipe is created and then watchpoints are attached to
it, using one of:
keyctl_watch_key(KEY_SPEC_SESSION_KEYRING, fds[1], 0x01);
watch_mount(AT_FDCWD, "/", 0, fd, 0x02);
watch_sb(AT_FDCWD, "/mnt", 0, fd, 0x03);
where in both cases, fd indicates the queue and the number after is a
tag between 0 and 255.
(10) Watches are removed if either the notification pipe is destroyed or
the watched object is destroyed. In the latter case, a message will
be generated indicating the enforced watch removal.
Things I want to avoid:
(1) Introducing features that make the core VFS dependent on the network
stack or networking namespaces (ie. usage of netlink).
(2) Dumping all this stuff into dmesg and having a daemon that sits there
parsing the output and distributing it as this then puts the
responsibility for security into userspace and makes handling
namespaces tricky. Further, dmesg might not exist or might be
inaccessible inside a container.
(3) Letting users see events they shouldn't be able to see.
Testing and manpages:
(*) The keyutils tree has a pipe-watch branch that has keyctl commands for
making use of notifications. Proposed manual pages can also be found
on this branch, though a couple of them really need to go to the main
manpages repository instead.
If the kernel supports the watching of keys, then running "make test"
on that branch will cause the testing infrastructure to spawn a
monitoring process on the side that monitors a notifications pipe for
all the key/keyring changes induced by the tests and they'll all be
checked off to make sure they happened.
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/keyutils.git/log/?h=pipe-watch
(*) A test program is provided (samples/watch_queue/watch_test) that can
be used to monitor for keyrings, mount and superblock events.
Information on the notifications is simply logged to stdout.
The kernel patches can also be found here:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=notifications-pipe-core
Changes:
ver #4:
(*) Dropped USB and device notifications for the moment as there's some
dispute over whether another avenue should be used for USB
notifications.
(*) Include mount and superblock event sources in the patchset.
- These now increment event counters that fsinfo() will be able to
retrieve (separate patch set).
ver #3:
(*) Rebase to after latest upstream pipe patches.
(*) Fix a missing ref get in add_watch_to_object().
ver #2:
(*) Declare O_NOTIFICATION_PIPE to use and switch it to be the same value
as O_EXCL rather then O_TMPFILE (the latter is a bit nasty in its
implementation).
ver #1:
(*) Build on top of standard pipes instead of having a driver.
David
---
David Howells (17):
uapi: General notification queue definitions
security: Add hooks to rule on setting a watch
security: Add a hook for the point of notification insertion
pipe: Add O_NOTIFICATION_PIPE
pipe: Add general notification queue support
watch_queue: Add a key/keyring notification facility
Add sample notification program
pipe: Allow buffers to be marked read-whole-or-error for notifications
pipe: Add notification lossage handling
selinux: Implement the watch_key security hook
smack: Implement the watch_key and post_notification hooks
watch_queue: Add security hooks to rule on setting mount and sb watches
watch_queue: Implement mount topology and attribute change notifications
watch_queue: sample: Display mount tree change notifications
watch_queue: Introduce a non-repeating system-unique superblock ID
watch_queue: Add superblock notifications
watch_queue: sample: Display superblock notifications
Documentation/security/keys/core.rst | 58 ++
Documentation/userspace-api/ioctl/ioctl-number.rst | 1
Documentation/watch_queue.rst | 354 +++++++++++
arch/alpha/kernel/syscalls/syscall.tbl | 2
arch/arm/tools/syscall.tbl | 2
arch/arm64/include/asm/unistd.h | 2
arch/ia64/kernel/syscalls/syscall.tbl | 2
arch/m68k/kernel/syscalls/syscall.tbl | 2
arch/microblaze/kernel/syscalls/syscall.tbl | 2
arch/mips/kernel/syscalls/syscall_n32.tbl | 2
arch/mips/kernel/syscalls/syscall_n64.tbl | 2
arch/mips/kernel/syscalls/syscall_o32.tbl | 2
arch/parisc/kernel/syscalls/syscall.tbl | 2
arch/powerpc/kernel/syscalls/syscall.tbl | 2
arch/s390/kernel/syscalls/syscall.tbl | 2
arch/sh/kernel/syscalls/syscall.tbl | 2
arch/sparc/kernel/syscalls/syscall.tbl | 2
arch/x86/entry/syscalls/syscall_32.tbl | 2
arch/x86/entry/syscalls/syscall_64.tbl | 2
arch/xtensa/kernel/syscalls/syscall.tbl | 2
fs/Kconfig | 21 +
fs/Makefile | 1
fs/internal.h | 1
fs/mount.h | 35 +
fs/mount_notify.c | 186 ++++++
fs/namespace.c | 22 +
fs/pipe.c | 242 +++++--
fs/splice.c | 12
fs/super.c | 156 +++++
include/linux/dcache.h | 1
include/linux/fs.h | 84 +++
include/linux/key.h | 3
include/linux/lsm_audit.h | 1
include/linux/lsm_hooks.h | 62 ++
include/linux/pipe_fs_i.h | 27 +
include/linux/security.h | 47 +
include/linux/syscalls.h | 4
include/linux/watch_queue.h | 127 ++++
include/uapi/asm-generic/unistd.h | 6
include/uapi/linux/keyctl.h | 2
include/uapi/linux/watch_queue.h | 163 +++++
init/Kconfig | 12
kernel/Makefile | 1
kernel/sys_ni.c | 6
kernel/watch_queue.c | 659 ++++++++++++++++++++
samples/Kconfig | 6
samples/Makefile | 1
samples/watch_queue/Makefile | 7
samples/watch_queue/watch_test.c | 260 ++++++++
security/keys/Kconfig | 9
security/keys/compat.c | 3
security/keys/gc.c | 5
security/keys/internal.h | 30 +
security/keys/key.c | 38 +
security/keys/keyctl.c | 99 +++
security/keys/keyring.c | 20 -
security/keys/request_key.c | 4
security/security.c | 37 +
security/selinux/hooks.c | 14
security/smack/smack_lsm.c | 83 ++-
60 files changed, 2837 insertions(+), 107 deletions(-)
create mode 100644 Documentation/watch_queue.rst
create mode 100644 fs/mount_notify.c
create mode 100644 include/linux/watch_queue.h
create mode 100644 include/uapi/linux/watch_queue.h
create mode 100644 kernel/watch_queue.c
create mode 100644 samples/watch_queue/Makefile
create mode 100644 samples/watch_queue/watch_test.c
^ permalink raw reply
* [PATCH v2 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Waiman Long @ 2020-03-08 17:04 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen, Waiman Long
In-Reply-To: <20200308170410.14166-1-longman@redhat.com>
By allocating a kernel buffer with an 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.
To reduce this possibility, we set a threshold (1024) over which we
do check the actual key length first before allocating a buffer of the
right size to hold it.
Signed-off-by: Waiman Long <longman@redhat.com>
---
security/keys/keyctl.c | 46 ++++++++++++++++++++++++++++++++----------
1 file changed, 35 insertions(+), 11 deletions(-)
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 89a14e71eb0a..662a638a680d 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -855,28 +855,52 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
* deadlock involving page fault and mmap_sem.
*/
char *tmpbuf = NULL;
+ size_t tbuflen = buflen;
- if (buffer && buflen) {
- tmpbuf = kmalloc(buflen, GFP_KERNEL);
+ /*
+ * We don't want an erronous -ENOMEM error due to an
+ * arbitrary large user-supplied buflen. So if buflen
+ * exceeds a threshold (1024 bytes in this case), we call
+ * the read method twice. The first time to get the buffer
+ * length and the second time to read out the key data.
+ *
+ * N.B. All the read methods will return the required
+ * buffer length with a NULL input buffer or when
+ * the input buffer length isn't large enough.
+ */
+ if (buflen && buffer && (buflen <= 0x400)) {
+allocbuf:
+ tmpbuf = kmalloc(tbuflen, GFP_KERNEL);
if (!tmpbuf) {
ret = -ENOMEM;
goto error2;
}
}
+
down_read(&key->sem);
ret = key_validate(key);
if (ret == 0)
- ret = key->type->read(key, tmpbuf, buflen);
+ ret = key->type->read(key, tmpbuf, tbuflen);
up_read(&key->sem);
- /*
- * Read methods will just return the required length
- * without any copying if the provided length isn't big
- * enough.
- */
- if ((ret > 0) && (ret <= buflen) && buffer &&
- copy_to_user(buffer, tmpbuf, ret))
- ret = -EFAULT;
+ if ((ret > 0) && (ret <= buflen) && buffer) {
+ /*
+ * It is possible, though unlikely, that the key
+ * changes in between the up_read->down_read period.
+ * If the key becomes longer, we will have to
+ * allocate a larger buffer and redo the key read
+ * again.
+ */
+ if (!tmpbuf || unlikely(ret > tbuflen)) {
+ tbuflen = ret;
+ if (unlikely(tmpbuf))
+ kzfree(tmpbuf);
+ goto allocbuf;
+ }
+
+ if (copy_to_user(buffer, tmpbuf, ret))
+ ret = -EFAULT;
+ }
if (tmpbuf)
kzfree(tmpbuf);
--
2.18.1
^ permalink raw reply related
* [PATCH v2 1/2] KEYS: Don't write out to userspace while holding key semaphore
From: Waiman Long @ 2020-03-08 17:04 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen, Waiman Long
In-Reply-To: <20200308170410.14166-1-longman@redhat.com>
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/0xdf
One 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 the read methods as
well as additional changes to not use any userspace write helpers.
Signed-off-by: Waiman Long <longman@redhat.com>
---
include/linux/key-type.h | 2 +-
security/keys/big_key.c | 11 +++------
security/keys/encrypted-keys/encrypted.c | 7 +++---
security/keys/keyctl.c | 30 +++++++++++++++++++++--
security/keys/keyring.c | 6 +----
security/keys/request_key_auth.c | 7 +++---
security/keys/trusted-keys/trusted_tpm1.c | 14 ++---------
security/keys/user_defined.c | 5 ++--
8 files changed, 44 insertions(+), 38 deletions(-)
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index 4ded94bcf274..2ab2d6d6aeab 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -127,7 +127,7 @@ struct key_type {
* much is copied into the buffer
* - shouldn't do the copy if the buffer is NULL
*/
- long (*read)(const struct key *key, char __user *buffer, size_t buflen);
+ long (*read)(const struct key *key, char *buffer, size_t buflen);
/* handle request_key() for this type instead of invoking
* /sbin/request-key (optional)
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 001abe530a0d..82008f900930 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -352,7 +352,7 @@ void big_key_describe(const struct key *key, struct seq_file *m)
* read the key data
* - the key's semaphore is read-locked
*/
-long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
+long big_key_read(const struct key *key, char *buffer, size_t buflen)
{
size_t datalen = (size_t)key->payload.data[big_key_len];
long ret;
@@ -391,9 +391,8 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
ret = datalen;
- /* copy decrypted data to user */
- if (copy_to_user(buffer, buf->virt, datalen) != 0)
- ret = -EFAULT;
+ /* copy out decrypted data */
+ memcpy(buffer, buf->virt, datalen);
err_fput:
fput(file);
@@ -401,9 +400,7 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
big_key_free_buffer(buf);
} else {
ret = datalen;
- if (copy_to_user(buffer, key->payload.data[big_key_data],
- datalen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, key->payload.data[big_key_data], datalen);
}
return ret;
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 60720f58cbe0..f6797ba44bf7 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -902,14 +902,14 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
}
/*
- * encrypted_read - format and copy the encrypted data to userspace
+ * encrypted_read - format and copy out the encrypted data
*
* The resulting datablob format is:
* <master-key name> <decrypted data length> <encrypted iv> <encrypted data>
*
* On success, return to userspace the encrypted key datablob size.
*/
-static long encrypted_read(const struct key *key, char __user *buffer,
+static long encrypted_read(const struct key *key, char *buffer,
size_t buflen)
{
struct encrypted_key_payload *epayload;
@@ -957,8 +957,7 @@ static long encrypted_read(const struct key *key, char __user *buffer,
key_put(mkey);
memzero_explicit(derived_key, sizeof(derived_key));
- if (copy_to_user(buffer, ascii_buf, asciiblob_len) != 0)
- ret = -EFAULT;
+ memcpy(buffer, ascii_buf, asciiblob_len);
kzfree(ascii_buf);
return asciiblob_len;
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 9b898c969558..89a14e71eb0a 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -846,14 +846,40 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
can_read_key:
ret = -EOPNOTSUPP;
if (key->type->read) {
- /* Read the data with the semaphore held (since we might sleep)
+ /*
+ * Read the data with the semaphore held (since we might sleep)
* to protect against the key being updated or revoked.
+ *
+ * Allocating a temporary buffer to hold the keys before
+ * transferring them to user buffer to avoid potential
+ * deadlock involving page fault and mmap_sem.
*/
+ char *tmpbuf = NULL;
+
+ if (buffer && buflen) {
+ tmpbuf = kmalloc(buflen, GFP_KERNEL);
+ if (!tmpbuf) {
+ ret = -ENOMEM;
+ goto error2;
+ }
+ }
down_read(&key->sem);
ret = key_validate(key);
if (ret == 0)
- ret = key->type->read(key, buffer, buflen);
+ ret = key->type->read(key, tmpbuf, buflen);
up_read(&key->sem);
+
+ /*
+ * Read methods will just return the required length
+ * without any copying if the provided length isn't big
+ * enough.
+ */
+ if ((ret > 0) && (ret <= buflen) && buffer &&
+ copy_to_user(buffer, tmpbuf, ret))
+ ret = -EFAULT;
+
+ if (tmpbuf)
+ kzfree(tmpbuf);
}
error2:
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index febf36c6ddc5..5ca620d31cd3 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -459,7 +459,6 @@ static int keyring_read_iterator(const void *object, void *data)
{
struct keyring_read_iterator_context *ctx = data;
const struct key *key = keyring_ptr_to_key(object);
- int ret;
kenter("{%s,%d},,{%zu/%zu}",
key->type->name, key->serial, ctx->count, ctx->buflen);
@@ -467,10 +466,7 @@ static int keyring_read_iterator(const void *object, void *data)
if (ctx->count >= ctx->buflen)
return 1;
- ret = put_user(key->serial, ctx->buffer);
- if (ret < 0)
- return ret;
- ctx->buffer++;
+ *ctx->buffer++ = key->serial;
ctx->count += sizeof(key->serial);
return 0;
}
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index ecba39c93fd9..41e9735006d0 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -22,7 +22,7 @@ static int request_key_auth_instantiate(struct key *,
static void request_key_auth_describe(const struct key *, struct seq_file *);
static void request_key_auth_revoke(struct key *);
static void request_key_auth_destroy(struct key *);
-static long request_key_auth_read(const struct key *, char __user *, size_t);
+static long request_key_auth_read(const struct key *, char *, size_t);
/*
* The request-key authorisation key type definition.
@@ -80,7 +80,7 @@ static void request_key_auth_describe(const struct key *key,
* - the key's semaphore is read-locked
*/
static long request_key_auth_read(const struct key *key,
- char __user *buffer, size_t buflen)
+ char *buffer, size_t buflen)
{
struct request_key_auth *rka = dereference_key_locked(key);
size_t datalen;
@@ -97,8 +97,7 @@ static long request_key_auth_read(const struct key *key,
if (buflen > datalen)
buflen = datalen;
- if (copy_to_user(buffer, rka->callout_info, buflen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, rka->callout_info, buflen);
}
return ret;
diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
index d2c5ec1e040b..8001ab07e63b 100644
--- a/security/keys/trusted-keys/trusted_tpm1.c
+++ b/security/keys/trusted-keys/trusted_tpm1.c
@@ -1130,11 +1130,10 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
* trusted_read - copy the sealed blob data to userspace in hex.
* On success, return to userspace the trusted key datablob size.
*/
-static long trusted_read(const struct key *key, char __user *buffer,
+static long trusted_read(const struct key *key, char *buffer,
size_t buflen)
{
const struct trusted_key_payload *p;
- char *ascii_buf;
char *bufp;
int i;
@@ -1143,18 +1142,9 @@ static long trusted_read(const struct key *key, char __user *buffer,
return -EINVAL;
if (buffer && buflen >= 2 * p->blob_len) {
- ascii_buf = kmalloc_array(2, p->blob_len, GFP_KERNEL);
- if (!ascii_buf)
- return -ENOMEM;
-
- bufp = ascii_buf;
+ bufp = buffer;
for (i = 0; i < p->blob_len; i++)
bufp = hex_byte_pack(bufp, p->blob[i]);
- if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) {
- kzfree(ascii_buf);
- return -EFAULT;
- }
- kzfree(ascii_buf);
}
return 2 * p->blob_len;
}
diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 6f12de4ce549..07d4287e9084 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -168,7 +168,7 @@ EXPORT_SYMBOL_GPL(user_describe);
* read the key data
* - the key's semaphore is read-locked
*/
-long user_read(const struct key *key, char __user *buffer, size_t buflen)
+long user_read(const struct key *key, char *buffer, size_t buflen)
{
const struct user_key_payload *upayload;
long ret;
@@ -181,8 +181,7 @@ long user_read(const struct key *key, char __user *buffer, size_t buflen)
if (buflen > upayload->datalen)
buflen = upayload->datalen;
- if (copy_to_user(buffer, upayload->data, buflen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, upayload->data, buflen);
}
return ret;
--
2.18.1
^ permalink raw reply related
* [PATCH v2 0/2] KEYS: Read keys to internal buffer & then copy to userspace
From: Waiman Long @ 2020-03-08 17:04 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen, Waiman Long
v2:
- Handle NULL buffer and buflen properly in patch 1.
- Fix a bug in big_key.c.
- Add patch 2 to handle arbitrary large user-supplied buflen.
The current security key read methods are called with the key semaphore
held. The methods then copy out the key data to userspace which is
subjected to page fault and may acquire the mmap semaphore. That can
result in circular lock dependency and hence a chance to get into
deadlock.
To avoid such a deadlock, an internal buffer is now allocated for getting
out the necessary data first. After releasing the key semaphore, the
key data are then copied out to userspace sidestepping the circular
lock dependency.
The keyutils test suite was run and the test passed with these patchset
applied without any falure.
Waiman Long (2):
KEYS: Don't write out to userspace while holding key semaphore
KEYS: Avoid false positive ENOMEM error on key read
include/linux/key-type.h | 2 +-
security/keys/big_key.c | 11 ++---
security/keys/encrypted-keys/encrypted.c | 7 ++-
security/keys/keyctl.c | 54 ++++++++++++++++++++++-
security/keys/keyring.c | 6 +--
security/keys/request_key_auth.c | 7 ++-
security/keys/trusted-keys/trusted_tpm1.c | 14 +-----
security/keys/user_defined.c | 5 +--
8 files changed, 68 insertions(+), 38 deletions(-)
--
2.18.1
^ permalink raw reply
* Re: [PATCH v15 18/23] NET: Store LSM netlabel data in a lsmblob
From: Paul Moore @ 2020-03-07 2:14 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
keescook, john.johansen, penguin-kernel, Stephen Smalley
In-Reply-To: <20200214234203.7086-19-casey@schaufler-ca.com>
On Fri, Feb 14, 2020 at 6:45 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Netlabel uses LSM interfaces requiring an lsmblob and
> the internal storage is used to pass information between
> these interfaces, so change the internal data from a secid
> to a lsmblob. Update the netlabel interfaces and their
> callers to accommodate the change. This requires that the
> modules using netlabel use the lsm_id.slot to access the
> correct secid when using netlabel.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/net/netlabel.h | 8 ++--
> net/ipv4/cipso_ipv4.c | 23 +++++++-----
> net/netlabel/netlabel_kapi.c | 6 +--
> net/netlabel/netlabel_unlabeled.c | 57 +++++++++++------------------
> net/netlabel/netlabel_unlabeled.h | 2 +-
> security/selinux/hooks.c | 2 +-
> security/selinux/include/security.h | 1 +
> security/selinux/netlabel.c | 2 +-
> security/selinux/ss/services.c | 4 +-
> security/smack/smack.h | 1 +
> security/smack/smack_lsm.c | 5 ++-
> security/smack/smackfs.c | 10 +++--
> 12 files changed, 59 insertions(+), 62 deletions(-)
...
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 376882215919..adb9dffc3952 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -106,15 +106,17 @@ int cipso_v4_rbm_strictvalid = 1;
> /* Base length of the local tag (non-standard tag).
> * Tag definition (may change between kernel versions)
> *
> - * 0 8 16 24 32
> - * +----------+----------+----------+----------+
> - * | 10000000 | 00000110 | 32-bit secid value |
> - * +----------+----------+----------+----------+
> - * | in (host byte order)|
> - * +----------+----------+
> - *
> + * 0 8 16 16 + sizeof(struct lsmblob)
> + * +----------+----------+---------------------+
> + * | 10000000 | 00000110 | LSM blob data |
> + * +----------+----------+---------------------+
> + *
> + * All secid and flag fields are in host byte order.
> + * The lsmblob structure size varies depending on which
> + * Linux security modules are built in the kernel.
> + * The data is opaque.
> */
> -#define CIPSO_V4_TAG_LOC_BLEN 6
> +#define CIPSO_V4_TAG_LOC_BLEN (2 + sizeof(struct lsmblob))
This isn't as bad as the sk_buff.cb limitation so I'm not going to
worry too much about it, but just to be safe I think we should put a
compile-time check to ensure that the local tag is within the bounds
of the IPv4 option limit. If we don't put a check I worry that there
is a chance someone could get a very rude surprise at some point in
the future (yes, this is highly unlikely, but still possible).
> /*
> * Helper Functions
> @@ -1467,7 +1469,8 @@ static int cipso_v4_gentag_loc(const struct cipso_v4_doi *doi_def,
>
> buffer[0] = CIPSO_V4_TAG_LOCAL;
> buffer[1] = CIPSO_V4_TAG_LOC_BLEN;
> - *(u32 *)&buffer[2] = secattr->attr.secid;
> + memcpy(&buffer[2], &secattr->attr.lsmblob,
> + sizeof(secattr->attr.lsmblob));
>
> return CIPSO_V4_TAG_LOC_BLEN;
> }
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v15 17/23] LSM: security_secid_to_secctx in netlink netfilter
From: Paul Moore @ 2020-03-07 2:06 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
keescook, john.johansen, penguin-kernel, Stephen Smalley
In-Reply-To: <20200214234203.7086-18-casey@schaufler-ca.com>
On Fri, Feb 14, 2020 at 6:44 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> Change netlink netfilter interfaces to use lsmcontext
> pointers, and remove scaffolding.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: netdev@vger.kernel.org
> ---
> net/netfilter/nfnetlink_queue.c | 32 +++++++++++++-------------------
> 1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 7e91f3c10b29..e0512e91fed9 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -629,10 +627,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> }
>
> nlh->nlmsg_len = skb->len;
> - if (seclen) {
> - lsmcontext_init(&scaff, secdata, seclen, 0);
> - security_release_secctx(&scaff);
> - }
> + if (seclen)
> + security_release_secctx(&context);
> return skb;
>
> nla_put_failure:
> @@ -640,10 +636,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> kfree_skb(skb);
> net_err_ratelimited("nf_queue: error creating packet message\n");
> nlmsg_failure:
> - if (seclen) {
> - lsmcontext_init(&scaff, secdata, seclen, 0);
> - security_release_secctx(&scaff);
> - }
> + if (seclen)
> + security_release_secctx(&context);
> return NULL;
> }
It feels like we should be converting nfqnl_build_packet_message() and
nfqnl_get_sk_secctx() as well, but perhaps that is coming in a later
patch in the patchset (although there are only six left).
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v15 13/23] LSM: Specify which LSM to display
From: Paul Moore @ 2020-03-07 1:49 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
keescook, john.johansen, penguin-kernel, Stephen Smalley
In-Reply-To: <20200214234203.7086-14-casey@schaufler-ca.com>
On Fri, Feb 14, 2020 at 6:44 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Create a new entry "display" in the procfs attr directory for
> controlling which LSM security information is displayed for a
> process. A process can only read or write its own display value.
>
> The name of an active LSM that supplies hooks for
> human readable data may be written to "display" to set the
> value. The name of the LSM currently in use can be read from
> "display". At this point there can only be one LSM capable
> of display active. A helper function lsm_task_display() is
> provided to get the display slot for a task_struct.
>
> Setting the "display" requires that all security modules using
> setprocattr hooks allow the action. Each security module is
> responsible for defining its policy.
>
> AppArmor hook provided by John Johansen <john.johansen@canonical.com>
> SELinux hook provided by Stephen Smalley <sds@tycho.nsa.gov>
>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> fs/proc/base.c | 1 +
> include/linux/lsm_hooks.h | 15 +++
> security/apparmor/include/apparmor.h | 3 +-
> security/apparmor/lsm.c | 32 +++++
> security/security.c | 167 ++++++++++++++++++++++++---
> security/selinux/hooks.c | 11 ++
> security/selinux/include/classmap.h | 2 +-
> security/smack/smack_lsm.c | 7 ++
> 8 files changed, 219 insertions(+), 19 deletions(-)
For the core and SELinux portions.
Acked-by: Paul Moore <paul@paul-moore.com>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v15 06/23] Use lsmblob in security_secctx_to_secid
From: Paul Moore @ 2020-03-07 0:58 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
keescook, john.johansen, penguin-kernel, Stephen Smalley
In-Reply-To: <20200214234203.7086-7-casey@schaufler-ca.com>
On Fri, Feb 14, 2020 at 6:43 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Change security_secctx_to_secid() to fill in a lsmblob instead
> of a u32 secid. Multiple LSMs may be able to interpret the
> string, and this allows for setting whichever secid is
> appropriate. In some cases there is scaffolding where other
> interfaces have yet to be converted.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/security.h | 5 +++--
> kernel/cred.c | 4 +---
> net/netfilter/nft_meta.c | 12 +++++++-----
> net/netfilter/xt_SECMARK.c | 5 ++++-
> net/netlabel/netlabel_unlabeled.c | 14 ++++++++------
> security/security.c | 18 +++++++++++++++---
> 6 files changed, 38 insertions(+), 20 deletions(-)
...
> diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
> index 951b6e87ed5d..e12125b85035 100644
> --- a/net/netfilter/nft_meta.c
> +++ b/net/netfilter/nft_meta.c
> @@ -811,21 +811,23 @@ static const struct nla_policy nft_secmark_policy[NFTA_SECMARK_MAX + 1] = {
>
> static int nft_secmark_compute_secid(struct nft_secmark *priv)
> {
> - u32 tmp_secid = 0;
> + struct lsmblob blob;
> int err;
>
> - err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &tmp_secid);
> + err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &blob);
> if (err)
> return err;
>
> - if (!tmp_secid)
> + if (!lsmblob_is_set(&blob))
> return -ENOENT;
>
> - err = security_secmark_relabel_packet(tmp_secid);
> + /* Using le[0] is scaffolding */
> + err = security_secmark_relabel_packet(blob.secid[0]);
> if (err)
> return err;
At the very least it looks like the comment above needs an update.
However, I would really like to see an explanation in this patch,
either in the comments or in the commit description, about how you
plan to handle secmarks. If your plan is to always have it be the
first LSM, let's admit that and document it appropriately. If there
is something much grander coming later in the patchset I guess
"scaffolding" is an okay term, but it would be good to mention in the
commit description that this will be replaced with something better
later in the patchset.
I'm worried about the case five years from know when we are changing
this code, either due to bugs or new features, and we stumble across
this commit. Was it always intended to be this way? Or was this
temporary? Right now I don't know.
> - priv->secid = tmp_secid;
> + /* Using le[0] is scaffolding */
> + priv->secid = blob.secid[0];
> return 0;
> }
...
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index d2e4ab8d1cb1..7a5a87f15736 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -881,7 +881,7 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb,
> void *addr;
> void *mask;
> u32 addr_len;
> - u32 secid;
> + struct lsmblob blob;
> struct netlbl_audit audit_info;
>
> /* Don't allow users to add both IPv4 and IPv6 addresses for a
> @@ -905,12 +905,13 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb,
> ret_val = security_secctx_to_secid(
> nla_data(info->attrs[NLBL_UNLABEL_A_SECCTX]),
> nla_len(info->attrs[NLBL_UNLABEL_A_SECCTX]),
> - &secid);
> + &blob);
> if (ret_val != 0)
> return ret_val;
>
> + /* scaffolding with the [0] */
> return netlbl_unlhsh_add(&init_net,
> - dev_name, addr, mask, addr_len, secid,
> + dev_name, addr, mask, addr_len, blob.secid[0],
> &audit_info);
> }
Same as above, although this time with the peer label.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v15 05/23] net: Prepare UDS for security module stacking
From: Paul Moore @ 2020-03-06 22:14 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
keescook, john.johansen, penguin-kernel, Stephen Smalley
In-Reply-To: <20200214234203.7086-6-casey@schaufler-ca.com>
On Fri, Feb 14, 2020 at 6:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Change the data used in UDS SO_PEERSEC processing from a
> secid to a more general struct lsmblob. Update the
> security_socket_getpeersec_dgram() interface to use the
> lsmblob. There is a small amount of scaffolding code
> that will come out when the security_secid_to_secctx()
> code is brought in line with the lsmblob.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: netdev@vger.kernel.org
> ---
> include/linux/security.h | 7 +++++--
> include/net/af_unix.h | 2 +-
> include/net/scm.h | 8 +++++---
> net/ipv4/ip_sockglue.c | 8 +++++---
> net/unix/af_unix.c | 6 +++---
> security/security.c | 18 +++++++++++++++---
> 6 files changed, 34 insertions(+), 15 deletions(-)
...
> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index 17e10fba2152..59af08ca802f 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -36,7 +36,7 @@ struct unix_skb_parms {
> kgid_t gid;
> struct scm_fp_list *fp; /* Passed files */
> #ifdef CONFIG_SECURITY_NETWORK
> - u32 secid; /* Security ID */
> + struct lsmblob lsmblob; /* Security LSM data */
> #endif
> u32 consumed;
> } __randomize_layout;
This might be a problem. As it currently stands, the sk_buff.cb field
is 48 bytes; with CONFIG_SECURITY_NETWORK=n unix_skb_parms is 28 bytes
on a 64-bit system. That leaves 20 bytes (room for 5 LSMs) assuming a
tight packing *and* that netdev doesn't swoop in and drop another few
fields in unix_skb_parms.
This may work now, and you might manage to sneak this by the netdev
crowd, but I predict problems in the future.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v15 04/23] LSM: Use lsmblob in security_kernel_act_as
From: Paul Moore @ 2020-03-06 22:06 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
keescook, john.johansen, penguin-kernel, Stephen Smalley
In-Reply-To: <20200214234203.7086-5-casey@schaufler-ca.com>
On Fri, Feb 14, 2020 at 6:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Change the security_kernel_act_as interface to use a lsmblob
> structure in place of the single u32 secid in support of
> module stacking. Change its only caller, set_security_override,
> to do the same. Change that one's only caller,
> set_security_override_from_ctx, to call it with the new
> parameter type.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/cred.h | 3 ++-
> include/linux/security.h | 5 +++--
> kernel/cred.c | 10 ++++++----
> security/security.c | 14 ++++++++++++--
> 4 files changed, 23 insertions(+), 9 deletions(-)
...
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 9d83c6832bae..1bfaf9ece13e 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -439,7 +439,7 @@ void security_cred_free(struct cred *cred);
> int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
> void security_transfer_creds(struct cred *new, const struct cred *old);
> void security_cred_getsecid(const struct cred *c, u32 *secid);
> -int security_kernel_act_as(struct cred *new, u32 secid);
> +int security_kernel_act_as(struct cred *new, struct lsmblob *blob);
> int security_kernel_create_files_as(struct cred *new, struct inode *inode);
> int security_kernel_module_request(char *kmod_name);
> int security_kernel_load_data(enum kernel_load_data_id id);
> @@ -1045,7 +1045,8 @@ static inline void security_transfer_creds(struct cred *new,
> {
> }
>
> -static inline int security_kernel_act_as(struct cred *cred, u32 secid)
> +static inline int security_kernel_act_as(struct cred *cred,
> + struct lsmblob *blob)
> {
> return 0;
> }
> diff --git a/kernel/cred.c b/kernel/cred.c
> index 809a985b1793..fae63c0a1b36 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -732,14 +732,14 @@ EXPORT_SYMBOL(prepare_kernel_cred);
> /**
> * set_security_override - Set the security ID in a set of credentials
> * @new: The credentials to alter
> - * @secid: The LSM security ID to set
> + * @blob: The LSM security information to set
> *
> * Set the LSM security ID in a set of credentials so that the subjective
> * security is overridden when an alternative set of credentials is used.
> */
> -int set_security_override(struct cred *new, u32 secid)
> +int set_security_override(struct cred *new, struct lsmblob *blob)
> {
> - return security_kernel_act_as(new, secid);
> + return security_kernel_act_as(new, blob);
> }
> EXPORT_SYMBOL(set_security_override);
>
> @@ -755,6 +755,7 @@ EXPORT_SYMBOL(set_security_override);
> */
> int set_security_override_from_ctx(struct cred *new, const char *secctx)
> {
> + struct lsmblob blob;
> u32 secid;
> int ret;
>
> @@ -762,7 +763,8 @@ int set_security_override_from_ctx(struct cred *new, const char *secctx)
> if (ret < 0)
> return ret;
>
> - return set_security_override(new, secid);
> + lsmblob_init(&blob, secid);
> + return set_security_override(new, &blob);
See my comments in 03/23, this is another case where the value of
lsmblob is not yet clear to me, but perhaps it will make sense later
in the patchset.
> }
> EXPORT_SYMBOL(set_security_override_from_ctx);
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v15 01/23] LSM: Infrastructure management of the sock security
From: Paul Moore @ 2020-03-06 20:37 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
keescook, john.johansen, penguin-kernel, Stephen Smalley
In-Reply-To: <20200214234203.7086-2-casey@schaufler-ca.com>
On Fri, Feb 14, 2020 at 6:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Move management of the sock->sk_security blob out
> of the individual security modules and into the security
> infrastructure. Instead of allocating the blobs from within
> the modules the modules tell the infrastructure how much
> space is required, and the space is allocated there.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/lsm_hooks.h | 1 +
> security/apparmor/include/net.h | 6 ++-
> security/apparmor/lsm.c | 38 ++++-----------
> security/security.c | 36 +++++++++++++-
> security/selinux/hooks.c | 78 +++++++++++++++----------------
> security/selinux/include/objsec.h | 5 ++
> security/selinux/netlabel.c | 23 ++++-----
> security/smack/smack.h | 5 ++
> security/smack/smack_lsm.c | 64 ++++++++++++-------------
> security/smack/smack_netfilter.c | 8 ++--
> 10 files changed, 144 insertions(+), 120 deletions(-)
Stephen already ACK'd this patch, but more acks/reviews are always
better so you can add my ACK as well.
Acked-by: Paul Moore <paul@paul-moore.com>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v28 11/22] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2020-03-06 19:00 UTC (permalink / raw)
To: Jethro Beekman
Cc: Sean Christopherson, linux-kernel, x86, linux-sgx, akpm,
dave.hansen, nhorman, npmccallum, haitao.huang, andriy.shevchenko,
tglx, kai.svahn, bp, josh, luto, kai.huang, rientjes, cedric.xing,
puiterwijk, linux-security-module, Suresh Siddha, Haitao Huang,
Chunyang Hui
In-Reply-To: <e445c2f8-fddd-5740-0e26-a03e023918bf@fortanix.com>
On Thu, Mar 05, 2020 at 07:24:11PM +0100, Jethro Beekman wrote:
> On 2020-03-05 18:40, Sean Christopherson wrote:
> > On Wed, Mar 04, 2020 at 01:35:58AM +0200, Jarkko Sakkinen wrote:
> >> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> >> new file mode 100644
> >> index 000000000000..5edb08ab8fd0
> >> --- /dev/null
> >> +++ b/arch/x86/include/uapi/asm/sgx.h
> >> @@ -0,0 +1,66 @@
> >> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */
> >> +/*
> >> + * Copyright(c) 2016-19 Intel Corporation.
> >> + */
> >> +#ifndef _UAPI_ASM_X86_SGX_H
> >> +#define _UAPI_ASM_X86_SGX_H
> >> +
> >> +#include <linux/types.h>
> >> +#include <linux/ioctl.h>
> >> +
> >> +/**
> >> + * enum sgx_epage_flags - page control flags
> >> + * %SGX_PAGE_MEASURE: Measure the page contents with a sequence of
> >> + * ENCLS[EEXTEND] operations.
> >> + */
> >> +enum sgx_page_flags {
> >> + SGX_PAGE_MEASURE = 0x01,
> >> +};
> >> +
> >> +#define SGX_MAGIC 0xA4
> >> +
> >> +#define SGX_IOC_ENCLAVE_CREATE \
> >> + _IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create)
> >> +#define SGX_IOC_ENCLAVE_ADD_PAGES \
> >> + _IOWR(SGX_MAGIC, 0x01, struct sgx_enclave_add_pages)
> >> +#define SGX_IOC_ENCLAVE_INIT \
> >> + _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> >> +
> >> +/**
> >> + * struct sgx_enclave_create - parameter structure for the
> >> + * %SGX_IOC_ENCLAVE_CREATE ioctl
> >> + * @src: address for the SECS page data
> >> + */
> >> +struct sgx_enclave_create {
> >> + __u64 src;
> >
> > Would it make sense to add reserved fields to the structs so that new
> > features can be added in a backwards compatible way? E.g. if we want to
> > allow userspace to control the backing store by passing in a file
> > descriptor ENCLAVE_CREATE.
>
> Reserving space for future fields is not necessary because the size of
> the struct is encoded in the ioctl number. The kernel can use this to
> differentiate between different call versions from userspace.
Sure but I still would never change the signature once a ioctl is added.
/Jarkko
^ permalink raw reply
* Re: [PATCH v28 11/22] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2020-03-06 18:58 UTC (permalink / raw)
To: Sean Christopherson
Cc: linux-kernel, x86, linux-sgx, akpm, dave.hansen, nhorman,
npmccallum, haitao.huang, andriy.shevchenko, tglx, kai.svahn, bp,
josh, luto, kai.huang, rientjes, cedric.xing, puiterwijk,
linux-security-module, Suresh Siddha, Haitao Huang,
Jethro Beekman, Chunyang Hui
In-Reply-To: <20200305174015.GJ11500@linux.intel.com>
On Thu, Mar 05, 2020 at 09:40:15AM -0800, Sean Christopherson wrote:
> Would it make sense to add reserved fields to the structs so that new
> features can be added in a backwards compatible way? E.g. if we want to
> allow userspace to control the backing store by passing in a file
> descriptor ENCLAVE_CREATE.
It would not really be a huge win even if you did that since old
software would not be aware of the new fields. Sounds somewhat
messy to me.
Even a new ioctl later on is in my opinion always a better option
than reserved fields when you add ioctl's.
/Jarkko
^ permalink raw reply
* Re: [PATCH] KEYS: Don't write out to userspace while holding key semaphore
From: Jarkko Sakkinen @ 2020-03-06 18:40 UTC (permalink / raw)
To: Waiman Long
Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
keyrings, linux-kernel, linux-security-module, linux-integrity,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200305210640.15315-1-longman@redhat.com>
On Thu, Mar 05, 2020 at 04:06:40PM -0500, Waiman Long wrote:
> 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/0xdf
>
> One 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 the read methods as
> well as additional changes to not use any userspace write helpers.
>
> Signed-off-by: Waiman Long <longman@redhat.com>
Hi, quickly checked this (it is Friday night in Finland) and did not
see anything obvious that'd be wrong. I'll give a more detailed look
next week.
Thank you.
/Jarkko
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: Fix bpf_prog_test_run_tracing for !CONFIG_NET
From: KP Singh @ 2020-03-05 23:22 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Randy Dunlap, LSM List, Linux-Next Mailing List, bpf,
Alexei Starovoitov, Daniel Borkmann, Stephen Rothwell,
Florent Revest, Brendan Jackman
In-Reply-To: <CAADnVQLjj+eMMLU3H4oNkzwPiSugm1knzd3RfBGb3NcVC785kg@mail.gmail.com>
On Fri, Mar 6, 2020 at 12:16 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Mar 5, 2020 at 3:12 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> >
> > On 3/5/20 2:01 PM, KP Singh wrote:
> > > From: KP Singh <kpsingh@google.com>
> > >
> > > test_run.o is not built when CONFIG_NET is not set and
> > > bpf_prog_test_run_tracing being referenced in bpf_trace.o causes the
> > > linker error:
> > >
> > > ld: kernel/trace/bpf_trace.o:(.rodata+0x38): undefined reference to
> > > `bpf_prog_test_run_tracing'
> > >
> > > Add a __weak function in bpf_trace.c to handle this.
> > >
> > > Fixes: da00d2f117a0 ("bpf: Add test ops for BPF_PROG_TYPE_TRACING")
> > > Signed-off-by: KP Singh <kpsingh@google.com>
> >
> > Reported-by: Randy Dunlap <rdunlap@infradead.org>
Thanks!
Apologies, I should have added the "Reported-by:" tag.
- KP
> > Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
>
> Since it was at the top of the tree I amended the commit
> with your tags.
> Thanks for reporting and testing.
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: Fix bpf_prog_test_run_tracing for !CONFIG_NET
From: Alexei Starovoitov @ 2020-03-05 23:16 UTC (permalink / raw)
To: Randy Dunlap
Cc: KP Singh, LSM List, Linux-Next Mailing List, bpf,
Alexei Starovoitov, Daniel Borkmann, Stephen Rothwell,
Florent Revest, Brendan Jackman
In-Reply-To: <92937298-69c1-be6f-3e40-75af1bc72d9e@infradead.org>
On Thu, Mar 5, 2020 at 3:12 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> On 3/5/20 2:01 PM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> >
> > test_run.o is not built when CONFIG_NET is not set and
> > bpf_prog_test_run_tracing being referenced in bpf_trace.o causes the
> > linker error:
> >
> > ld: kernel/trace/bpf_trace.o:(.rodata+0x38): undefined reference to
> > `bpf_prog_test_run_tracing'
> >
> > Add a __weak function in bpf_trace.c to handle this.
> >
> > Fixes: da00d2f117a0 ("bpf: Add test ops for BPF_PROG_TYPE_TRACING")
> > Signed-off-by: KP Singh <kpsingh@google.com>
>
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
Since it was at the top of the tree I amended the commit
with your tags.
Thanks for reporting and testing.
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: Fix bpf_prog_test_run_tracing for !CONFIG_NET
From: Randy Dunlap @ 2020-03-05 23:12 UTC (permalink / raw)
To: KP Singh, linux-security-module, linux-next, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Stephen Rothwell,
Florent Revest, Brendan Jackman
In-Reply-To: <20200305220127.29109-1-kpsingh@chromium.org>
On 3/5/20 2:01 PM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
>
> test_run.o is not built when CONFIG_NET is not set and
> bpf_prog_test_run_tracing being referenced in bpf_trace.o causes the
> linker error:
>
> ld: kernel/trace/bpf_trace.o:(.rodata+0x38): undefined reference to
> `bpf_prog_test_run_tracing'
>
> Add a __weak function in bpf_trace.c to handle this.
>
> Fixes: da00d2f117a0 ("bpf: Add test ops for BPF_PROG_TYPE_TRACING")
> Signed-off-by: KP Singh <kpsingh@google.com>
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
Thanks.
> ---
> kernel/trace/bpf_trace.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 363e0a2c75cf..6a490d8ce9de 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1252,6 +1252,13 @@ static bool tracing_prog_is_valid_access(int off, int size,
> return btf_ctx_access(off, size, type, prog, info);
> }
>
> +int __weak bpf_prog_test_run_tracing(struct bpf_prog *prog,
> + const union bpf_attr *kattr,
> + union bpf_attr __user *uattr)
> +{
> + return -ENOTSUPP;
> +}
> +
> const struct bpf_verifier_ops raw_tracepoint_verifier_ops = {
> .get_func_proto = raw_tp_prog_func_proto,
> .is_valid_access = raw_tp_prog_is_valid_access,
>
--
~Randy
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: Fix bpf_prog_test_run_tracing for !CONFIG_NET
From: Alexei Starovoitov @ 2020-03-05 22:43 UTC (permalink / raw)
To: KP Singh
Cc: linux-security-module, linux-next, bpf, Alexei Starovoitov,
Daniel Borkmann, Randy Dunlap, Stephen Rothwell, Florent Revest,
Brendan Jackman
In-Reply-To: <20200305220127.29109-1-kpsingh@chromium.org>
On Thu, Mar 05, 2020 at 11:01:27PM +0100, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
>
> test_run.o is not built when CONFIG_NET is not set and
> bpf_prog_test_run_tracing being referenced in bpf_trace.o causes the
> linker error:
>
> ld: kernel/trace/bpf_trace.o:(.rodata+0x38): undefined reference to
> `bpf_prog_test_run_tracing'
>
> Add a __weak function in bpf_trace.c to handle this.
>
> Fixes: da00d2f117a0 ("bpf: Add test ops for BPF_PROG_TYPE_TRACING")
> Signed-off-by: KP Singh <kpsingh@google.com>
Applied. Thanks
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: Remove unnecessary CAP_MAC_ADMIN check
From: Alexei Starovoitov @ 2020-03-05 22:38 UTC (permalink / raw)
To: KP Singh
Cc: linux-security-module, linux-kernel, bpf, Alexei Starovoitov,
Daniel Borkmann, Paul Turner, Jann Horn, Stephen Smalley,
Florent Revest, Brendan Jackman
In-Reply-To: <20200305204955.31123-1-kpsingh@chromium.org>
On Thu, Mar 05, 2020 at 09:49:55PM +0100, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
>
> While well intentioned, checking CAP_MAC_ADMIN for attaching
> BPF_MODIFY_RETURN tracing programs to "security_" functions is not
> necessary as tracing BPF programs already require CAP_SYS_ADMIN.
>
> Fixes: 6ba43b761c41 ("bpf: Attachment verification for BPF_MODIFY_RETURN")
> Signed-off-by: KP Singh <kpsingh@google.com>
Applied. Thanks
^ permalink raw reply
* [PATCH bpf-next] bpf: Fix bpf_prog_test_run_tracing for !CONFIG_NET
From: KP Singh @ 2020-03-05 22:01 UTC (permalink / raw)
To: linux-security-module, linux-next, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Randy Dunlap,
Stephen Rothwell, Florent Revest, Brendan Jackman
From: KP Singh <kpsingh@google.com>
test_run.o is not built when CONFIG_NET is not set and
bpf_prog_test_run_tracing being referenced in bpf_trace.o causes the
linker error:
ld: kernel/trace/bpf_trace.o:(.rodata+0x38): undefined reference to
`bpf_prog_test_run_tracing'
Add a __weak function in bpf_trace.c to handle this.
Fixes: da00d2f117a0 ("bpf: Add test ops for BPF_PROG_TYPE_TRACING")
Signed-off-by: KP Singh <kpsingh@google.com>
---
kernel/trace/bpf_trace.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 363e0a2c75cf..6a490d8ce9de 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1252,6 +1252,13 @@ static bool tracing_prog_is_valid_access(int off, int size,
return btf_ctx_access(off, size, type, prog, info);
}
+int __weak bpf_prog_test_run_tracing(struct bpf_prog *prog,
+ const union bpf_attr *kattr,
+ union bpf_attr __user *uattr)
+{
+ return -ENOTSUPP;
+}
+
const struct bpf_verifier_ops raw_tracepoint_verifier_ops = {
.get_func_proto = raw_tp_prog_func_proto,
.is_valid_access = raw_tp_prog_is_valid_access,
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] KEYS: Don't write out to userspace while holding key semaphore
From: Waiman Long @ 2020-03-05 21:30 UTC (permalink / raw)
To: Eric Biggers
Cc: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, keyrings, linux-kernel, linux-security-module,
linux-integrity, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Chris von Recklinghausen
In-Reply-To: <20200305211651.GA225345@gmail.com>
On 3/5/20 4:16 PM, Eric Biggers wrote:
> On Thu, Mar 05, 2020 at 04:06:40PM -0500, Waiman Long wrote:
>> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
>> index 9b898c969558..564a4d187329 100644
>> --- a/security/keys/keyctl.c
>> +++ b/security/keys/keyctl.c
>> @@ -846,14 +846,36 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>> can_read_key:
>> ret = -EOPNOTSUPP;
>> if (key->type->read) {
>> - /* Read the data with the semaphore held (since we might sleep)
>> + /*
>> + * Read the data with the semaphore held (since we might sleep)
>> * to protect against the key being updated or revoked.
>> + *
>> + * Allocating a temporary buffer to hold the keys before
>> + * transferring them to user buffer to avoid potential
>> + * deadlock involving page fault and mmap_sem.
>> */
>> + char *tmpbuf = kmalloc(buflen, GFP_KERNEL);
> This is passing an arbitrarily large size from userspace into kmalloc().
>
> - Eric
>
That is true. Is there a limit of how much key data are expected to be
returned from the kernel? We could set an internal limit here on how
large the buffer can be.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH] KEYS: Don't write out to userspace while holding key semaphore
From: Eric Biggers @ 2020-03-05 21:16 UTC (permalink / raw)
To: Waiman Long
Cc: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, keyrings, linux-kernel, linux-security-module,
linux-integrity, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Chris von Recklinghausen
In-Reply-To: <20200305210640.15315-1-longman@redhat.com>
On Thu, Mar 05, 2020 at 04:06:40PM -0500, Waiman Long wrote:
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 9b898c969558..564a4d187329 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -846,14 +846,36 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
> can_read_key:
> ret = -EOPNOTSUPP;
> if (key->type->read) {
> - /* Read the data with the semaphore held (since we might sleep)
> + /*
> + * Read the data with the semaphore held (since we might sleep)
> * to protect against the key being updated or revoked.
> + *
> + * Allocating a temporary buffer to hold the keys before
> + * transferring them to user buffer to avoid potential
> + * deadlock involving page fault and mmap_sem.
> */
> + char *tmpbuf = kmalloc(buflen, GFP_KERNEL);
This is passing an arbitrarily large size from userspace into kmalloc().
- Eric
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/7] bpf: Introduce BPF_MODIFY_RETURN
From: KP Singh @ 2020-03-05 21:16 UTC (permalink / raw)
To: Stephen Smalley
Cc: linux-security-module, linux-kernel, bpf, Andrii Nakryiko,
Alexei Starovoitov, Daniel Borkmann, Paul Turner, Jann Horn,
Florent Revest, Brendan Jackman, jmorris, Paul Moore,
Casey Schaufler
In-Reply-To: <CAEjxPJ5u7tsa_9-7Oq_Wi28mZD_aDC1tVWj5Tb8ud=bfEYsY9Q@mail.gmail.com>
On 05-Mär 14:43, Stephen Smalley wrote:
> On Thu, Mar 5, 2020 at 10:54 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > On 05-Mar 08:51, Stephen Smalley wrote:
> > > IIUC you've switched from a model where the BPF program would be
> > > invoked after the original function logic
> > > and the BPF program is skipped if the original function logic returns
> > > non-zero to a model where the BPF program is invoked first and
> > > the original function logic is skipped if the BPF program returns
> > > non-zero. I'm not keen on that for userspace-loaded code attached
> >
> > We do want to continue the KRSI series and the effort to implement a
> > proper BPF LSM. In the meantime, the tracing + error injection
> > solution helps us to:
> >
> > * Provide better debug capabilities.
> > * And parallelize the effort to come up with the right helpers
> > for our LSM work and work on sleepable BPF which is also essential
> > for some of the helpers.
> >
> > As you noted, in the KRSI v4 series, we mentioned that we would like
> > to have the user-space loaded BPF programs be unable to override the
> > decision made by the in-kernel logic/LSMs, but this got shot down:
> >
> > https://lore.kernel.org/bpf/00c216e1-bcfd-b7b1-5444-2a2dfa69190b@schaufler-ca.com
> >
> > I would like to continue this discussion when we post the v5 series
> > for KRSI as to what the correct precedence order should be for the
> > BPF_PROG_TYPE_LSM and would appreciate if you also bring it up there.
>
> That's fine but I guess I don't see why you or anyone else would
> bother with introducing a BPF_PROG_TYPE_LSM
> if BPF_PROG_MODIFY_RETURN is accepted and is allowed to attach to the
> LSM hooks. What's the benefit to you
> if you can achieve your goals directly with MODIFY_RETURN?
There is still value in being a proper LSM, as I had mentioned in KRSI
v3 that not all security_* wrappers simply call the attached hooks and
return their exit code.
It's also okay, taking into consideration Casey's objections and Kees'
suggestion, to be properly registered with the LSM framework (even if
it is with LSM_ORDER_LAST) and work towards improving some of the
performance bottle-necks in the framework. It would be a positive
outcome for all LSMs.
BPF_MODIFY_RETURN is just a first step which lays the foundation for
BPF_PROG_TYPE_LSM and facilitates us to build BPF infrastructure for
it.
- KP
>
> > > to LSM hooks; it means that userspace BPF programs can run even if
> > > SELinux would have denied access and SELinux hooks get
> > > skipped entirely if the BPF program returns an error. I think Casey
> > > may have wrongly pointed you in this direction on the grounds
> > > it can already happen with the base DAC checking logic. But that's
> >
> > What we can do for this tracing/modify_ret series, is to remove
> > the special casing for "security_" functions in the BPF code and add
> > ALLOW_ERROR_INJECTION calls to the security hooks. This way, if
> > someone needs to disable the BPF programs being able to modify
> > security hooks, they can disable error injection. If that's okay, we
> > can send a patch.
>
> Realistically distros tend to enable lots of developer-friendly
> options including error injection, and most users don't build their
> own kernels
> and distros won't support them when they do. So telling users they can
> just rebuild their kernel without error injection if they care about
> BPF programs being able to modify security hooks isn't really viable.
> The security modules need a way to veto it based on their policies.
> That's why I suggested a security hook here.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox