[RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[David Howells]

Hi Al,

Here's a set of patches to add a general variable-length notification queue
concept and to add sources of events for:

 (1) Mount topology events, such as mounting, unmounting, mount expiry,
     mount reconfiguration.

 (2) Superblock events, such as R/W<->R/O changes, quota overrun and I/O
     errors (not complete yet).

 (3) Key/keyring events, such as creating, linking and removal of keys.

 (4) General device events (single common queue) including:

     - Block layer events, such as device errors

     - USB subsystem events, such as device/bus attach/remove, device
       reset, device errors.

One of the reasons for this is so that we can remove the issue of processes
having to repeatedly and regularly scan /proc/mounts, which has proven to
be a system performance problem.  To further aid this, the fsinfo() syscall
on which this patch series depends, provides a way to access superblock and
mount information in binary form without the need to parse /proc/mounts.


LSM support is included, but controversial:

 (1) The creds of the process that did the fput() that reduced the refcount
     to zero are cached in the file struct.

 (2) __fput() overrides the current creds with the creds from (1) whilst
     doing the cleanup, thereby making sure that the creds seen by the
     destruction notification generated by mntput() appears to come from
     the last fputter.

 (3) security_post_notification() is called for each queue that we might
     want to post a notification into, thereby allowing the LSM to prevent
     covert communications.

 (?) Do I need to add security_set_watch(), say, to rule on whether a watch
     may be set in the first place?  I might need to add a variant per
     watch-type.

 (?) Do I really need to keep track of the process creds in which an
     implicit object destruction happened?  For example, imagine you create
     an fd with fsopen()/fsmount().  It is marked to dissolve the mount it
     refers to on close unless move_mount() clears that flag.  Now, imagine
     someone looking at that fd through procfs at the same time as you exit
     due to an error.  The LSM sees the destruction notification come from
     the looker if they happen to do their fput() after yours.


Design decisions:

 (1) A misc chardev is used to create and open a ring buffer:

	fd = open("/dev/watch_queue", O_RDWR);

     which is then configured and mmap'd into userspace:

	ioctl(fd, IOC_WATCH_QUEUE_SET_SIZE, BUF_SIZE);
	ioctl(fd, IOC_WATCH_QUEUE_SET_FILTER, &filter);
	buf = mmap(NULL, BUF_SIZE * page_size, PROT_READ | PROT_WRITE,
		   MAP_SHARED, fd, 0);

     The fd cannot be read or written (though there is a facility to use
     write to inject records for debugging) and userspace just pulls data
     directly out of the buffer.

 (2) The ring index pointers are stored inside the ring and are thus
     accessible to userspace.  Userspace should only update the tail
     pointer and never the head pointer or risk breaking the buffer.  The
     kernel checks that the pointers appear valid before trying to use
     them.  A 'skip' record is maintained around the pointers.

 (3) poll() can be used to wait for data to appear in the buffer.

 (4) Records in the buffer are binary, typed and have a length so that they
     can be of varying size.

     This means that multiple heterogeneous sources can share a common
     buffer.  Tags may be specified when a watchpoint is created to help
     distinguish the sources.

 (5) The queue is reusable as there are 16 million types available, of
     which I've used 4, so there is scope for others to be used.

 (6) Records are filterable as types have up to 256 subtypes that can be
     individually filtered.  Other filtration is also available.

 (7) Each time the buffer is opened, a new buffer is created - this means
     that there's no interference between watchers.

 (8) When recording a notification, the kernel will not sleep, but will
     rather mark a queue as overrun if there's insufficient space, thereby
     avoiding userspace causing the kernel to hang.

 (9) The 'watchpoint' should be specific where possible, meaning that you
     specify the object that you want to watch.

(10) The buffer is created and then watchpoints are attached to it, using
     one of:

	keyctl_watch_key(KEY_SPEC_SESSION_KEYRING, fd, 0x01);
	mount_notify(AT_FDCWD, "/", 0, fd, 0x02);
	sb_notify(AT_FDCWD, "/mnt", 0, fd, 0x03);

     where in all three cases, fd indicates the queue and the number after
     is a tag between 0 and 255.

(11) The watch must be removed if either the watch buffer is destroyed or
     the watched object is destroyed.


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.


Further things that could be considered:

 (1) Adding a keyctl call to allow a watch on a keyring to be extended to
     "children" of that keyring, such that the watch is removed from the
     child if it is unlinked from the keyring.

 (2) Adding global superblock event queue.

 (3) Propagating watches to child superblock over automounts.


The patches can be found here also:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=notifications

Changes:

 v3: I've added a USB notification source and reformulated the block
     notification source so that there's now a common watch list, for which
     the system call is now device_notify().

     I've assigned a pair of unused ioctl numbers in the 'W' series to the
     ioctls added by this series.

     I've also added a description of the kernel API to the documentation.

 v2: I've fixed various issues raised by Jann Horn and GregKH and moved to
     krefs for refcounting.  I've added some security features to try and
     give Casey Schaufler the LSM control he wants.

David
---
David Howells (10):
      security: Override creds in __fput() with last fputter's creds
      General notification queue with user mmap()'able ring buffer
      keys: Add a notification facility
      vfs: Add a mount-notification facility
      vfs: Add superblock notifications
      fsinfo: Export superblock notification counter
      Add a general, global device notification watch list
      block: Add block layer notifications
      usb: Add USB subsystem notifications
      Add sample notification program


 Documentation/ioctl/ioctl-number.txt   |    1 
 Documentation/security/keys/core.rst   |   58 ++
 Documentation/watch_queue.rst          |  492 ++++++++++++++++++
 arch/x86/entry/syscalls/syscall_32.tbl |    3 
 arch/x86/entry/syscalls/syscall_64.tbl |    3 
 block/Kconfig                          |    9 
 block/blk-core.c                       |   29 +
 drivers/base/Kconfig                   |    9 
 drivers/base/Makefile                  |    1 
 drivers/base/notify.c                  |   82 +++
 drivers/misc/Kconfig                   |   13 
 drivers/misc/Makefile                  |    1 
 drivers/misc/watch_queue.c             |  889 ++++++++++++++++++++++++++++++++
 drivers/usb/core/Kconfig               |   10 
 drivers/usb/core/devio.c               |   55 ++
 drivers/usb/core/hub.c                 |    3 
 fs/Kconfig                             |   21 +
 fs/Makefile                            |    1 
 fs/file_table.c                        |   12 
 fs/fsinfo.c                            |   12 
 fs/mount.h                             |   33 +
 fs/mount_notify.c                      |  180 ++++++
 fs/namespace.c                         |    9 
 fs/super.c                             |  116 ++++
 include/linux/blkdev.h                 |   15 +
 include/linux/dcache.h                 |    1 
 include/linux/device.h                 |    7 
 include/linux/fs.h                     |   79 +++
 include/linux/key.h                    |    4 
 include/linux/lsm_hooks.h              |   15 +
 include/linux/security.h               |   14 +
 include/linux/syscalls.h               |    5 
 include/linux/usb.h                    |   19 +
 include/linux/watch_queue.h            |   87 +++
 include/uapi/linux/fsinfo.h            |   10 
 include/uapi/linux/keyctl.h            |    1 
 include/uapi/linux/watch_queue.h       |  213 ++++++++
 kernel/sys_ni.c                        |    7 
 mm/interval_tree.c                     |    2 
 mm/memory.c                            |    1 
 samples/Kconfig                        |    6 
 samples/Makefile                       |    1 
 samples/vfs/test-fsinfo.c              |   13 
 samples/watch_queue/Makefile           |    9 
 samples/watch_queue/watch_test.c       |  310 +++++++++++
 security/keys/Kconfig                  |   10 
 security/keys/compat.c                 |    2 
 security/keys/gc.c                     |    5 
 security/keys/internal.h               |   30 +
 security/keys/key.c                    |   37 +
 security/keys/keyctl.c                 |   88 +++
 security/keys/keyring.c                |   17 -
 security/keys/request_key.c            |    4 
 security/security.c                    |    9 
 54 files changed, 3025 insertions(+), 38 deletions(-)
 create mode 100644 Documentation/watch_queue.rst
 create mode 100644 drivers/base/notify.c
 create mode 100644 drivers/misc/watch_queue.c
 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 samples/watch_queue/Makefile
 create mode 100644 samples/watch_queue/watch_test.c

[PATCH 09/10] usb: Add USB subsystem notifications [ver #3]

[David Howells]

Add a USB subsystem notification mechanism whereby notifications about
hardware events such as device connection, disconnection, reset and I/O
errors, can be reported to a monitoring process asynchronously.

Firstly, an event queue needs to be created:

	fd = open("/dev/event_queue", O_RDWR);
	ioctl(fd, IOC_WATCH_QUEUE_SET_SIZE, page_size << n);

then a notification can be set up to report USB notifications via that
queue:

	struct watch_notification_filter filter = {
		.nr_filters = 1,
		.filters = {
			[0] = {
				.type = WATCH_TYPE_USB_NOTIFY,
				.subtype_filter[0] = UINT_MAX;
			},
		},
	};
	ioctl(fd, IOC_WATCH_QUEUE_SET_FILTER, &filter);
	device_notify(fd, 12);

After that, records will be placed into the queue when events occur on a
USB device or bus.  Records are of the following format:

	struct usb_notification {
		struct watch_notification watch;
		__u32	error;
		__u32	reserved;
		__u8	name_len;
		__u8	name[0];
	} *n;

Where:

	n->watch.type will be WATCH_TYPE_USB_NOTIFY

	n->watch.subtype will be the type of notification, such as
	NOTIFY_USB_DEVICE_ADD.

	n->watch.info & WATCH_INFO_LENGTH will indicate the length of the
	record.

	n->watch.info & WATCH_INFO_ID will be the second argument to
	device_notify(), shifted.

	n->error and n->reserved are intended to convey information such as
	error codes, but are currently not used

	n->name_len and n->name convey the USB device name as an
	unterminated string.  This may be truncated - it is currently
	limited to a maximum 63 chars.

Note that it is permissible for event records to be of variable length -
or, at least, the length may be dependent on the subtype.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
cc: linux-usb@vger.kernel.org
---

 Documentation/watch_queue.rst    |    9 ++++++
 drivers/usb/core/Kconfig         |   10 +++++++
 drivers/usb/core/devio.c         |   55 ++++++++++++++++++++++++++++++++++++++
 drivers/usb/core/hub.c           |    3 ++
 include/linux/usb.h              |   19 +++++++++++++
 include/uapi/linux/watch_queue.h |   30 ++++++++++++++++++++-
 6 files changed, 125 insertions(+), 1 deletion(-)

diff --git a/Documentation/watch_queue.rst b/Documentation/watch_queue.rst
index c2954e191989..7ce5d4147fa9 100644
--- a/Documentation/watch_queue.rst
+++ b/Documentation/watch_queue.rst
@@ -15,6 +15,8 @@ receive notifications from the kernel.  This can be used in conjunction with::
 
     * Block layer event notifications
 
+    * USB subsystem event notifications
+
 
 The notifications buffers can be enabled by:
 
@@ -344,6 +346,13 @@ Any particular buffer can be fed from multiple sources.  Sources include:
     or temporary link loss.  Watchpoints of this type are set on the global
     device watch list.
 
+  * WATCH_TYPE_USB_NOTIFY
+
+    Notifications of this type indicate USB subsystem events, such as
+    attachment, removal, reset and I/O errors.  Separate events are generated
+    for buses and devices.  Watchpoints of this type are set on the global
+    device watch list.
+
 
 Event Filtering
 ===============
diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
index bdb6bd0b63a6..4be88368ab6b 100644
--- a/drivers/usb/core/Kconfig
+++ b/drivers/usb/core/Kconfig
@@ -103,3 +103,13 @@ config USB_AUTOSUSPEND_DELAY
 	  The default value Linux has always had is 2 seconds.  Change
 	  this value if you want a different delay and cannot modify
 	  the command line or module parameter.
+
+config USB_NOTIFICATIONS
+	bool "Provide USB hardware event notifications"
+	depends on USB
+	select DEVICE_NOTIFICATIONS
+	help
+	  This option provides support for getting hardware event notifications
+	  on USB devices and interfaces.  This makes use of the
+	  /dev/watch_queue misc device to handle the notification buffer.
+	  device_notify(2) is used to set/remove watches.
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index fa783531ee88..af7f339c35c5 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -41,6 +41,7 @@
 #include <linux/dma-mapping.h>
 #include <asm/byteorder.h>
 #include <linux/moduleparam.h>
+#include <linux/watch_queue.h>
 
 #include "usb.h"
 
@@ -2633,13 +2634,67 @@ static void usbdev_remove(struct usb_device *udev)
 	}
 }
 
+#ifdef CONFIG_USB_NOTIFICATIONS
+static noinline void post_usb_notification(const char *devname,
+					   enum usb_notification_type subtype,
+					   u32 error)
+{
+	unsigned int name_len, n_len;
+	u64 id = 0; /* Might want to put a dev# here. */
+
+	struct {
+		struct usb_notification n;
+		char more_name[USB_NOTIFICATION_MAX_NAME_LEN -
+			       (sizeof(struct usb_notification) -
+				offsetof(struct usb_notification, name))];
+	} n;
+
+	name_len = strlen(devname);
+	name_len = min_t(size_t, name_len, USB_NOTIFICATION_MAX_NAME_LEN);
+	n_len = round_up(offsetof(struct usb_notification, name) + name_len,
+			 sizeof(__u64));
+
+	memset(&n, 0, sizeof(n));
+	memcpy(n.n.name, devname, n_len);
+
+	n.n.watch.type		= WATCH_TYPE_USB_NOTIFY;
+	n.n.watch.subtype	= subtype;
+	n.n.watch.info		= n_len;
+	n.n.error		= error;
+	n.n.name_len		= name_len;
+
+	post_device_notification(&n.n.watch, id);
+}
+
+void post_usb_device_notification(const struct usb_device *udev,
+				  enum usb_notification_type subtype, u32 error)
+{
+	post_usb_notification(dev_name(&udev->dev), subtype, error);
+}
+
+void post_usb_bus_notification(const struct usb_bus *ubus,
+			       enum usb_notification_type subtype, u32 error)
+{
+	post_usb_notification(ubus->bus_name, subtype, error);
+}
+#endif
+
 static int usbdev_notify(struct notifier_block *self,
 			       unsigned long action, void *dev)
 {
 	switch (action) {
 	case USB_DEVICE_ADD:
+		post_usb_device_notification(dev, NOTIFY_USB_DEVICE_ADD, 0);
 		break;
 	case USB_DEVICE_REMOVE:
+		post_usb_device_notification(dev, NOTIFY_USB_DEVICE_REMOVE, 0);
+		usbdev_remove(dev);
+		break;
+	case USB_BUS_ADD:
+		post_usb_bus_notification(dev, NOTIFY_USB_BUS_ADD, 0);
+		break;
+	case USB_BUS_REMOVE:
+		post_usb_bus_notification(dev, NOTIFY_USB_BUS_REMOVE, 0);
 		usbdev_remove(dev);
 		break;
 	}
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 2f94568ba385..722013d8142c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4596,6 +4596,9 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
 				(udev->config) ? "reset" : "new", speed,
 				devnum, driver_name);
 
+	if (udev->config)
+		post_usb_device_notification(udev, NOTIFY_USB_DEVICE_RESET, 0);
+
 	/* Set up TT records, if needed  */
 	if (hdev->tt) {
 		udev->tt = hdev->tt;
diff --git a/include/linux/usb.h b/include/linux/usb.h
index ae82d9d1112b..12687b55811d 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -2008,6 +2008,25 @@ extern void usb_led_activity(enum usb_led_event ev);
 static inline void usb_led_activity(enum usb_led_event ev) {}
 #endif
 
+/*
+ * Notification functions.
+ */
+#ifdef CONFIG_USB_NOTIFICATIONS
+extern void post_usb_device_notification(const struct usb_device *udev,
+					 enum usb_notification_type subtype,
+					 u32 error);
+extern void post_usb_bus_notification(const struct usb_bus *ubus,
+				      enum usb_notification_type subtype,
+				      u32 error);
+#else
+static inline void post_usb_device_notification(const struct usb_device *udev,
+						enum usb_notification_type subtype,
+						u32 error) {}
+static inline void post_usb_bus_notification(const struct usb_bus *ubus,
+					     enum usb_notification_type subtype,
+					     u32 error) {}
+#endif
+
 #endif  /* __KERNEL__ */
 
 #endif
diff --git a/include/uapi/linux/watch_queue.h b/include/uapi/linux/watch_queue.h
index 22e3326b83a6..d596ac5a61e4 100644
--- a/include/uapi/linux/watch_queue.h
+++ b/include/uapi/linux/watch_queue.h
@@ -14,7 +14,8 @@ enum watch_notification_type {
 	WATCH_TYPE_SB_NOTIFY	= 2,	/* Superblock notification */
 	WATCH_TYPE_KEY_NOTIFY	= 3,	/* Key/keyring change notification */
 	WATCH_TYPE_BLOCK_NOTIFY	= 4,	/* Block layer notifications */
-#define WATCH_TYPE___NR 5
+	WATCH_TYPE_USB_NOTIFY	= 5,	/* USB subsystem notifications */
+#define WATCH_TYPE___NR 6
 };
 
 enum watch_meta_notification_subtype {
@@ -182,4 +183,31 @@ struct block_notification {
 	__u64	sector;			/* Affected sector */
 };
 
+/*
+ * Type of USB layer notification.
+ */
+enum usb_notification_type {
+	NOTIFY_USB_DEVICE_ADD		= 0, /* USB device added */
+	NOTIFY_USB_DEVICE_REMOVE	= 1, /* USB device removed */
+	NOTIFY_USB_BUS_ADD		= 2, /* USB bus added */
+	NOTIFY_USB_BUS_REMOVE		= 3, /* USB bus removed */
+	NOTIFY_USB_DEVICE_RESET		= 4, /* USB device reset */
+	NOTIFY_USB_DEVICE_ERROR		= 5, /* USB device error */
+};
+
+/*
+ * USB subsystem notification record.
+ * - watch.type = WATCH_TYPE_USB_NOTIFY
+ * - watch.subtype = enum usb_notification_type
+ */
+struct usb_notification {
+	struct watch_notification watch; /* WATCH_TYPE_USB_NOTIFY */
+	__u32	error;
+	__u32	reserved;
+	__u8	name_len;		/* Length of device name */
+	__u8	name[0];		/* Device name (padded to __u64, truncated at 63 chars) */
+};
+
+#define USB_NOTIFICATION_MAX_NAME_LEN 63
+
 #endif /* _UAPI_LINUX_WATCH_QUEUE_H */

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[Stephen Smalley]

On 6/6/19 5:41 AM, David Howells wrote:
> 
> Hi Al,
> 
> Here's a set of patches to add a general variable-length notification queue
> concept and to add sources of events for:
> 
>   (1) Mount topology events, such as mounting, unmounting, mount expiry,
>       mount reconfiguration.
> 
>   (2) Superblock events, such as R/W<->R/O changes, quota overrun and I/O
>       errors (not complete yet).
> 
>   (3) Key/keyring events, such as creating, linking and removal of keys.
> 
>   (4) General device events (single common queue) including:
> 
>       - Block layer events, such as device errors
> 
>       - USB subsystem events, such as device/bus attach/remove, device
>         reset, device errors.
> 
> One of the reasons for this is so that we can remove the issue of processes
> having to repeatedly and regularly scan /proc/mounts, which has proven to
> be a system performance problem.  To further aid this, the fsinfo() syscall
> on which this patch series depends, provides a way to access superblock and
> mount information in binary form without the need to parse /proc/mounts.
> 
> 
> LSM support is included, but controversial:
> 
>   (1) The creds of the process that did the fput() that reduced the refcount
>       to zero are cached in the file struct.
> 
>   (2) __fput() overrides the current creds with the creds from (1) whilst
>       doing the cleanup, thereby making sure that the creds seen by the
>       destruction notification generated by mntput() appears to come from
>       the last fputter.
> 
>   (3) security_post_notification() is called for each queue that we might
>       want to post a notification into, thereby allowing the LSM to prevent
>       covert communications.
> 
>   (?) Do I need to add security_set_watch(), say, to rule on whether a watch
>       may be set in the first place?  I might need to add a variant per
>       watch-type.
> 
>   (?) Do I really need to keep track of the process creds in which an
>       implicit object destruction happened?  For example, imagine you create
>       an fd with fsopen()/fsmount().  It is marked to dissolve the mount it
>       refers to on close unless move_mount() clears that flag.  Now, imagine
>       someone looking at that fd through procfs at the same time as you exit
>       due to an error.  The LSM sees the destruction notification come from
>       the looker if they happen to do their fput() after yours.


I'm not in favor of this approach. Can we check permission to the object 
being watched when a watch is set (read-like access), make sure every 
access that can trigger a notification requires a (write-like) 
permission to the accessed object, and make sure there is some sane way 
to control the relationship between the accessed object and the watched 
object (write-like)?  For cases where we have no object per se or at 
least no security structure/label associated with it, we may have to 
fall back to a coarse-grained "Can the watcher get this kind of 
notification in general?".

> 
> 
> Design decisions:
> 
>   (1) A misc chardev is used to create and open a ring buffer:
> 
> 	fd = open("/dev/watch_queue", O_RDWR);
> 
>       which is then configured and mmap'd into userspace:
> 
> 	ioctl(fd, IOC_WATCH_QUEUE_SET_SIZE, BUF_SIZE);
> 	ioctl(fd, IOC_WATCH_QUEUE_SET_FILTER, &filter);
> 	buf = mmap(NULL, BUF_SIZE * page_size, PROT_READ | PROT_WRITE,
> 		   MAP_SHARED, fd, 0);
> 
>       The fd cannot be read or written (though there is a facility to use
>       write to inject records for debugging) and userspace just pulls data
>       directly out of the buffer.
> 
>   (2) The ring index pointers are stored inside the ring and are thus
>       accessible to userspace.  Userspace should only update the tail
>       pointer and never the head pointer or risk breaking the buffer.  The
>       kernel checks that the pointers appear valid before trying to use
>       them.  A 'skip' record is maintained around the pointers.
> 
>   (3) poll() can be used to wait for data to appear in the buffer.
> 
>   (4) Records in the buffer are binary, typed and have a length so that they
>       can be of varying size.
> 
>       This means that multiple heterogeneous sources can share a common
>       buffer.  Tags may be specified when a watchpoint is created to help
>       distinguish the sources.
> 
>   (5) The queue is reusable as there are 16 million types available, of
>       which I've used 4, so there is scope for others to be used.
> 
>   (6) Records are filterable as types have up to 256 subtypes that can be
>       individually filtered.  Other filtration is also available.
> 
>   (7) Each time the buffer is opened, a new buffer is created - this means
>       that there's no interference between watchers.
> 
>   (8) When recording a notification, the kernel will not sleep, but will
>       rather mark a queue as overrun if there's insufficient space, thereby
>       avoiding userspace causing the kernel to hang.
> 
>   (9) The 'watchpoint' should be specific where possible, meaning that you
>       specify the object that you want to watch.
> 
> (10) The buffer is created and then watchpoints are attached to it, using
>       one of:
> 
> 	keyctl_watch_key(KEY_SPEC_SESSION_KEYRING, fd, 0x01);
> 	mount_notify(AT_FDCWD, "/", 0, fd, 0x02);
> 	sb_notify(AT_FDCWD, "/mnt", 0, fd, 0x03);
> 
>       where in all three cases, fd indicates the queue and the number after
>       is a tag between 0 and 255.
> 
> (11) The watch must be removed if either the watch buffer is destroyed or
>       the watched object is destroyed.
> 
> 
> 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.
> 
> 
> Further things that could be considered:
> 
>   (1) Adding a keyctl call to allow a watch on a keyring to be extended to
>       "children" of that keyring, such that the watch is removed from the
>       child if it is unlinked from the keyring.
> 
>   (2) Adding global superblock event queue.
> 
>   (3) Propagating watches to child superblock over automounts.
> 
> 
> The patches can be found here also:
> 
> 	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=notifications
> 
> Changes:
> 
>   v3: I've added a USB notification source and reformulated the block
>       notification source so that there's now a common watch list, for which
>       the system call is now device_notify().
> 
>       I've assigned a pair of unused ioctl numbers in the 'W' series to the
>       ioctls added by this series.
> 
>       I've also added a description of the kernel API to the documentation.
> 
>   v2: I've fixed various issues raised by Jann Horn and GregKH and moved to
>       krefs for refcounting.  I've added some security features to try and
>       give Casey Schaufler the LSM control he wants.
> 
> David
> ---
> David Howells (10):
>        security: Override creds in __fput() with last fputter's creds
>        General notification queue with user mmap()'able ring buffer
>        keys: Add a notification facility
>        vfs: Add a mount-notification facility
>        vfs: Add superblock notifications
>        fsinfo: Export superblock notification counter
>        Add a general, global device notification watch list
>        block: Add block layer notifications
>        usb: Add USB subsystem notifications
>        Add sample notification program
> 
> 
>   Documentation/ioctl/ioctl-number.txt   |    1
>   Documentation/security/keys/core.rst   |   58 ++
>   Documentation/watch_queue.rst          |  492 ++++++++++++++++++
>   arch/x86/entry/syscalls/syscall_32.tbl |    3
>   arch/x86/entry/syscalls/syscall_64.tbl |    3
>   block/Kconfig                          |    9
>   block/blk-core.c                       |   29 +
>   drivers/base/Kconfig                   |    9
>   drivers/base/Makefile                  |    1
>   drivers/base/notify.c                  |   82 +++
>   drivers/misc/Kconfig                   |   13
>   drivers/misc/Makefile                  |    1
>   drivers/misc/watch_queue.c             |  889 ++++++++++++++++++++++++++++++++
>   drivers/usb/core/Kconfig               |   10
>   drivers/usb/core/devio.c               |   55 ++
>   drivers/usb/core/hub.c                 |    3
>   fs/Kconfig                             |   21 +
>   fs/Makefile                            |    1
>   fs/file_table.c                        |   12
>   fs/fsinfo.c                            |   12
>   fs/mount.h                             |   33 +
>   fs/mount_notify.c                      |  180 ++++++
>   fs/namespace.c                         |    9
>   fs/super.c                             |  116 ++++
>   include/linux/blkdev.h                 |   15 +
>   include/linux/dcache.h                 |    1
>   include/linux/device.h                 |    7
>   include/linux/fs.h                     |   79 +++
>   include/linux/key.h                    |    4
>   include/linux/lsm_hooks.h              |   15 +
>   include/linux/security.h               |   14 +
>   include/linux/syscalls.h               |    5
>   include/linux/usb.h                    |   19 +
>   include/linux/watch_queue.h            |   87 +++
>   include/uapi/linux/fsinfo.h            |   10
>   include/uapi/linux/keyctl.h            |    1
>   include/uapi/linux/watch_queue.h       |  213 ++++++++
>   kernel/sys_ni.c                        |    7
>   mm/interval_tree.c                     |    2
>   mm/memory.c                            |    1
>   samples/Kconfig                        |    6
>   samples/Makefile                       |    1
>   samples/vfs/test-fsinfo.c              |   13
>   samples/watch_queue/Makefile           |    9
>   samples/watch_queue/watch_test.c       |  310 +++++++++++
>   security/keys/Kconfig                  |   10
>   security/keys/compat.c                 |    2
>   security/keys/gc.c                     |    5
>   security/keys/internal.h               |   30 +
>   security/keys/key.c                    |   37 +
>   security/keys/keyctl.c                 |   88 +++
>   security/keys/keyring.c                |   17 -
>   security/keys/request_key.c            |    4
>   security/security.c                    |    9
>   54 files changed, 3025 insertions(+), 38 deletions(-)
>   create mode 100644 Documentation/watch_queue.rst
>   create mode 100644 drivers/base/notify.c
>   create mode 100644 drivers/misc/watch_queue.c
>   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 samples/watch_queue/Makefile
>   create mode 100644 samples/watch_queue/watch_test.c
> 

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[David Howells]

Stephen Smalley <sds@tycho.nsa.gov> wrote:

This might be easier to discuss if you can reply to:

	https://lore.kernel.org/lkml/5393.1559768763@warthog.procyon.org.uk/

which is on the ver #2 posting of this patchset.

> > LSM support is included, but controversial:
> >
> >   (1) The creds of the process that did the fput() that reduced the refcount
> >       to zero are cached in the file struct.
> >
> >   (2) __fput() overrides the current creds with the creds from (1) whilst
> >       doing the cleanup, thereby making sure that the creds seen by the
> >       destruction notification generated by mntput() appears to come from
> >       the last fputter.
> >
> >   (3) security_post_notification() is called for each queue that we might
> >       want to post a notification into, thereby allowing the LSM to prevent
> >       covert communications.
> >
> >   (?) Do I need to add security_set_watch(), say, to rule on whether a watch
> >       may be set in the first place?  I might need to add a variant per
> >       watch-type.
> >
> >   (?) Do I really need to keep track of the process creds in which an
> >       implicit object destruction happened?  For example, imagine you create
> >       an fd with fsopen()/fsmount().  It is marked to dissolve the mount it
> >       refers to on close unless move_mount() clears that flag.  Now, imagine
> >       someone looking at that fd through procfs at the same time as you exit
> >       due to an error.  The LSM sees the destruction notification come from
> >       the looker if they happen to do their fput() after yours.
> 
> 
> I'm not in favor of this approach.

Which bit?  The last point?  Keeping track of the process creds after an
implicit object destruction.

> Can we check permission to the object being watched when a watch is set
> (read-like access),

Yes, and I need to do that.  I think it's likely to require an extra hook for
each entry point added because the objects are different:

	int security_watch_key(struct watch *watch, struct key *key);
	int security_watch_sb(struct watch *watch, struct path *path);
	int security_watch_mount(struct watch *watch, struct path *path);
	int security_watch_devices(struct watch *watch);

> make sure every access that can trigger a notification requires a
> (write-like) permission to the accessed object,

"write-like permssion" for whom?  The triggerer or the watcher?

There are various 'classes' of events:

 (1) System events (eg. hardware I/O errors, automount points expiring).

 (2) Direct events (eg. automounts, manual mounts, EDQUOT, key linkage).

 (3) Indirect events (eg. exit/close doing the last fput and causing an
     unmount).

Class (1) are uncaused by a process, so I use init_cred for them.  One could
argue that the automount point expiry should perhaps take place under the
creds of whoever triggered it in the first place, but we need to be careful
about long-term cred pinning.

Class (2) the causing process must've had permission to cause them - otherwise
we wouldn't have got the event.

Class (3) is interesting since it's currently entirely cleanup events and the
process may have the right to do them (close, dup2, exit, but also execve)
whether the LSM thinks it should be able to cause the object to be destroyed
or not.

It gets more complicated than that, though: multiple processes with different
security attributes can all have fds pointing to a common file object - and
the last one to close carries the can as far as the LSM is concerned.

And yet more complicated when you throw in unix sockets with partially passed
fds still in their queues.  That's what patch 01 is designed to try and cope
with.

> and make sure there is some sane way to control the relationship between the
> accessed object and the watched object (write-like)?

This is the trick.  Keys and superblocks have object labels of their own and
don't - for now - propagate their watches.  With these, the watch is on the
object you initially assign it to and it goes no further than that.

mount_notify() is the interesting case since we want to be able to detect
mount topology change events from within the vfs subtree rooted at the watched
directory without having to manually put a watch on every directory in that
subtree - or even just every mount object.

Or, maybe, that's what I'll have to do: make it mount_notify() can only apply
to the subtree within its superblock, and the caller must call mount_notify()
for every mount object it wants to monitor.  That would at least ensure that
the caller can, at that point, reach all those mount points.

> For cases where we have no object per se or at least no security
> structure/label associated with it, we may have to fall back to a
> coarse-grained "Can the watcher get this kind of notification in general?".

Agreed - and we should probably have that anyway.

David

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[Stephen Smalley]

On 6/6/19 9:16 AM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
> This might be easier to discuss if you can reply to:
> 
> 	https://lore.kernel.org/lkml/5393.1559768763@warthog.procyon.org.uk/
> 
> which is on the ver #2 posting of this patchset.

Sorry for being late to the party.  Not sure whether you're asking me to 
respond only there or both there and here to your comments below.  I'll 
start here but can revisit if it's a problem.
> 
>>> LSM support is included, but controversial:
>>>
>>>    (1) The creds of the process that did the fput() that reduced the refcount
>>>        to zero are cached in the file struct.
>>>
>>>    (2) __fput() overrides the current creds with the creds from (1) whilst
>>>        doing the cleanup, thereby making sure that the creds seen by the
>>>        destruction notification generated by mntput() appears to come from
>>>        the last fputter.
>>>
>>>    (3) security_post_notification() is called for each queue that we might
>>>        want to post a notification into, thereby allowing the LSM to prevent
>>>        covert communications.
>>>
>>>    (?) Do I need to add security_set_watch(), say, to rule on whether a watch
>>>        may be set in the first place?  I might need to add a variant per
>>>        watch-type.
>>>
>>>    (?) Do I really need to keep track of the process creds in which an
>>>        implicit object destruction happened?  For example, imagine you create
>>>        an fd with fsopen()/fsmount().  It is marked to dissolve the mount it
>>>        refers to on close unless move_mount() clears that flag.  Now, imagine
>>>        someone looking at that fd through procfs at the same time as you exit
>>>        due to an error.  The LSM sees the destruction notification come from
>>>        the looker if they happen to do their fput() after yours.
>>
>>
>> I'm not in favor of this approach.
> 
> Which bit?  The last point?  Keeping track of the process creds after an
> implicit object destruction.

(1), (2), (3), and the last point.

>> Can we check permission to the object being watched when a watch is set
>> (read-like access),
> 
> Yes, and I need to do that.  I think it's likely to require an extra hook for
> each entry point added because the objects are different:
> 
> 	int security_watch_key(struct watch *watch, struct key *key);
> 	int security_watch_sb(struct watch *watch, struct path *path);
> 	int security_watch_mount(struct watch *watch, struct path *path);
> 	int security_watch_devices(struct watch *watch);
> 
>> make sure every access that can trigger a notification requires a
>> (write-like) permission to the accessed object,
> 
> "write-like permssion" for whom?  The triggerer or the watcher?

The former, i.e. the process that performed the operation that triggered 
the notification.  Think of it as a write from the process to the 
accessed object, which triggers a notification (another write) on some 
related object (the watched object), which is then read by the watcher.

> There are various 'classes' of events:
> 
>   (1) System events (eg. hardware I/O errors, automount points expiring).
> 
>   (2) Direct events (eg. automounts, manual mounts, EDQUOT, key linkage).
> 
>   (3) Indirect events (eg. exit/close doing the last fput and causing an
>       unmount).
> 
> Class (1) are uncaused by a process, so I use init_cred for them.  One could
> argue that the automount point expiry should perhaps take place under the
> creds of whoever triggered it in the first place, but we need to be careful
> about long-term cred pinning.

This seems equivalent to just checking whether the watcher is allowed to 
get that kind of event, no other cred truly needed.

> Class (2) the causing process must've had permission to cause them - otherwise
> we wouldn't have got the event.

So we've already done a check on the causing process, and we're going to 
check whether the watcher can set the watch. We just need to establish 
the connection between the accessed object and the watched object in 
some manner.

> Class (3) is interesting since it's currently entirely cleanup events and the
> process may have the right to do them (close, dup2, exit, but also execve)
> whether the LSM thinks it should be able to cause the object to be destroyed
> or not.
> 
> It gets more complicated than that, though: multiple processes with different
> security attributes can all have fds pointing to a common file object - and
> the last one to close carries the can as far as the LSM is concerned.

Yes, I'd prefer to avoid that.  You can't write policy that is stable 
and meaningful that way.  This may fall under a similar situation as 
class (1) - all we can meaningfully do is check whether the watcher is 
allowed to see all such events.

> And yet more complicated when you throw in unix sockets with partially passed
> fds still in their queues.  That's what patch 01 is designed to try and cope
> with.
> 
>> and make sure there is some sane way to control the relationship between the
>> accessed object and the watched object (write-like)?
> 
> This is the trick.  Keys and superblocks have object labels of their own and
> don't - for now - propagate their watches.  With these, the watch is on the
> object you initially assign it to and it goes no further than that.
> 
> mount_notify() is the interesting case since we want to be able to detect
> mount topology change events from within the vfs subtree rooted at the watched
> directory without having to manually put a watch on every directory in that
> subtree - or even just every mount object. >
> Or, maybe, that's what I'll have to do: make it mount_notify() can only apply
> to the subtree within its superblock, and the caller must call mount_notify()
> for every mount object it wants to monitor.  That would at least ensure that
> the caller can, at that point, reach all those mount points.

Would that at least make it consistent with fanotify (not that it 
provides a great example)?

>> For cases where we have no object per se or at least no security
>> structure/label associated with it, we may have to fall back to a
>> coarse-grained "Can the watcher get this kind of notification in general?".
> 
> Agreed - and we should probably have that anyway.
> 
> David

Re: [PATCH 09/10] usb: Add USB subsystem notifications [ver #3]

[Alan Stern]

On Thu, 6 Jun 2019, David Howells wrote:

> Add a USB subsystem notification mechanism whereby notifications about
> hardware events such as device connection, disconnection, reset and I/O
> errors, can be reported to a monitoring process asynchronously.

USB I/O errors covers an awfully large and vague field.  Do we really
want to include them?  I'm doubtful.

Alan Stern

Re: [PATCH 09/10] usb: Add USB subsystem notifications [ver #3]

[Greg Kroah-Hartman]

On Thu, Jun 06, 2019 at 10:24:18AM -0400, Alan Stern wrote:
> On Thu, 6 Jun 2019, David Howells wrote:
> 
> > Add a USB subsystem notification mechanism whereby notifications about
> > hardware events such as device connection, disconnection, reset and I/O
> > errors, can be reported to a monitoring process asynchronously.
> 
> USB I/O errors covers an awfully large and vague field.  Do we really
> want to include them?  I'm doubtful.

See the other patch on the linux-usb list that wanted to start adding
KOBJ_CHANGE notifications about USB "i/o errors".

So for "severe" issues, yes, we should do this, but perhaps not for all
of the "normal" things we see when a device is yanked out of the system
and the like.

thanks,

greg k-h

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[Christian Brauner]

On Thu, Jun 06, 2019 at 10:41:59AM +0100, David Howells wrote:
> 
> Hi Al,
> 
> Here's a set of patches to add a general variable-length notification queue
> concept and to add sources of events for:
> 
>  (1) Mount topology events, such as mounting, unmounting, mount expiry,
>      mount reconfiguration.
> 
>  (2) Superblock events, such as R/W<->R/O changes, quota overrun and I/O
>      errors (not complete yet).
> 
>  (3) Key/keyring events, such as creating, linking and removal of keys.
> 
>  (4) General device events (single common queue) including:
> 
>      - Block layer events, such as device errors
> 
>      - USB subsystem events, such as device/bus attach/remove, device
>        reset, device errors.
> 
> One of the reasons for this is so that we can remove the issue of processes
> having to repeatedly and regularly scan /proc/mounts, which has proven to
> be a system performance problem.  To further aid this, the fsinfo() syscall
> on which this patch series depends, provides a way to access superblock and
> mount information in binary form without the need to parse /proc/mounts.
> 
> 
> LSM support is included, but controversial:

Apart from the LSM/security controversy here the general direction of
this patchset is pretty well received it seems.
Imho, having a notification mechanism like this is a very good thing for
userspace. So would be great if there'd be a consensus on the LSM bits.

Christian

Re: [PATCH 09/10] usb: Add USB subsystem notifications [ver #3]

[Alan Stern]

On Thu, 6 Jun 2019, Greg Kroah-Hartman wrote:

> On Thu, Jun 06, 2019 at 10:24:18AM -0400, Alan Stern wrote:
> > On Thu, 6 Jun 2019, David Howells wrote:
> > 
> > > Add a USB subsystem notification mechanism whereby notifications about
> > > hardware events such as device connection, disconnection, reset and I/O
> > > errors, can be reported to a monitoring process asynchronously.
> > 
> > USB I/O errors covers an awfully large and vague field.  Do we really
> > want to include them?  I'm doubtful.
> 
> See the other patch on the linux-usb list that wanted to start adding
> KOBJ_CHANGE notifications about USB "i/o errors".

That patch wanted to add notifications only for enumeration failures
(assuming you're talking about the patch from Eugeniu Rosca), not I/O
errors in general.

> So for "severe" issues, yes, we should do this, but perhaps not for all
> of the "normal" things we see when a device is yanked out of the system
> and the like.

Then what counts as a "severe" issue?  Anything besides enumeration 
failure?

Alan Stern

Re: [PATCH 09/10] usb: Add USB subsystem notifications [ver #3]

[Greg Kroah-Hartman]

On Thu, Jun 06, 2019 at 10:55:24AM -0400, Alan Stern wrote:
> On Thu, 6 Jun 2019, Greg Kroah-Hartman wrote:
> 
> > On Thu, Jun 06, 2019 at 10:24:18AM -0400, Alan Stern wrote:
> > > On Thu, 6 Jun 2019, David Howells wrote:
> > > 
> > > > Add a USB subsystem notification mechanism whereby notifications about
> > > > hardware events such as device connection, disconnection, reset and I/O
> > > > errors, can be reported to a monitoring process asynchronously.
> > > 
> > > USB I/O errors covers an awfully large and vague field.  Do we really
> > > want to include them?  I'm doubtful.
> > 
> > See the other patch on the linux-usb list that wanted to start adding
> > KOBJ_CHANGE notifications about USB "i/o errors".
> 
> That patch wanted to add notifications only for enumeration failures
> (assuming you're talking about the patch from Eugeniu Rosca), not I/O
> errors in general.

Yes, sorry, I was thinking that as a "I/O failed in enumeration" :)

> > So for "severe" issues, yes, we should do this, but perhaps not for all
> > of the "normal" things we see when a device is yanked out of the system
> > and the like.
> 
> Then what counts as a "severe" issue?  Anything besides enumeration 
> failure?

Not that I can think of at the moment, other than the other recently
added KOBJ_CHANGE issue.  I'm sure we have other "hard failure" issues
in the USB stack that people will want exposed over time.

thanks,

greg k-h

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[Casey Schaufler]

On 6/6/2019 7:05 AM, Stephen Smalley wrote:
> On 6/6/19 9:16 AM, David Howells wrote:
>> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>
>> This might be easier to discuss if you can reply to:
>>
>>     https://lore.kernel.org/lkml/5393.1559768763@warthog.procyon.org.uk/
>>
>> which is on the ver #2 posting of this patchset.
>
> Sorry for being late to the party.  Not sure whether you're asking me to respond only there or both there and here to your comments below.  I'll start here but can revisit if it's a problem.
>>
>>>> LSM support is included, but controversial:
>>>>
>>>>    (1) The creds of the process that did the fput() that reduced the refcount
>>>>        to zero are cached in the file struct.
>>>>
>>>>    (2) __fput() overrides the current creds with the creds from (1) whilst
>>>>        doing the cleanup, thereby making sure that the creds seen by the
>>>>        destruction notification generated by mntput() appears to come from
>>>>        the last fputter.
>>>>
>>>>    (3) security_post_notification() is called for each queue that we might
>>>>        want to post a notification into, thereby allowing the LSM to prevent
>>>>        covert communications.
>>>>
>>>>    (?) Do I need to add security_set_watch(), say, to rule on whether a watch
>>>>        may be set in the first place?  I might need to add a variant per
>>>>        watch-type.
>>>>
>>>>    (?) Do I really need to keep track of the process creds in which an
>>>>        implicit object destruction happened?  For example, imagine you create
>>>>        an fd with fsopen()/fsmount().  It is marked to dissolve the mount it
>>>>        refers to on close unless move_mount() clears that flag.  Now, imagine
>>>>        someone looking at that fd through procfs at the same time as you exit
>>>>        due to an error.  The LSM sees the destruction notification come from
>>>>        the looker if they happen to do their fput() after yours.
>>>
>>>
>>> I'm not in favor of this approach.
>>
>> Which bit?  The last point?  Keeping track of the process creds after an
>> implicit object destruction.
>
> (1), (2), (3), and the last point.
>
>>> Can we check permission to the object being watched when a watch is set
>>> (read-like access),
>>
>> Yes, and I need to do that.  I think it's likely to require an extra hook for
>> each entry point added because the objects are different:
>>
>>     int security_watch_key(struct watch *watch, struct key *key);
>>     int security_watch_sb(struct watch *watch, struct path *path);
>>     int security_watch_mount(struct watch *watch, struct path *path);
>>     int security_watch_devices(struct watch *watch);
>>
>>> make sure every access that can trigger a notification requires a
>>> (write-like) permission to the accessed object,
>>
>> "write-like permssion" for whom?  The triggerer or the watcher?
>
> The former, i.e. the process that performed the operation that triggered the notification.  Think of it as a write from the process to the accessed object, which triggers a notification (another write) on some related object (the watched object), which is then read by the watcher.

We agree that the process that performed the operation that triggered
the notification is the initial subject. Smack will treat the process
with the watch set (in particular, its ring buffer) as the object
being written to. SELinux, with its finer grained controls, will
involve other things as noted above. There are other place where we
see this, notably IP packet delivery.

The implication is that the information about the triggering
process needs to be available throughout.

>
>> There are various 'classes' of events:
>>
>>   (1) System events (eg. hardware I/O errors, automount points expiring).
>>
>>   (2) Direct events (eg. automounts, manual mounts, EDQUOT, key linkage).
>>
>>   (3) Indirect events (eg. exit/close doing the last fput and causing an
>>       unmount).
>>
>> Class (1) are uncaused by a process, so I use init_cred for them.  One could
>> argue that the automount point expiry should perhaps take place under the
>> creds of whoever triggered it in the first place, but we need to be careful
>> about long-term cred pinning.
>
> This seems equivalent to just checking whether the watcher is allowed to get that kind of event, no other cred truly needed.
>
>> Class (2) the causing process must've had permission to cause them - otherwise
>> we wouldn't have got the event.
>
> So we've already done a check on the causing process, and we're going to check whether the watcher can set the watch. We just need to establish the connection between the accessed object and the watched object in some manner.

I don't agree. That is, I don't believe it is sufficient.
There is no guarantee that being able to set a watch on an
object implies that every process that can trigger the event
can send it to you.

	Watcher has Smack label W
	Triggerer has Smack label T
	Watched object has Smack label O

	Relevant Smack rules are

	W O rw
	T O rw

The watcher will be able to set the watch,
the triggerer will be able to trigger the event,
but there is nothing that would allow the watcher
to receive the event. This is not a case of watcher
reading the watched object, as the event is delivered
without any action by watcher.

>
>> Class (3) is interesting since it's currently entirely cleanup events and the
>> process may have the right to do them (close, dup2, exit, but also execve)
>> whether the LSM thinks it should be able to cause the object to be destroyed
>> or not.
>>
>> It gets more complicated than that, though: multiple processes with different
>> security attributes can all have fds pointing to a common file object - and
>> the last one to close carries the can as far as the LSM is concerned.
>
> Yes, I'd prefer to avoid that.  You can't write policy that is stable and meaningful that way.  This may fall under a similar situation as class (1) - all we can meaningfully do is check whether the watcher is allowed to see all such events.

Back in the day when we were doing security evaluations
the implications of "deleting" an object gave the security
evaluators fits. UNIX/Linux files don't get deleted, they
simply fall off the filesystem namespace when no one cares
about them anymore. The model we used back in the day was
that "delete" wasn't an operation that occurs on filesystem
objects.

But now you want to do something with security implications
when the object disappears. We could say that the event does
nothing but signal that the system has removed the watch on
your behalf because it is now meaningless. No reason to worry
about who dropped the last reference. We don't care about
that from a policy viewpoint anyway.

>
>> And yet more complicated when you throw in unix sockets with partially passed
>> fds still in their queues.  That's what patch 01 is designed to try and cope
>> with.
>>
>>> and make sure there is some sane way to control the relationship between the
>>> accessed object and the watched object (write-like)?
>>
>> This is the trick.  Keys and superblocks have object labels of their own and
>> don't - for now - propagate their watches.  With these, the watch is on the
>> object you initially assign it to and it goes no further than that.
>>
>> mount_notify() is the interesting case since we want to be able to detect
>> mount topology change events from within the vfs subtree rooted at the watched
>> directory without having to manually put a watch on every directory in that
>> subtree - or even just every mount object. >
>> Or, maybe, that's what I'll have to do: make it mount_notify() can only apply
>> to the subtree within its superblock, and the caller must call mount_notify()
>> for every mount object it wants to monitor.  That would at least ensure that
>> the caller can, at that point, reach all those mount points.
>
> Would that at least make it consistent with fanotify (not that it provides a great example)?
>
>>> For cases where we have no object per se or at least no security
>>> structure/label associated with it, we may have to fall back to a
>>> coarse-grained "Can the watcher get this kind of notification in general?".
>>
>> Agreed - and we should probably have that anyway.
>>
>> David

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[Andy Lutomirski]

On Thu, Jun 6, 2019 at 9:43 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 6/6/2019 7:05 AM, Stephen Smalley wrote:
> > On 6/6/19 9:16 AM, David Howells wrote:
> >> Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >>
> >> This might be easier to discuss if you can reply to:
> >>
> >>     https://lore.kernel.org/lkml/5393.1559768763@warthog.procyon.org.uk/
> >>
> >> which is on the ver #2 posting of this patchset.
> >
> > Sorry for being late to the party.  Not sure whether you're asking me to respond only there or both there and here to your comments below.  I'll start here but can revisit if it's a problem.
> >>
> >>>> LSM support is included, but controversial:
> >>>>
> >>>>    (1) The creds of the process that did the fput() that reduced the refcount
> >>>>        to zero are cached in the file struct.
> >>>>
> >>>>    (2) __fput() overrides the current creds with the creds from (1) whilst
> >>>>        doing the cleanup, thereby making sure that the creds seen by the
> >>>>        destruction notification generated by mntput() appears to come from
> >>>>        the last fputter.
> >>>>
> >>>>    (3) security_post_notification() is called for each queue that we might
> >>>>        want to post a notification into, thereby allowing the LSM to prevent
> >>>>        covert communications.
> >>>>
> >>>>    (?) Do I need to add security_set_watch(), say, to rule on whether a watch
> >>>>        may be set in the first place?  I might need to add a variant per
> >>>>        watch-type.
> >>>>
> >>>>    (?) Do I really need to keep track of the process creds in which an
> >>>>        implicit object destruction happened?  For example, imagine you create
> >>>>        an fd with fsopen()/fsmount().  It is marked to dissolve the mount it
> >>>>        refers to on close unless move_mount() clears that flag.  Now, imagine
> >>>>        someone looking at that fd through procfs at the same time as you exit
> >>>>        due to an error.  The LSM sees the destruction notification come from
> >>>>        the looker if they happen to do their fput() after yours.
> >>>
> >>>
> >>> I'm not in favor of this approach.
> >>
> >> Which bit?  The last point?  Keeping track of the process creds after an
> >> implicit object destruction.
> >
> > (1), (2), (3), and the last point.
> >
> >>> Can we check permission to the object being watched when a watch is set
> >>> (read-like access),
> >>
> >> Yes, and I need to do that.  I think it's likely to require an extra hook for
> >> each entry point added because the objects are different:
> >>
> >>     int security_watch_key(struct watch *watch, struct key *key);
> >>     int security_watch_sb(struct watch *watch, struct path *path);
> >>     int security_watch_mount(struct watch *watch, struct path *path);
> >>     int security_watch_devices(struct watch *watch);
> >>
> >>> make sure every access that can trigger a notification requires a
> >>> (write-like) permission to the accessed object,
> >>
> >> "write-like permssion" for whom?  The triggerer or the watcher?
> >
> > The former, i.e. the process that performed the operation that triggered the notification.  Think of it as a write from the process to the accessed object, which triggers a notification (another write) on some related object (the watched object), which is then read by the watcher.
>
> We agree that the process that performed the operation that triggered
> the notification is the initial subject. Smack will treat the process
> with the watch set (in particular, its ring buffer) as the object
> being written to. SELinux, with its finer grained controls, will
> involve other things as noted above. There are other place where we
> see this, notably IP packet delivery.
>
> The implication is that the information about the triggering
> process needs to be available throughout.
>
> >
> >> There are various 'classes' of events:
> >>
> >>   (1) System events (eg. hardware I/O errors, automount points expiring).
> >>
> >>   (2) Direct events (eg. automounts, manual mounts, EDQUOT, key linkage).
> >>
> >>   (3) Indirect events (eg. exit/close doing the last fput and causing an
> >>       unmount).
> >>
> >> Class (1) are uncaused by a process, so I use init_cred for them.  One could
> >> argue that the automount point expiry should perhaps take place under the
> >> creds of whoever triggered it in the first place, but we need to be careful
> >> about long-term cred pinning.
> >
> > This seems equivalent to just checking whether the watcher is allowed to get that kind of event, no other cred truly needed.
> >
> >> Class (2) the causing process must've had permission to cause them - otherwise
> >> we wouldn't have got the event.
> >
> > So we've already done a check on the causing process, and we're going to check whether the watcher can set the watch. We just need to establish the connection between the accessed object and the watched object in some manner.
>
> I don't agree. That is, I don't believe it is sufficient.
> There is no guarantee that being able to set a watch on an
> object implies that every process that can trigger the event
> can send it to you.
>
>         Watcher has Smack label W
>         Triggerer has Smack label T
>         Watched object has Smack label O
>
>         Relevant Smack rules are
>
>         W O rw
>         T O rw
>
> The watcher will be able to set the watch,
> the triggerer will be able to trigger the event,
> but there is nothing that would allow the watcher
> to receive the event. This is not a case of watcher
> reading the watched object, as the event is delivered
> without any action by watcher.

I think this is an example of a bogus policy that should not be
supported by the kernel.

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[Stephen Smalley]

On 6/6/19 12:43 PM, Casey Schaufler wrote:
> On 6/6/2019 7:05 AM, Stephen Smalley wrote:
>> On 6/6/19 9:16 AM, David Howells wrote:
>>> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>
>>> This might be easier to discuss if you can reply to:
>>>
>>>      https://lore.kernel.org/lkml/5393.1559768763@warthog.procyon.org.uk/
>>>
>>> which is on the ver #2 posting of this patchset.
>>
>> Sorry for being late to the party.  Not sure whether you're asking me to respond only there or both there and here to your comments below.  I'll start here but can revisit if it's a problem.
>>>
>>>>> LSM support is included, but controversial:
>>>>>
>>>>>     (1) The creds of the process that did the fput() that reduced the refcount
>>>>>         to zero are cached in the file struct.
>>>>>
>>>>>     (2) __fput() overrides the current creds with the creds from (1) whilst
>>>>>         doing the cleanup, thereby making sure that the creds seen by the
>>>>>         destruction notification generated by mntput() appears to come from
>>>>>         the last fputter.
>>>>>
>>>>>     (3) security_post_notification() is called for each queue that we might
>>>>>         want to post a notification into, thereby allowing the LSM to prevent
>>>>>         covert communications.
>>>>>
>>>>>     (?) Do I need to add security_set_watch(), say, to rule on whether a watch
>>>>>         may be set in the first place?  I might need to add a variant per
>>>>>         watch-type.
>>>>>
>>>>>     (?) Do I really need to keep track of the process creds in which an
>>>>>         implicit object destruction happened?  For example, imagine you create
>>>>>         an fd with fsopen()/fsmount().  It is marked to dissolve the mount it
>>>>>         refers to on close unless move_mount() clears that flag.  Now, imagine
>>>>>         someone looking at that fd through procfs at the same time as you exit
>>>>>         due to an error.  The LSM sees the destruction notification come from
>>>>>         the looker if they happen to do their fput() after yours.
>>>>
>>>>
>>>> I'm not in favor of this approach.
>>>
>>> Which bit?  The last point?  Keeping track of the process creds after an
>>> implicit object destruction.
>>
>> (1), (2), (3), and the last point.
>>
>>>> Can we check permission to the object being watched when a watch is set
>>>> (read-like access),
>>>
>>> Yes, and I need to do that.  I think it's likely to require an extra hook for
>>> each entry point added because the objects are different:
>>>
>>>      int security_watch_key(struct watch *watch, struct key *key);
>>>      int security_watch_sb(struct watch *watch, struct path *path);
>>>      int security_watch_mount(struct watch *watch, struct path *path);
>>>      int security_watch_devices(struct watch *watch);
>>>
>>>> make sure every access that can trigger a notification requires a
>>>> (write-like) permission to the accessed object,
>>>
>>> "write-like permssion" for whom?  The triggerer or the watcher?
>>
>> The former, i.e. the process that performed the operation that triggered the notification.  Think of it as a write from the process to the accessed object, which triggers a notification (another write) on some related object (the watched object), which is then read by the watcher.
> 
> We agree that the process that performed the operation that triggered
> the notification is the initial subject. Smack will treat the process
> with the watch set (in particular, its ring buffer) as the object
> being written to. SELinux, with its finer grained controls, will
> involve other things as noted above. There are other place where we
> see this, notably IP packet delivery.
> 
> The implication is that the information about the triggering
> process needs to be available throughout.
> 
>>
>>> There are various 'classes' of events:
>>>
>>>    (1) System events (eg. hardware I/O errors, automount points expiring).
>>>
>>>    (2) Direct events (eg. automounts, manual mounts, EDQUOT, key linkage).
>>>
>>>    (3) Indirect events (eg. exit/close doing the last fput and causing an
>>>        unmount).
>>>
>>> Class (1) are uncaused by a process, so I use init_cred for them.  One could
>>> argue that the automount point expiry should perhaps take place under the
>>> creds of whoever triggered it in the first place, but we need to be careful
>>> about long-term cred pinning.
>>
>> This seems equivalent to just checking whether the watcher is allowed to get that kind of event, no other cred truly needed.
>>
>>> Class (2) the causing process must've had permission to cause them - otherwise
>>> we wouldn't have got the event.
>>
>> So we've already done a check on the causing process, and we're going to check whether the watcher can set the watch. We just need to establish the connection between the accessed object and the watched object in some manner.
> 
> I don't agree. That is, I don't believe it is sufficient.
> There is no guarantee that being able to set a watch on an
> object implies that every process that can trigger the event
> can send it to you.
> 
> 	Watcher has Smack label W
> 	Triggerer has Smack label T
> 	Watched object has Smack label O
> 
> 	Relevant Smack rules are
> 
> 	W O rw
> 	T O rw
> 
> The watcher will be able to set the watch,
> the triggerer will be able to trigger the event,
> but there is nothing that would allow the watcher
> to receive the event. This is not a case of watcher
> reading the watched object, as the event is delivered
> without any action by watcher.

You are allowing arbitrary information flow between T and W above.  Who 
cares about notifications?

How is it different from W and T mapping the same file as a shared 
mapping and communicating by reading and writing the shared memory?  You 
aren't performing a permission check directly between W and T there.

> 
>>
>>> Class (3) is interesting since it's currently entirely cleanup events and the
>>> process may have the right to do them (close, dup2, exit, but also execve)
>>> whether the LSM thinks it should be able to cause the object to be destroyed
>>> or not.
>>>
>>> It gets more complicated than that, though: multiple processes with different
>>> security attributes can all have fds pointing to a common file object - and
>>> the last one to close carries the can as far as the LSM is concerned.
>>
>> Yes, I'd prefer to avoid that.  You can't write policy that is stable and meaningful that way.  This may fall under a similar situation as class (1) - all we can meaningfully do is check whether the watcher is allowed to see all such events.
> 
> Back in the day when we were doing security evaluations
> the implications of "deleting" an object gave the security
> evaluators fits. UNIX/Linux files don't get deleted, they
> simply fall off the filesystem namespace when no one cares
> about them anymore. The model we used back in the day was
> that "delete" wasn't an operation that occurs on filesystem
> objects.
> 
> But now you want to do something with security implications
> when the object disappears. We could say that the event does
> nothing but signal that the system has removed the watch on
> your behalf because it is now meaningless. No reason to worry
> about who dropped the last reference. We don't care about
> that from a policy viewpoint anyway.
> 
>>
>>> And yet more complicated when you throw in unix sockets with partially passed
>>> fds still in their queues.  That's what patch 01 is designed to try and cope
>>> with.
>>>
>>>> and make sure there is some sane way to control the relationship between the
>>>> accessed object and the watched object (write-like)?
>>>
>>> This is the trick.  Keys and superblocks have object labels of their own and
>>> don't - for now - propagate their watches.  With these, the watch is on the
>>> object you initially assign it to and it goes no further than that.
>>>
>>> mount_notify() is the interesting case since we want to be able to detect
>>> mount topology change events from within the vfs subtree rooted at the watched
>>> directory without having to manually put a watch on every directory in that
>>> subtree - or even just every mount object. >
>>> Or, maybe, that's what I'll have to do: make it mount_notify() can only apply
>>> to the subtree within its superblock, and the caller must call mount_notify()
>>> for every mount object it wants to monitor.  That would at least ensure that
>>> the caller can, at that point, reach all those mount points.
>>
>> Would that at least make it consistent with fanotify (not that it provides a great example)?
>>
>>>> For cases where we have no object per se or at least no security
>>>> structure/label associated with it, we may have to fall back to a
>>>> coarse-grained "Can the watcher get this kind of notification in general?".
>>>
>>> Agreed - and we should probably have that anyway.
>>>
>>> David
> 

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[Casey Schaufler]

On 6/6/2019 10:11 AM, Andy Lutomirski wrote:
> On Thu, Jun 6, 2019 at 9:43 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> ...
>> I don't agree. That is, I don't believe it is sufficient.
>> There is no guarantee that being able to set a watch on an
>> object implies that every process that can trigger the event
>> can send it to you.
>>
>>         Watcher has Smack label W
>>         Triggerer has Smack label T
>>         Watched object has Smack label O
>>
>>         Relevant Smack rules are
>>
>>         W O rw
>>         T O rw
>>
>> The watcher will be able to set the watch,
>> the triggerer will be able to trigger the event,
>> but there is nothing that would allow the watcher
>> to receive the event. This is not a case of watcher
>> reading the watched object, as the event is delivered
>> without any action by watcher.
> I think this is an example of a bogus policy that should not be
> supported by the kernel.

At this point it's pretty hard for me to care much what
you think. You don't seem to have any insight into the
implications of the features you're advocating, or their
potential consequences.

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[Andy Lutomirski]

> On Jun 6, 2019, at 11:33 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> 
>> On 6/6/2019 10:11 AM, Andy Lutomirski wrote:
>>> On Thu, Jun 6, 2019 at 9:43 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> ...
>>> I don't agree. That is, I don't believe it is sufficient.
>>> There is no guarantee that being able to set a watch on an
>>> object implies that every process that can trigger the event
>>> can send it to you.
>>> 
>>>        Watcher has Smack label W
>>>        Triggerer has Smack label T
>>>        Watched object has Smack label O
>>> 
>>>        Relevant Smack rules are
>>> 
>>>        W O rw
>>>        T O rw
>>> 
>>> The watcher will be able to set the watch,
>>> the triggerer will be able to trigger the event,
>>> but there is nothing that would allow the watcher
>>> to receive the event. This is not a case of watcher
>>> reading the watched object, as the event is delivered
>>> without any action by watcher.
>> I think this is an example of a bogus policy that should not be
>> supported by the kernel.
> 
> At this point it's pretty hard for me to care much what
> you think. You don't seem to have any insight into the
> implications of the features you're advocating, or their
> potential consequences.
> 
> 

Can you try to spell it out, then?  A mostly or fully worked out example might help.

As Stephen said, it looks like you are considering cases where there is already a full communication channel between two processes, and you’re concerned that this new mechanism might add a side channel too.  If this is wrong, can you explain how?

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[Casey Schaufler]

On 6/6/2019 10:16 AM, Stephen Smalley wrote:
> On 6/6/19 12:43 PM, Casey Schaufler wrote:
>> ...
>> I don't agree. That is, I don't believe it is sufficient.
>> There is no guarantee that being able to set a watch on an
>> object implies that every process that can trigger the event
>> can send it to you.
>>
>>     Watcher has Smack label W
>>     Triggerer has Smack label T
>>     Watched object has Smack label O
>>
>>     Relevant Smack rules are
>>
>>     W O rw
>>     T O rw
>>
>> The watcher will be able to set the watch,
>> the triggerer will be able to trigger the event,
>> but there is nothing that would allow the watcher
>> to receive the event. This is not a case of watcher
>> reading the watched object, as the event is delivered
>> without any action by watcher.
>
> You are allowing arbitrary information flow between T and W above.  Who cares about notifications?

I do. If Watched object is /dev/null no data flow is possible.
There are many objects on a modern Linux system for which this
is true. Even if it's "just a file" the existence of one path
for data to flow does not justify ignoring the rules for other
data paths.

>
> How is it different from W and T mapping the same file as a shared mapping and communicating by reading and writing the shared memory?  You aren't performing a permission check directly between W and T there.

In this case there is one object O, two subjects, W and T and two accesses.

	W open O
	T open O

They fiddle about with the data in O.

In the event case, there are two objects, O and W, two subjects W and T, and
three accesses.

	W watch O
	T trigger O
	T write-event W

You can't wave away the flow of data. Different objects are involved.

An analogy is that two processes with different UIDs can open a file,
but still can't signal each other. Different mechanisms have different
policies. I'm not saying that's good, but it's the context we're in.

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[Andy Lutomirski]

On Thu, Jun 6, 2019 at 11:56 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 6/6/2019 10:16 AM, Stephen Smalley wrote:
> > On 6/6/19 12:43 PM, Casey Schaufler wrote:
> >> ...
> >> I don't agree. That is, I don't believe it is sufficient.
> >> There is no guarantee that being able to set a watch on an
> >> object implies that every process that can trigger the event
> >> can send it to you.
> >>
> >>     Watcher has Smack label W
> >>     Triggerer has Smack label T
> >>     Watched object has Smack label O
> >>
> >>     Relevant Smack rules are
> >>
> >>     W O rw
> >>     T O rw
> >>
> >> The watcher will be able to set the watch,
> >> the triggerer will be able to trigger the event,
> >> but there is nothing that would allow the watcher
> >> to receive the event. This is not a case of watcher
> >> reading the watched object, as the event is delivered
> >> without any action by watcher.
> >
> > You are allowing arbitrary information flow between T and W above.  Who cares about notifications?
>
> I do. If Watched object is /dev/null no data flow is possible.
> There are many objects on a modern Linux system for which this
> is true. Even if it's "just a file" the existence of one path
> for data to flow does not justify ignoring the rules for other
> data paths.

Aha!

Even ignoring security, writes to things like /dev/null should
probably not trigger notifications to people who are watching
/dev/null.  (There are probably lots of things like this: /dev/zero,
/dev/urandom, etc.)  David, are there any notification types that have
this issue in your patchset?  If so, is there a straightforward way to
fix it?  Generically, it seems like maybe writes to device nodes
shouldn't trigger notifications since, despite the fact that different
openers of a device node share an inode, there isn't necessarily any
connection between them.

Casey, if this is fixed in general, do you have another case where the
right to write and the right to read do not imply the right to
communicate?

> An analogy is that two processes with different UIDs can open a file,
> but still can't signal each other.

What do you mean "signal"?  If two processes with different UIDs can
open the same file for read and write, then they can communicate with
each other in many ways.  For example, one can write to the file and
the other can read it.  One can take locks and the other can read the
lock state.  They can both map it and use any number of memory access
side channels to communicate.  But, of course, they can't send each
other signals with kill().

If, however, one of these processes is using some fancy mechanism
(inotify, dnotify, kqueue, fanotify, whatever) to watch the file, and
the other one writes it, then it seems inconsistent to lie to the
watching process and say that the file wasn't written because some
security policy has decided to allow the write, allow the read, but
suppress this particular notification.  Hence my request for a real
example: when would it make sense to do this?

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[David Howells]

Andy Lutomirski <luto@kernel.org> wrote:

> > > You are allowing arbitrary information flow between T and W above.  Who
> > > cares about notifications?
> >
> > I do. If Watched object is /dev/null no data flow is possible.
> > There are many objects on a modern Linux system for which this
> > is true. Even if it's "just a file" the existence of one path
> > for data to flow does not justify ignoring the rules for other
> > data paths.
> 
> Aha!
> 
> Even ignoring security, writes to things like /dev/null should
> probably not trigger notifications to people who are watching
> /dev/null.  (There are probably lots of things like this: /dev/zero,
> /dev/urandom, etc.)

Even writes to /dev/null might generate access notifications; leastways,
vfs_read() will call fsnotify_access() afterwards on success.

Whether or not you can set marks on open device files is another matter.

> David, are there any notification types that have this issue in your
> patchset?  If so, is there a straightforward way to fix it?

I'm not sure what issue you're referring to specifically.  Do you mean whether
writes to device files generate notifications?

> Generically, it seems like maybe writes to device nodes shouldn't trigger
> notifications since, despite the fact that different openers of a device
> node share an inode, there isn't necessarily any connection between them.

With the notification types I have currently implemented, I don't even notice
any accesses to a device file unless:

 (1) Someone mounts over the top of one.

 (2) The access triggers an I/O error or device reset or causes the device to
     be attached or detached.

 (3) Wangling the device causes some other superblock event.

 (4) The driver calls request_key() and that creates a new key.

> Casey, if this is fixed in general, do you have another case where the
> right to write and the right to read do not imply the right to
> communicate?
> 
> > An analogy is that two processes with different UIDs can open a file,
> > but still can't signal each other.
> 
> What do you mean "signal"?  If two processes with different UIDs can
> open the same file for read and write, then they can communicate with
> each other in many ways.  For example, one can write to the file and
> the other can read it.  One can take locks and the other can read the
> lock state.  They can both map it and use any number of memory access
> side channels to communicate.  But, of course, they can't send each
> other signals with kill().
> 
> If, however, one of these processes is using some fancy mechanism
> (inotify, dnotify, kqueue, fanotify, whatever) to watch the file, and
> the other one writes it, then it seems inconsistent to lie to the
> watching process and say that the file wasn't written because some
> security policy has decided to allow the write, allow the read, but
> suppress this particular notification.  Hence my request for a real
> example: when would it make sense to do this?

Note that fanotify requires CAP_SYS_ADMIN, but inotify and dnotify do not.

dnotify is applied to an open file, so it might be usable on a chardev such as
/dev/null, say.

David

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[Andy Lutomirski]

> On Jun 6, 2019, at 2:17 PM, David Howells <dhowells@redhat.com> wrote:
> 
> Andy Lutomirski <luto@kernel.org> wrote:
> 
>>>> You are allowing arbitrary information flow between T and W above.  Who
>>>> cares about notifications?
>>> 
>>> I do. If Watched object is /dev/null no data flow is possible.
>>> There are many objects on a modern Linux system for which this
>>> is true. Even if it's "just a file" the existence of one path
>>> for data to flow does not justify ignoring the rules for other
>>> data paths.
>> 
>> Aha!
>> 
>> Even ignoring security, writes to things like /dev/null should
>> probably not trigger notifications to people who are watching
>> /dev/null.  (There are probably lots of things like this: /dev/zero,
>> /dev/urandom, etc.)
> 
> Even writes to /dev/null might generate access notifications; leastways,
> vfs_read() will call fsnotify_access() afterwards on success.

Hmm. I can see this being an issue, but I guess not with your patch set.

> 
> Whether or not you can set marks on open device files is another matter.
> 
>> David, are there any notification types that have this issue in your
>> patchset?  If so, is there a straightforward way to fix it?
> 
> I'm not sure what issue you're referring to specifically.  Do you mean whether
> writes to device files generate notifications?

I mean: are there cases where some action generates a notification but does not otherwise have an effect visible to the users who can receive the notification. It looks like the answer is probably “no”, which is good.

Casey, is this good enough for you, or is there still an issue?

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[David Howells]

Andy Lutomirski <luto@amacapital.net> wrote:

> I mean: are there cases where some action generates a notification but does
> not otherwise have an effect visible to the users who can receive the
> notification. It looks like the answer is probably “no”, which is good.

mount_notify().  You can get a notification that someone altered the mount
topology (eg. by mounting something).  A process receiving a notification
could then use fsinfo(), say, to reread the mount topology tree, find out
where the new mount is and wander over there to have a look - assuming they
have the permissions for pathwalk to succeed.

David

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[Andy Lutomirski]

> On Jun 6, 2019, at 3:38 PM, David Howells <dhowells@redhat.com> wrote:
> 
> Andy Lutomirski <luto@amacapital.net> wrote:
> 
>> I mean: are there cases where some action generates a notification but does
>> not otherwise have an effect visible to the users who can receive the
>> notification. It looks like the answer is probably “no”, which is good.
> 
> mount_notify().  You can get a notification that someone altered the mount
> topology (eg. by mounting something).  A process receiving a notification
> could then use fsinfo(), say, to reread the mount topology tree, find out
> where the new mount is and wander over there to have a look - assuming they
> have the permissions for pathwalk to succeed.
> 
> 

They can call fsinfo() anyway, or just read /proc/self/mounts. As far as I’m concerned, if you have CAP_SYS_ADMIN over a mount namespace and LSM policy lets you mount things, the of course you can get information to basically anyone who can use that mount namespace.

Re: [RFC][PATCH 00/10] Mount, FS, Block and Keyrings notifications [ver #3]

[David Howells]

Andy Lutomirski <luto@amacapital.net> wrote:

> They can call fsinfo() anyway, or just read /proc/self/mounts. As far as I’m
> concerned, if you have CAP_SYS_ADMIN over a mount namespace and LSM policy
> lets you mount things, the of course you can get information to basically
> anyone who can use that mount namespace.

And automounts?  You don't need CAP_SYS_ADMIN to trigger one of those, but
they still generate events.  On the other hand, you need CSA to mount
something that has automounts in the first place, and if you're particularly
concerned about security, you probably don't want the processes you might be
suspicious of having access to things that contain automounts (typically
network filesystems).

David

Re: [PATCH 09/10] usb: Add USB subsystem notifications [ver #3]

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]


Hi,

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Thu, Jun 06, 2019 at 10:55:24AM -0400, Alan Stern wrote:
>> On Thu, 6 Jun 2019, Greg Kroah-Hartman wrote:
>> 
>> > On Thu, Jun 06, 2019 at 10:24:18AM -0400, Alan Stern wrote:
>> > > On Thu, 6 Jun 2019, David Howells wrote:
>> > > 
>> > > > Add a USB subsystem notification mechanism whereby notifications about
>> > > > hardware events such as device connection, disconnection, reset and I/O
>> > > > errors, can be reported to a monitoring process asynchronously.
>> > > 
>> > > USB I/O errors covers an awfully large and vague field.  Do we really
>> > > want to include them?  I'm doubtful.
>> > 
>> > See the other patch on the linux-usb list that wanted to start adding
>> > KOBJ_CHANGE notifications about USB "i/o errors".
>> 
>> That patch wanted to add notifications only for enumeration failures
>> (assuming you're talking about the patch from Eugeniu Rosca), not I/O
>> errors in general.
>
> Yes, sorry, I was thinking that as a "I/O failed in enumeration" :)
>
>> > So for "severe" issues, yes, we should do this, but perhaps not for all
>> > of the "normal" things we see when a device is yanked out of the system
>> > and the like.
>> 
>> Then what counts as a "severe" issue?  Anything besides enumeration 
>> failure?
>
> Not that I can think of at the moment, other than the other recently
> added KOBJ_CHANGE issue.  I'm sure we have other "hard failure" issues
> in the USB stack that people will want exposed over time.

From an XHCI standpoint, Transaction Errors might be one thing. They
happen rarely and are a strong indication that the bus itself is
bad. Either bad cable, misbehaving PHYs, improper power management, etc.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 09/10] usb: Add USB subsystem notifications [ver #3]

[Alan Stern]

On Fri, 7 Jun 2019, Felipe Balbi wrote:

> Hi,
> 
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> > On Thu, Jun 06, 2019 at 10:55:24AM -0400, Alan Stern wrote:
> >> On Thu, 6 Jun 2019, Greg Kroah-Hartman wrote:
> >> 
> >> > On Thu, Jun 06, 2019 at 10:24:18AM -0400, Alan Stern wrote:
> >> > > On Thu, 6 Jun 2019, David Howells wrote:
> >> > > 
> >> > > > Add a USB subsystem notification mechanism whereby notifications about
> >> > > > hardware events such as device connection, disconnection, reset and I/O
> >> > > > errors, can be reported to a monitoring process asynchronously.
> >> > > 
> >> > > USB I/O errors covers an awfully large and vague field.  Do we really
> >> > > want to include them?  I'm doubtful.
> >> > 
> >> > See the other patch on the linux-usb list that wanted to start adding
> >> > KOBJ_CHANGE notifications about USB "i/o errors".
> >> 
> >> That patch wanted to add notifications only for enumeration failures
> >> (assuming you're talking about the patch from Eugeniu Rosca), not I/O
> >> errors in general.
> >
> > Yes, sorry, I was thinking that as a "I/O failed in enumeration" :)
> >
> >> > So for "severe" issues, yes, we should do this, but perhaps not for all
> >> > of the "normal" things we see when a device is yanked out of the system
> >> > and the like.
> >> 
> >> Then what counts as a "severe" issue?  Anything besides enumeration 
> >> failure?
> >
> > Not that I can think of at the moment, other than the other recently
> > added KOBJ_CHANGE issue.  I'm sure we have other "hard failure" issues
> > in the USB stack that people will want exposed over time.
> 
> From an XHCI standpoint, Transaction Errors might be one thing. They
> happen rarely and are a strong indication that the bus itself is
> bad. Either bad cable, misbehaving PHYs, improper power management, etc.

Don't you also get transaction errors if the user unplugs a device in 
the middle of a transfer?  That's not the sort of thing we want to sent 
notifications about.

Alan Stern

Re: [PATCH 09/10] usb: Add USB subsystem notifications [ver #3]

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2262 bytes --]


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> > On Thu, Jun 06, 2019 at 10:55:24AM -0400, Alan Stern wrote:
>> >> On Thu, 6 Jun 2019, Greg Kroah-Hartman wrote:
>> >> 
>> >> > On Thu, Jun 06, 2019 at 10:24:18AM -0400, Alan Stern wrote:
>> >> > > On Thu, 6 Jun 2019, David Howells wrote:
>> >> > > 
>> >> > > > Add a USB subsystem notification mechanism whereby notifications about
>> >> > > > hardware events such as device connection, disconnection, reset and I/O
>> >> > > > errors, can be reported to a monitoring process asynchronously.
>> >> > > 
>> >> > > USB I/O errors covers an awfully large and vague field.  Do we really
>> >> > > want to include them?  I'm doubtful.
>> >> > 
>> >> > See the other patch on the linux-usb list that wanted to start adding
>> >> > KOBJ_CHANGE notifications about USB "i/o errors".
>> >> 
>> >> That patch wanted to add notifications only for enumeration failures
>> >> (assuming you're talking about the patch from Eugeniu Rosca), not I/O
>> >> errors in general.
>> >
>> > Yes, sorry, I was thinking that as a "I/O failed in enumeration" :)
>> >
>> >> > So for "severe" issues, yes, we should do this, but perhaps not for all
>> >> > of the "normal" things we see when a device is yanked out of the system
>> >> > and the like.
>> >> 
>> >> Then what counts as a "severe" issue?  Anything besides enumeration 
>> >> failure?
>> >
>> > Not that I can think of at the moment, other than the other recently
>> > added KOBJ_CHANGE issue.  I'm sure we have other "hard failure" issues
>> > in the USB stack that people will want exposed over time.
>> 
>> From an XHCI standpoint, Transaction Errors might be one thing. They
>> happen rarely and are a strong indication that the bus itself is
>> bad. Either bad cable, misbehaving PHYs, improper power management, etc.
>
> Don't you also get transaction errors if the user unplugs a device in 
> the middle of a transfer?  That's not the sort of thing we want to sent 
> notifications about.

Mathias, do we get Transaction Error if user removes cable during a
transfer? I thought we would just get Port Status Change with CC bit
cleared, no?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 09/10] usb: Add USB subsystem notifications [ver #3]

[Alan Stern]

On Tue, 11 Jun 2019, Felipe Balbi wrote:

> >> >> > So for "severe" issues, yes, we should do this, but perhaps not for all
> >> >> > of the "normal" things we see when a device is yanked out of the system
> >> >> > and the like.
> >> >> 
> >> >> Then what counts as a "severe" issue?  Anything besides enumeration 
> >> >> failure?
> >> >
> >> > Not that I can think of at the moment, other than the other recently
> >> > added KOBJ_CHANGE issue.  I'm sure we have other "hard failure" issues
> >> > in the USB stack that people will want exposed over time.
> >> 
> >> From an XHCI standpoint, Transaction Errors might be one thing. They
> >> happen rarely and are a strong indication that the bus itself is
> >> bad. Either bad cable, misbehaving PHYs, improper power management, etc.
> >
> > Don't you also get transaction errors if the user unplugs a device in 
> > the middle of a transfer?  That's not the sort of thing we want to sent 
> > notifications about.
> 
> Mathias, do we get Transaction Error if user removes cable during a
> transfer? I thought we would just get Port Status Change with CC bit
> cleared, no?

Even if xHCI doesn't give Transaction Errors when a cable is unplugged 
during a transfer, other host controllers do.  Sometimes quite a lot -- 
they continue to occur until the kernel polls the parent hub's 
interrupt ep and learns that the port is disconnected, which can take 
up to 250 ms.

Alan Stern

Re: [PATCH 09/10] usb: Add USB subsystem notifications [ver #3]

[Felipe Balbi]

Hi,

Alan Stern <stern@rowland.harvard.edu> writes:

> On Tue, 11 Jun 2019, Felipe Balbi wrote:
>
>> >> >> > So for "severe" issues, yes, we should do this, but perhaps not for all
>> >> >> > of the "normal" things we see when a device is yanked out of the system
>> >> >> > and the like.
>> >> >> 
>> >> >> Then what counts as a "severe" issue?  Anything besides enumeration 
>> >> >> failure?
>> >> >
>> >> > Not that I can think of at the moment, other than the other recently
>> >> > added KOBJ_CHANGE issue.  I'm sure we have other "hard failure" issues
>> >> > in the USB stack that people will want exposed over time.
>> >> 
>> >> From an XHCI standpoint, Transaction Errors might be one thing. They
>> >> happen rarely and are a strong indication that the bus itself is
>> >> bad. Either bad cable, misbehaving PHYs, improper power management, etc.
>> >
>> > Don't you also get transaction errors if the user unplugs a device in 
>> > the middle of a transfer?  That's not the sort of thing we want to sent 
>> > notifications about.
>> 
>> Mathias, do we get Transaction Error if user removes cable during a
>> transfer? I thought we would just get Port Status Change with CC bit
>> cleared, no?
>
> Even if xHCI doesn't give Transaction Errors when a cable is unplugged 
> during a transfer, other host controllers do.  Sometimes quite a lot -- 
> they continue to occur until the kernel polls the parent hub's 
> interrupt ep and learns that the port is disconnected, which can take 
> up to 250 ms.

my comment was specific about XHCI. It even started with "From an XHCI
standpoint" :-)

-- 
balbi