[PATCH v7 0/4] fanotify: introduce new event mask FAN_OPEN_EXEC and FAN_OPEN_EXEC_PERM

[PATCH v7 0/4] fanotify: introduce new event mask FAN_OPEN_EXEC and FAN_OPEN_EXEC_PERM

[Matthew Bobrowski]

Currently, the fanotify API does not provide a means for user space
applications to receive events when a file has been opened specifically
for execution. New event masks FAN_OPEN_EXEC and FAN_OPEN_EXEC_PERM have
been introduced in order to provide users with this capability.

These event types, when either are explicitly requested by the user, will
be returned within the event mask when a marked file object is being
opened has __FMODE_EXEC set as one of the flags for open_flag.

Linux is used as an operating system in some products, with an environment
that can be certified under the Common Criteria Operating System
Protection Profile (OSPP). This is a formal threat model for a class of
technology. It requires specific countermeasures to mitigate threats. It
requires documentation to explain how a product implements these
countermeasures. It requires proof via a test suite to demonstrate that
the requirements are met, observed and checked by an independent qualified
third party. The latest set of requirements for OSPP v4.2 can be found
here:

https://www.niap-ccevs.org/Profile/Info.cfm?PPID=424&id=424

If you look on page 58, you will see the following requirement:

FPT_SRP_EXT.1 Software Restriction Policies FPT_SRP_EXT.1.1
administrator specified [selection:
        file path,
        file digital signature,
        version,
        hash,
        [assignment: other characteristics]
]

This patch is to help aid in meeting this requirement.

Changes since v6:
	* Reordered patches within the patch series for the sake of
	  correctness.
	* Updated patch that contains FAN_OPEN_EXEC_PERM functionality to
	  include separate call to fsnotify_parent()/fsnotify() which is
	  used to mitigate merging of permission events.

Note that this set of patches are based on Amir's fsnotify-fixes branch,
which has already been posted through for review. For those interested,
this branch can be found here:
https://github.com/amir73il/linux/commits/fsnotify-fixes

LTP tests for this feature are on my 'fanotify-exec' branch here:
https://github.com/matthewbobrowski/ltp/commits/fanotify_exec. The files
that contains the test cases are provided below:

syscalls/fanotify03: test cases have been updated to cover
                     FAN_OPEN_EXEC_PERM events
syscalls/fanotify12: newly introduced LTP test file to cover
                     FAN_OPEN_EXEC events

All LTP tests have been run against this series of patches and all are
returning as a pass.

The man pages updates for these newly defined event masks can be found
here:
https://github.com/matthewbobrowski/man-pages/commits/fanotify_exec

I would like to thank both Amir and Jan for their time and help, highly
appreciated once again.

Matthew Bobrowski (4):
  fanotify: return only user requested event types in event mask
  fanotify: introduce new event mask FAN_OPEN_EXEC
  fsnotify: refactor fsnotify_parent()/fsnotify() paired calls when
    event is on path
  fanotify: introduce new event mask FAN_OPEN_EXEC_PERM

 fs/notify/fanotify/fanotify.c    | 29 ++++++++-------
 fs/notify/fsnotify.c             |  2 +-
 include/linux/fanotify.h         |  5 +--
 include/linux/fsnotify.h         | 61 +++++++++++++++++++-------------
 include/linux/fsnotify_backend.h | 11 ++++--
 include/uapi/linux/fanotify.h    |  2 ++
 6 files changed, 67 insertions(+), 43 deletions(-)

-- 
2.17.2


-- 
Matthew Bobrowski

[PATCH v7 1/4] fanotify: return only user requested event types in event mask

[Matthew Bobrowski]

Modify fanotify_should_send_event() so that it now returns a mask for
an event that contains ONLY flags for the event types that have been
specifically requested by the user. Flags that may have been included
within the event mask, but have not been explicitly requested by the
user will not be present in the returned value.

As an example, given the situation where a user requests events of type
FAN_OPEN. Traditionally, the event mask returned within an event that
occurred on a filesystem object that has been marked for monitoring and is
opened, will only ever have the FAN_OPEN bit set. With the introduction of
the new flags like FAN_OPEN_EXEC, and perhaps any other future event
flags, there is a possibility of the returned event mask containing more
than a single bit set, despite having only requested the single event type.
Prior to these modifications performed to fanotify_should_send_event(), a
user would have received a bundled event mask containing flags FAN_OPEN
and FAN_OPEN_EXEC in the instance that a file was opened for execution via
execve(), for example. This means that a user would receive event types
in the returned event mask that have not been requested. This runs the
possibility of breaking existing systems and causing other unforeseen
issues.

To mitigate this possibility, fanotify_should_send_event() has been
modified to return the event mask containing ONLY event types explicitly
requested by the user. This means that we will NOT report events that the
user did no set a mask for, and we will NOT report events that the user
has set an ignore mask for.

The function name fanotify_should_send_event() has also been updated so
that it's more relevant to what it has been designed to do.

Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index e08a6647267b..0a09950317dd 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -89,7 +89,13 @@ static int fanotify_get_response(struct fsnotify_group *group,
 	return ret;
 }
 
-static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
+/*
+ * This function returns a mask for an event that only contains the flags
+ * that have been specifically requested by the user. Flags that may have
+ * been included within the event mask, but have not been explicitly
+ * requested by the user, will not be present in the returned mask.
+ */
+static u32 fanotify_group_event_mask(struct fsnotify_iter_info *iter_info,
 				       u32 event_mask, const void *data,
 				       int data_type)
 {
@@ -101,14 +107,14 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
 	pr_debug("%s: report_mask=%x mask=%x data=%p data_type=%d\n",
 		 __func__, iter_info->report_mask, event_mask, data, data_type);
 
-	/* if we don't have enough info to send an event to userspace say no */
+	/* If we don't have enough info to send an event to userspace say no */
 	if (data_type != FSNOTIFY_EVENT_PATH)
-		return false;
+		return 0;
 
-	/* sorry, fanotify only gives a damn about files and dirs */
+	/* Sorry, fanotify only gives a damn about files and dirs */
 	if (!d_is_reg(path->dentry) &&
 	    !d_can_lookup(path->dentry))
-		return false;
+		return 0;
 
 	fsnotify_foreach_obj_type(type) {
 		if (!fsnotify_iter_should_report_type(iter_info, type))
@@ -131,11 +137,7 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
 	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
 		return false;
 
-	if (event_mask & FANOTIFY_OUTGOING_EVENTS &
-	    marks_mask & ~marks_ignored_mask)
-		return true;
-
-	return false;
+	return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask;
 }
 
 struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
@@ -210,7 +212,8 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 
 	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 10);
 
-	if (!fanotify_should_send_event(iter_info, mask, data, data_type))
+	mask = fanotify_group_event_mask(iter_info, mask, data, data_type);
+	if (!mask)
 		return 0;
 
 	pr_debug("%s: group=%p inode=%p mask=%x\n", __func__, group, inode,
-- 
2.17.2


-- 
Matthew Bobrowski

[PATCH v7 2/4] fanotify: introduce new event mask FAN_OPEN_EXEC

[Matthew Bobrowski]

A new event mask FAN_OPEN_EXEC has been defined so that users have the
ability to receive events specifically when a file has been opened with
the intent to be executed. Events of FAN_OPEN_EXEC type will be
generated when a file has been opened using either execve(), execveat()
or uselib() system calls.

The feature is implemented within fsnotify_open() by generating the
FAN_OPEN_EXEC event type if __FMODE_EXEC is set within file->f_flags.

Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c    | 3 ++-
 fs/notify/fsnotify.c             | 2 +-
 include/linux/fanotify.h         | 2 +-
 include/linux/fsnotify.h         | 2 ++
 include/linux/fsnotify_backend.h | 7 +++++--
 include/uapi/linux/fanotify.h    | 1 +
 6 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 0a09950317dd..e30f3a1d9699 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -209,8 +209,9 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 	BUILD_BUG_ON(FAN_OPEN_PERM != FS_OPEN_PERM);
 	BUILD_BUG_ON(FAN_ACCESS_PERM != FS_ACCESS_PERM);
 	BUILD_BUG_ON(FAN_ONDIR != FS_ISDIR);
+	BUILD_BUG_ON(FAN_OPEN_EXEC != FS_OPEN_EXEC);
 
-	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 10);
+	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 11);
 
 	mask = fanotify_group_event_mask(iter_info, mask, data, data_type);
 	if (!mask)
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index d2c34900ae05..b3f58f36a0ab 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -401,7 +401,7 @@ static __init int fsnotify_init(void)
 {
 	int ret;
 
-	BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 23);
+	BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 24);
 
 	ret = init_srcu_struct(&fsnotify_mark_srcu);
 	if (ret)
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index a5a60691e48b..c521e4264f2b 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -37,7 +37,7 @@
 
 /* Events that user can request to be notified on */
 #define FANOTIFY_EVENTS		(FAN_ACCESS | FAN_MODIFY | \
-				 FAN_CLOSE | FAN_OPEN)
+				 FAN_CLOSE | FAN_OPEN | FAN_OPEN_EXEC)
 
 /* Events that require a permission response from user */
 #define FANOTIFY_PERM_EVENTS	(FAN_OPEN_PERM | FAN_ACCESS_PERM)
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index fd1ce10553bf..1fe5ac93b252 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -215,6 +215,8 @@ static inline void fsnotify_open(struct file *file)
 
 	if (S_ISDIR(inode->i_mode))
 		mask |= FS_ISDIR;
+	if (file->f_flags & __FMODE_EXEC)
+		mask |= FS_OPEN_EXEC;
 
 	fsnotify_parent(path, NULL, mask);
 	fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 135b973e44d1..39d94e62a836 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -38,6 +38,7 @@
 #define FS_DELETE		0x00000200	/* Subfile was deleted */
 #define FS_DELETE_SELF		0x00000400	/* Self was deleted */
 #define FS_MOVE_SELF		0x00000800	/* Self was moved */
+#define FS_OPEN_EXEC		0x00001000	/* File was opened for exec */
 
 #define FS_UNMOUNT		0x00002000	/* inode on umount fs */
 #define FS_Q_OVERFLOW		0x00004000	/* Event queued overflowed */
@@ -62,7 +63,8 @@
 #define FS_EVENTS_POSS_ON_CHILD   (FS_ACCESS | FS_MODIFY | FS_ATTRIB |\
 				   FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | FS_OPEN |\
 				   FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE |\
-				   FS_DELETE | FS_OPEN_PERM | FS_ACCESS_PERM)
+				   FS_DELETE | FS_OPEN_PERM | FS_ACCESS_PERM | \
+				   FS_OPEN_EXEC)
 
 #define FS_MOVE			(FS_MOVED_FROM | FS_MOVED_TO)
 
@@ -74,7 +76,8 @@
 			     FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE | \
 			     FS_DELETE | FS_DELETE_SELF | FS_MOVE_SELF | \
 			     FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | \
-			     FS_OPEN_PERM | FS_ACCESS_PERM | FS_DN_RENAME)
+			     FS_OPEN_PERM | FS_ACCESS_PERM | FS_DN_RENAME | \
+			     FS_OPEN_EXEC)
 
 /* Extra flags that may be reported with event or control handling of events */
 #define ALL_FSNOTIFY_FLAGS  (FS_EXCL_UNLINK | FS_ISDIR | FS_IN_ONESHOT | \
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index b86740d1c50a..d9664fbc905b 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -10,6 +10,7 @@
 #define FAN_CLOSE_WRITE		0x00000008	/* Writtable file closed */
 #define FAN_CLOSE_NOWRITE	0x00000010	/* Unwrittable file closed */
 #define FAN_OPEN		0x00000020	/* File was opened */
+#define FAN_OPEN_EXEC		0x00001000	/* File was opened for exec */
 
 #define FAN_Q_OVERFLOW		0x00004000	/* Event queued overflowed */
 
-- 
2.17.2


-- 
Matthew Bobrowski

[PATCH v7 3/4] fsnotify: refactor fsnotify_parent()/fsnotify() paired calls when event is on path

[Matthew Bobrowski]

A wrapper function fsnotify_path() has been defined to simplify the
paired calls to fsnotify_parent()/fsnotify(). All hooks that made use
these paired calls and passed FSNOTIFY_EVENT_PATH have been updated
accordingly.

Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/fsnotify.h | 42 +++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 1fe5ac93b252..c29f2f072c2c 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -26,13 +26,26 @@ static inline int fsnotify_parent(const struct path *path, struct dentry *dentry
 	return __fsnotify_parent(path, dentry, mask);
 }
 
+/*
+ * Simple wrapper to consolidate calls fsnotify_parent()/fsnotify() when
+ * an event is on a path.
+ */
+static inline int fsnotify_path(struct inode *inode, const struct path *path,
+				__u32 mask)
+{
+	int ret = fsnotify_parent(path, NULL, mask);
+
+	if (ret)
+		return ret;
+	return fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
+}
+
 /* simple call site for access decisions */
 static inline int fsnotify_perm(struct file *file, int mask)
 {
 	const struct path *path = &file->f_path;
 	struct inode *inode = file_inode(file);
 	__u32 fsnotify_mask = 0;
-	int ret;
 
 	if (file->f_mode & FMODE_NONOTIFY)
 		return 0;
@@ -45,11 +58,7 @@ static inline int fsnotify_perm(struct file *file, int mask)
 	else
 		BUG();
 
-	ret = fsnotify_parent(path, NULL, fsnotify_mask);
-	if (ret)
-		return ret;
-
-	return fsnotify(inode, fsnotify_mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
+	return fsnotify_path(inode, path, fsnotify_mask);
 }
 
 /*
@@ -180,10 +189,8 @@ static inline void fsnotify_access(struct file *file)
 	if (S_ISDIR(inode->i_mode))
 		mask |= FS_ISDIR;
 
-	if (!(file->f_mode & FMODE_NONOTIFY)) {
-		fsnotify_parent(path, NULL, mask);
-		fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
-	}
+	if (!(file->f_mode & FMODE_NONOTIFY))
+		fsnotify_path(inode, path, mask);
 }
 
 /*
@@ -198,10 +205,8 @@ static inline void fsnotify_modify(struct file *file)
 	if (S_ISDIR(inode->i_mode))
 		mask |= FS_ISDIR;
 
-	if (!(file->f_mode & FMODE_NONOTIFY)) {
-		fsnotify_parent(path, NULL, mask);
-		fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
-	}
+	if (!(file->f_mode & FMODE_NONOTIFY))
+		fsnotify_path(inode, path, mask);
 }
 
 /*
@@ -218,8 +223,7 @@ static inline void fsnotify_open(struct file *file)
 	if (file->f_flags & __FMODE_EXEC)
 		mask |= FS_OPEN_EXEC;
 
-	fsnotify_parent(path, NULL, mask);
-	fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
+	fsnotify_path(inode, path, mask);
 }
 
 /*
@@ -235,10 +239,8 @@ static inline void fsnotify_close(struct file *file)
 	if (S_ISDIR(inode->i_mode))
 		mask |= FS_ISDIR;
 
-	if (!(file->f_mode & FMODE_NONOTIFY)) {
-		fsnotify_parent(path, NULL, mask);
-		fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
-	}
+	if (!(file->f_mode & FMODE_NONOTIFY))
+		fsnotify_path(inode, path, mask);
 }
 
 /*
-- 
2.17.2


-- 
Matthew Bobrowski

[PATCH v7 4/4] fanotify: introduce new event mask FAN_OPEN_EXEC_PERM

[Matthew Bobrowski]

A new event mask FAN_OPEN_EXEC_PERM has been defined. This allows users
to receive events and grant access to files that are intending to be
opened for execution. Events of FAN_OPEN_EXEC_PERM type will be
generated when a file has been opened by using either execve(),
execveat() or uselib() system calls.

This acts in the same manner as previous permission event maks, meaning
that an access response is required from the user application in order
to permit any further operations on the file. This feature is
implemented within fsnotify_perm() hook by setting the
FAN_OPEN_EXEC_PERM mask if __FMODE_EXEC is set within file->f_flags.

Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c    |  3 ++-
 fs/notify/fsnotify.c             |  2 +-
 include/linux/fanotify.h         |  3 ++-
 include/linux/fsnotify.h         | 17 ++++++++++++-----
 include/linux/fsnotify_backend.h |  8 +++++---
 include/uapi/linux/fanotify.h    |  1 +
 6 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index e30f3a1d9699..d9aa505591eb 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -210,8 +210,9 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 	BUILD_BUG_ON(FAN_ACCESS_PERM != FS_ACCESS_PERM);
 	BUILD_BUG_ON(FAN_ONDIR != FS_ISDIR);
 	BUILD_BUG_ON(FAN_OPEN_EXEC != FS_OPEN_EXEC);
+	BUILD_BUG_ON(FAN_OPEN_EXEC_PERM != FS_OPEN_EXEC_PERM);
 
-	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 11);
+	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 12);
 
 	mask = fanotify_group_event_mask(iter_info, mask, data, data_type);
 	if (!mask)
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index b3f58f36a0ab..ecf09b6243d9 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -401,7 +401,7 @@ static __init int fsnotify_init(void)
 {
 	int ret;
 
-	BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 24);
+	BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 25);
 
 	ret = init_srcu_struct(&fsnotify_mark_srcu);
 	if (ret)
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index c521e4264f2b..9e2142795335 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -40,7 +40,8 @@
 				 FAN_CLOSE | FAN_OPEN | FAN_OPEN_EXEC)
 
 /* Events that require a permission response from user */
-#define FANOTIFY_PERM_EVENTS	(FAN_OPEN_PERM | FAN_ACCESS_PERM)
+#define FANOTIFY_PERM_EVENTS	(FAN_OPEN_PERM | FAN_ACCESS_PERM | \
+				 FAN_OPEN_EXEC_PERM)
 
 /* Extra flags that may be reported with event or control handling of events */
 #define FANOTIFY_EVENT_FLAGS	(FAN_EVENT_ON_CHILD | FAN_ONDIR)
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index c29f2f072c2c..2ccb08cb5d6a 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -40,9 +40,10 @@ static inline int fsnotify_path(struct inode *inode, const struct path *path,
 	return fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
 }
 
-/* simple call site for access decisions */
+/* Simple call site for access decisions */
 static inline int fsnotify_perm(struct file *file, int mask)
 {
+	int ret;
 	const struct path *path = &file->f_path;
 	struct inode *inode = file_inode(file);
 	__u32 fsnotify_mask = 0;
@@ -51,12 +52,18 @@ static inline int fsnotify_perm(struct file *file, int mask)
 		return 0;
 	if (!(mask & (MAY_READ | MAY_OPEN)))
 		return 0;
-	if (mask & MAY_OPEN)
+	if (mask & MAY_OPEN) {
 		fsnotify_mask = FS_OPEN_PERM;
-	else if (mask & MAY_READ)
+
+		if (file->f_flags & __FMODE_EXEC) {
+			ret = fsnotify_path(inode, path, FS_OPEN_EXEC_PERM);
+
+			if (ret)
+				return ret;
+		}
+	} else if (mask & MAY_READ) {
 		fsnotify_mask = FS_ACCESS_PERM;
-	else
-		BUG();
+	}
 
 	return fsnotify_path(inode, path, fsnotify_mask);
 }
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 39d94e62a836..7639774e7475 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -46,6 +46,7 @@
 
 #define FS_OPEN_PERM		0x00010000	/* open event in an permission hook */
 #define FS_ACCESS_PERM		0x00020000	/* access event in a permissions hook */
+#define FS_OPEN_EXEC_PERM	0x00040000	/* open/exec event in a permission hook */
 
 #define FS_EXCL_UNLINK		0x04000000	/* do not send events if object is unlinked */
 #define FS_ISDIR		0x40000000	/* event occurred against dir */
@@ -64,11 +65,12 @@
 				   FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | FS_OPEN |\
 				   FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE |\
 				   FS_DELETE | FS_OPEN_PERM | FS_ACCESS_PERM | \
-				   FS_OPEN_EXEC)
+				   FS_OPEN_EXEC | FS_OPEN_EXEC_PERM)
 
 #define FS_MOVE			(FS_MOVED_FROM | FS_MOVED_TO)
 
-#define ALL_FSNOTIFY_PERM_EVENTS (FS_OPEN_PERM | FS_ACCESS_PERM)
+#define ALL_FSNOTIFY_PERM_EVENTS (FS_OPEN_PERM | FS_ACCESS_PERM | \
+				  FS_OPEN_EXEC_PERM)
 
 /* Events that can be reported to backends */
 #define ALL_FSNOTIFY_EVENTS (FS_ACCESS | FS_MODIFY | FS_ATTRIB | \
@@ -77,7 +79,7 @@
 			     FS_DELETE | FS_DELETE_SELF | FS_MOVE_SELF | \
 			     FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | \
 			     FS_OPEN_PERM | FS_ACCESS_PERM | FS_DN_RENAME | \
-			     FS_OPEN_EXEC)
+			     FS_OPEN_EXEC | FS_OPEN_EXEC_PERM)
 
 /* Extra flags that may be reported with event or control handling of events */
 #define ALL_FSNOTIFY_FLAGS  (FS_EXCL_UNLINK | FS_ISDIR | FS_IN_ONESHOT | \
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index d9664fbc905b..909c98fcace2 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -16,6 +16,7 @@
 
 #define FAN_OPEN_PERM		0x00010000	/* File open in perm check */
 #define FAN_ACCESS_PERM		0x00020000	/* File accessed in perm check */
+#define FAN_OPEN_EXEC_PERM	0x00040000	/* File open/exec in perm check */
 
 #define FAN_ONDIR		0x40000000	/* event occurred against dir */
 
-- 
2.17.2


-- 
Matthew Bobrowski

Re: [PATCH v7 2/4] fanotify: introduce new event mask FAN_OPEN_EXEC

[Andy Lutomirski]

On Wed, Nov 7, 2018 at 7:07 PM Matthew Bobrowski
<mbobrowski@mbobrowski.org> wrote:
>
> A new event mask FAN_OPEN_EXEC has been defined so that users have the
> ability to receive events specifically when a file has been opened with
> the intent to be executed. Events of FAN_OPEN_EXEC type will be
> generated when a file has been opened using either execve(), execveat()
> or uselib() system calls.
>
> The feature is implemented within fsnotify_open() by generating the
> FAN_OPEN_EXEC event type if __FMODE_EXEC is set within file->f_flags.
>

I think this needs some clarification.  In particular:

Do current kernels generate some other fanotify on execve or do they
generate no event at all?

What is the intended use case?

What semantics do you provide for the opening of the ELF loader?  Are
those semantics useful?

How are users of this mechanism expected to handle DSOs?

--Andy

Re: [PATCH v7 2/4] fanotify: introduce new event mask FAN_OPEN_EXEC

[Matthew Bobrowski]

On Thu, Nov 08, 2018 at 10:22:50AM -0800, Andy Lutomirski wrote:
> On Wed, Nov 7, 2018 at 7:07 PM Matthew Bobrowski
> <mbobrowski@mbobrowski.org> wrote:
> >
> > A new event mask FAN_OPEN_EXEC has been defined so that users have the
> > ability to receive events specifically when a file has been opened with
> > the intent to be executed. Events of FAN_OPEN_EXEC type will be
> > generated when a file has been opened using either execve(), execveat()
> > or uselib() system calls.
> >
> > The feature is implemented within fsnotify_open() by generating the
> > FAN_OPEN_EXEC event type if __FMODE_EXEC is set within file->f_flags.
> >
> 
> I think this needs some clarification.  In particular:

OK, sure.

> Do current kernels generate some other fanotify on execve or do they
> generate no event at all?

Yes, it does currently generate events on execve. Due to the nature of
how this particular system call works, the API, as is, will generate a
number of FAN_OPEN and FAN_ACCESS events.
 
> What is the intended use case?

For our particular use case, this is to greatly assist with an auditing
application that we're in the midst of developing. More specifically
though, it is to aid with providing additional context around why the
marked object(s) is being opened. We're interested to understand when
the direct execution of a file occurs via execve() or execveat(), for
example. This becomes exceptionally helpful on a busy filesystem when
you're trying to sift through and correlate FAN_OPEN and FAN_ACCESS
events while having marks placed on either a mount(s) or superblock(s).

> What semantics do you provide for the opening of the ELF loader?  Are
> those semantics useful?

I don't exactly understand what you mean by providing semantics around
the opening of the ELF loader. However, I'm going to work with the
assumption that you're referring to how this particular event mask works
with the implicit invocation of the ELF loader when an ELF program is
being prepared for execution? If that's the case, it's quite simple. If
the ELF loader has been marked to receive events of this type, then an
event will also be generated for the ELF loader when an ELF program is
invoked via execve. If the ELF loader has not been marked, then only the
event for the ELF program itself will be generated. 

If I've misunderstood what you're referring to, please kindly elaborate.
 
> How are users of this mechanism expected to handle DSOs?

Well, if they're concerned about the direct execution of a shared
library, then they'd just place a mark on it using this mask. Generally
speaking though, I can't see that being particularly useful seeing as
though DSOs in most cases are not actually directly executed per se, but
rather opened, read and then mapped into the process address space. So,
if they're concerned with handling DSOs, then it's the users discretion
about whether they mark it and what type of mark is to be placed on the
DSO object itself.

-- 
Matthew Bobrowski

Re: [PATCH v7 2/4] fanotify: introduce new event mask FAN_OPEN_EXEC

[Andy Lutomirski]

On Thu, Nov 8, 2018 at 9:41 PM Matthew Bobrowski
<mbobrowski@mbobrowski.org> wrote:
>
> On Thu, Nov 08, 2018 at 10:22:50AM -0800, Andy Lutomirski wrote:
> > On Wed, Nov 7, 2018 at 7:07 PM Matthew Bobrowski
> > <mbobrowski@mbobrowski.org> wrote:
> > >
> > > A new event mask FAN_OPEN_EXEC has been defined so that users have the
> > > ability to receive events specifically when a file has been opened with
> > > the intent to be executed. Events of FAN_OPEN_EXEC type will be
> > > generated when a file has been opened using either execve(), execveat()
> > > or uselib() system calls.
> > >
> > > The feature is implemented within fsnotify_open() by generating the
> > > FAN_OPEN_EXEC event type if __FMODE_EXEC is set within file->f_flags.
> > >
> >
> > I think this needs some clarification.  In particular:
>
> OK, sure.
>
> > Do current kernels generate some other fanotify on execve or do they
> > generate no event at all?
>
> Yes, it does currently generate events on execve. Due to the nature of
> how this particular system call works, the API, as is, will generate a
> number of FAN_OPEN and FAN_ACCESS events.
>
> > What is the intended use case?
>
> For our particular use case, this is to greatly assist with an auditing
> application that we're in the midst of developing. More specifically
> though, it is to aid with providing additional context around why the
> marked object(s) is being opened. We're interested to understand when
> the direct execution of a file occurs via execve() or execveat(), for
> example. This becomes exceptionally helpful on a busy filesystem when
> you're trying to sift through and correlate FAN_OPEN and FAN_ACCESS
> events while having marks placed on either a mount(s) or superblock(s).

Seems reasonable.

>
> > What semantics do you provide for the opening of the ELF loader?  Are
> > those semantics useful?
>
> I don't exactly understand what you mean by providing semantics around
> the opening of the ELF loader. However, I'm going to work with the
> assumption that you're referring to how this particular event mask works
> with the implicit invocation of the ELF loader when an ELF program is
> being prepared for execution? If that's the case, it's quite simple. If
> the ELF loader has been marked to receive events of this type, then an
> event will also be generated for the ELF loader when an ELF program is
> invoked via execve. If the ELF loader has not been marked, then only the
> event for the ELF program itself will be generated.

OK.  You should probably add to your documentation that interpreters
opened as a result of execve() and execveat() also set FAN_OPEN_EXEC.

>
> If I've misunderstood what you're referring to, please kindly elaborate.
>
> > How are users of this mechanism expected to handle DSOs?
>
> Well, if they're concerned about the direct execution of a shared
> library, then they'd just place a mark on it using this mask. Generally
> speaking though, I can't see that being particularly useful seeing as
> though DSOs in most cases are not actually directly executed per se, but
> rather opened, read and then mapped into the process address space. So,
> if they're concerned with handling DSOs, then it's the users discretion
> about whether they mark it and what type of mark is to be placed on the
> DSO object itself.

Are you sure?  Because I don't think that DSOs actually get
__FMODE_EXEC set.  So I expect that, if you do:

$ /bin/echo foo

then you'll get FAN_OPEN_EXEC.  If, on the other hand, you do:

$ /lib64/ld-linux-x86-64.so.2 /bin/echo foo

then I think you will *not* get FAN_OPEN_EXEC.

Re: [PATCH v7 2/4] fanotify: introduce new event mask FAN_OPEN_EXEC

[Matthew Bobrowski]

On Thu, Nov 08, 2018 at 10:04:08PM -0800, Andy Lutomirski wrote:
> On Thu, Nov 8, 2018 at 9:41 PM Matthew Bobrowski
> <mbobrowski@mbobrowski.org> wrote:
> >
> > On Thu, Nov 08, 2018 at 10:22:50AM -0800, Andy Lutomirski wrote:
> > > On Wed, Nov 7, 2018 at 7:07 PM Matthew Bobrowski
> > > <mbobrowski@mbobrowski.org> wrote:
> > > >
> > > > A new event mask FAN_OPEN_EXEC has been defined so that users have the
> > > > ability to receive events specifically when a file has been opened with
> > > > the intent to be executed. Events of FAN_OPEN_EXEC type will be
> > > > generated when a file has been opened using either execve(), execveat()
> > > > or uselib() system calls.
> > > >
> > > > The feature is implemented within fsnotify_open() by generating the
> > > > FAN_OPEN_EXEC event type if __FMODE_EXEC is set within file->f_flags.
> > > >
> > >
> > > I think this needs some clarification.  In particular:
> >
> > OK, sure.
> >
> > > Do current kernels generate some other fanotify on execve or do they
> > > generate no event at all?
> >
> > Yes, it does currently generate events on execve. Due to the nature of
> > how this particular system call works, the API, as is, will generate a
> > number of FAN_OPEN and FAN_ACCESS events.
> >
> > > What is the intended use case?
> >
> > For our particular use case, this is to greatly assist with an auditing
> > application that we're in the midst of developing. More specifically
> > though, it is to aid with providing additional context around why the
> > marked object(s) is being opened. We're interested to understand when
> > the direct execution of a file occurs via execve() or execveat(), for
> > example. This becomes exceptionally helpful on a busy filesystem when
> > you're trying to sift through and correlate FAN_OPEN and FAN_ACCESS
> > events while having marks placed on either a mount(s) or superblock(s).
> 
> Seems reasonable.
> 
> >
> > > What semantics do you provide for the opening of the ELF loader?  Are
> > > those semantics useful?
> >
> > I don't exactly understand what you mean by providing semantics around
> > the opening of the ELF loader. However, I'm going to work with the
> > assumption that you're referring to how this particular event mask works
> > with the implicit invocation of the ELF loader when an ELF program is
> > being prepared for execution? If that's the case, it's quite simple. If
> > the ELF loader has been marked to receive events of this type, then an
> > event will also be generated for the ELF loader when an ELF program is
> > invoked via execve. If the ELF loader has not been marked, then only the
> > event for the ELF program itself will be generated.
> 
> OK.  You should probably add to your documentation that interpreters
> opened as a result of execve() and execveat() also set FAN_OPEN_EXEC.

Sure, I can add that as a clarifying point to the documentation. 

> 
> >
> > If I've misunderstood what you're referring to, please kindly elaborate.
> >
> > > How are users of this mechanism expected to handle DSOs?
> >
> > Well, if they're concerned about the direct execution of a shared
> > library, then they'd just place a mark on it using this mask. Generally
> > speaking though, I can't see that being particularly useful seeing as
> > though DSOs in most cases are not actually directly executed per se, but
> > rather opened, read and then mapped into the process address space. So,
> > if they're concerned with handling DSOs, then it's the users discretion
> > about whether they mark it and what type of mark is to be placed on the
> > DSO object itself.
> 
> Are you sure?  Because I don't think that DSOs actually get
> __FMODE_EXEC set.  So I expect that, if you do:
> 
> $ /bin/echo foo
> 
> then you'll get FAN_OPEN_EXEC.  

Correct. If the marked object here was /bin/echo, then yes, doing
exactly that would result in a FAN_OPEN_EXEC as you're passing it to
execve, so __FMODE_EXEC is set in the open_flag accordingly.

> If, on the other hand, you do:
> 
> $ /lib64/ld-linux-x86-64.so.2 /bin/echo foo
> 
> then I think you will *not* get FAN_OPEN_EXEC.

Here, you're also correct. 

Remember though, FAN_OPEN_EXEC is set purely for an object that is
opened and contains __FMODE_EXEC in the open_flag. Thus, anything opened
via syscalls execve, execveat or uselib. In the above example, direct
execution via execve is performed on /lib64/ld-linux-x86-64.so.2 and the
object /bin/echo in this instance is passed to it as an argument. This 
results in an open/read !(open_flag & __FMODE_EXEC), as oppose to
execve. So here, providing that you have a mark placed on the loader,
you'd only get a FAN_OPEN_EXEC for that object and consequently nothing
for the program that has been passed to it as an argument. 

Events of type FAN_OPEN_EXEC will *not* be raised in the situation where
an interpreter reads data as input and subsequently results in arbitrary
computation. I've also made this explicitly clear in my supporting
documentation (man-pages). Not sure, whether this should also be added
to the changelog. Thoughts?

-- 
Matthew Bobrowski

Re: [PATCH v7 2/4] fanotify: introduce new event mask FAN_OPEN_EXEC

[Jan Kara]

On Thu 08-11-18 22:04:08, Andy Lutomirski wrote:
> On Thu, Nov 8, 2018 at 9:41 PM Matthew Bobrowski
> <mbobrowski@mbobrowski.org> wrote:
> >
> > On Thu, Nov 08, 2018 at 10:22:50AM -0800, Andy Lutomirski wrote:
> > > On Wed, Nov 7, 2018 at 7:07 PM Matthew Bobrowski
> > > <mbobrowski@mbobrowski.org> wrote:
> > > >
> > > > A new event mask FAN_OPEN_EXEC has been defined so that users have the
> > > > ability to receive events specifically when a file has been opened with
> > > > the intent to be executed. Events of FAN_OPEN_EXEC type will be
> > > > generated when a file has been opened using either execve(), execveat()
> > > > or uselib() system calls.
> > > >
> > > > The feature is implemented within fsnotify_open() by generating the
> > > > FAN_OPEN_EXEC event type if __FMODE_EXEC is set within file->f_flags.
> > > >
> > >
> > > I think this needs some clarification.  In particular:
> >
> > OK, sure.
> >
> > > Do current kernels generate some other fanotify on execve or do they
> > > generate no event at all?
> >
> > Yes, it does currently generate events on execve. Due to the nature of
> > how this particular system call works, the API, as is, will generate a
> > number of FAN_OPEN and FAN_ACCESS events.
> >
> > > What is the intended use case?
> >
> > For our particular use case, this is to greatly assist with an auditing
> > application that we're in the midst of developing. More specifically
> > though, it is to aid with providing additional context around why the
> > marked object(s) is being opened. We're interested to understand when
> > the direct execution of a file occurs via execve() or execveat(), for
> > example. This becomes exceptionally helpful on a busy filesystem when
> > you're trying to sift through and correlate FAN_OPEN and FAN_ACCESS
> > events while having marks placed on either a mount(s) or superblock(s).
> 
> Seems reasonable.
> 
> >
> > > What semantics do you provide for the opening of the ELF loader?  Are
> > > those semantics useful?
> >
> > I don't exactly understand what you mean by providing semantics around
> > the opening of the ELF loader. However, I'm going to work with the
> > assumption that you're referring to how this particular event mask works
> > with the implicit invocation of the ELF loader when an ELF program is
> > being prepared for execution? If that's the case, it's quite simple. If
> > the ELF loader has been marked to receive events of this type, then an
> > event will also be generated for the ELF loader when an ELF program is
> > invoked via execve. If the ELF loader has not been marked, then only the
> > event for the ELF program itself will be generated.
> 
> OK.  You should probably add to your documentation that interpreters
> opened as a result of execve() and execveat() also set FAN_OPEN_EXEC.

I'm not sure I understand your concern (and thus need for documentation).
In the following I assume you watch the whole system for fanotify events
(you can restrict them to specific files / mount points / superblocks
but that's besides the point of this discussion).
If you do:

~> /bin/echo

Then you get FAN_OPEN_EXEC event for '/bin/echo' file and nothing more.
Similarly if you do:
~> cat /tmp/test.sh
#!/bin/bash
/bin/echo
~> /tmp/test.sh

You will get FAN_OPEN_EXEC events for /tmp/test.sh and /bin/echo and
nothing more.

If you do:

~> /bin/sh /tmp/test.sh

You will get FAN_OPEN_EXEC events for /bin/sh and /bin/echo and
nothing more.

I other words generated FAN_OPEN_EXEC events 1:1 correspond to the first
argument of the execve(2) syscall. Nothing less and nothing more.

> > If I've misunderstood what you're referring to, please kindly elaborate.
> >
> > > How are users of this mechanism expected to handle DSOs?
> >
> > Well, if they're concerned about the direct execution of a shared
> > library, then they'd just place a mark on it using this mask. Generally
> > speaking though, I can't see that being particularly useful seeing as
> > though DSOs in most cases are not actually directly executed per se, but
> > rather opened, read and then mapped into the process address space. So,
> > if they're concerned with handling DSOs, then it's the users discretion
> > about whether they mark it and what type of mark is to be placed on the
> > DSO object itself.
> 
> Are you sure?  Because I don't think that DSOs actually get
> __FMODE_EXEC set.  So I expect that, if you do:
> 
> $ /bin/echo foo
> 
> then you'll get FAN_OPEN_EXEC.  If, on the other hand, you do:
> 
> $ /lib64/ld-linux-x86-64.so.2 /bin/echo foo
> 
> then I think you will *not* get FAN_OPEN_EXEC.

Exactly and I think that was what Matthew was trying to tell. Generally
there are no FAN_OPEN_EXEC events for DSOs. Is it clearer now?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v7 2/4] fanotify: introduce new event mask FAN_OPEN_EXEC

[Andy Lutomirski]

> On Nov 12, 2018, at 8:14 AM, Jan Kara <jack@suse.cz> wrote:
> 
>> On Thu 08-11-18 22:04:08, Andy Lutomirski wrote:
>> On Thu, Nov 8, 2018 at 9:41 PM Matthew Bobrowski
>> <mbobrowski@mbobrowski.org> wrote:
>>> 
>>>> On Thu, Nov 08, 2018 at 10:22:50AM -0800, Andy Lutomirski wrote:
>>>> On Wed, Nov 7, 2018 at 7:07 PM Matthew Bobrowski
>>>> <mbobrowski@mbobrowski.org> wrote:
>>>>> 
>>>>> A new event mask FAN_OPEN_EXEC has been defined so that users have the
>>>>> ability to receive events specifically when a file has been opened with
>>>>> the intent to be executed. Events of FAN_OPEN_EXEC type will be
>>>>> generated when a file has been opened using either execve(), execveat()
>>>>> or uselib() system calls.
>>>>> 
>>>>> The feature is implemented within fsnotify_open() by generating the
>>>>> FAN_OPEN_EXEC event type if __FMODE_EXEC is set within file->f_flags.
>>>>> 
>>>> 
>>>> I think this needs some clarification.  In particular:
>>> 
>>> OK, sure.
>>> 
>>>> Do current kernels generate some other fanotify on execve or do they
>>>> generate no event at all?
>>> 
>>> Yes, it does currently generate events on execve. Due to the nature of
>>> how this particular system call works, the API, as is, will generate a
>>> number of FAN_OPEN and FAN_ACCESS events.
>>> 
>>>> What is the intended use case?
>>> 
>>> For our particular use case, this is to greatly assist with an auditing
>>> application that we're in the midst of developing. More specifically
>>> though, it is to aid with providing additional context around why the
>>> marked object(s) is being opened. We're interested to understand when
>>> the direct execution of a file occurs via execve() or execveat(), for
>>> example. This becomes exceptionally helpful on a busy filesystem when
>>> you're trying to sift through and correlate FAN_OPEN and FAN_ACCESS
>>> events while having marks placed on either a mount(s) or superblock(s).
>> 
>> Seems reasonable.
>> 
>>> 
>>>> What semantics do you provide for the opening of the ELF loader?  Are
>>>> those semantics useful?
>>> 
>>> I don't exactly understand what you mean by providing semantics around
>>> the opening of the ELF loader. However, I'm going to work with the
>>> assumption that you're referring to how this particular event mask works
>>> with the implicit invocation of the ELF loader when an ELF program is
>>> being prepared for execution? If that's the case, it's quite simple. If
>>> the ELF loader has been marked to receive events of this type, then an
>>> event will also be generated for the ELF loader when an ELF program is
>>> invoked via execve. If the ELF loader has not been marked, then only the
>>> event for the ELF program itself will be generated.
>> 
>> OK.  You should probably add to your documentation that interpreters
>> opened as a result of execve() and execveat() also set FAN_OPEN_EXEC.
> 
> I'm not sure I understand your concern (and thus need for documentation).
> In the following I assume you watch the whole system for fanotify events
> (you can restrict them to specific files / mount points / superblocks
> but that's besides the point of this discussion).
> If you do:
> 
> ~> /bin/echo
> 
> Then you get FAN_OPEN_EXEC event for '/bin/echo' file and nothing more.

If indeed that’s what the code does, then documenting it as such seems fine. But, by inspection, ELF interpreters are opened with open_exec(), so they should fire the event too. Am I wrong?

Re: [PATCH v7 2/4] fanotify: introduce new event mask FAN_OPEN_EXEC

[Matthew Bobrowski]

On Mon, Nov 12, 2018 at 08:37:25AM -0800, Andy Lutomirski wrote:
> >> OK.  You should probably add to your documentation that interpreters
> >> opened as a result of execve() and execveat() also set FAN_OPEN_EXEC.
> > 
> > I'm not sure I understand your concern (and thus need for documentation).
> > In the following I assume you watch the whole system for fanotify events
> > (you can restrict them to specific files / mount points / superblocks
> > but that's besides the point of this discussion).
> > If you do:
> > 
> > ~> /bin/echo
> > 
> > Then you get FAN_OPEN_EXEC event for '/bin/echo' file and nothing more.
> 
> If indeed that’s what the code does, then documenting it as such seems fine.
> But, by inspection, ELF interpreters are opened with open_exec(), so they
> should fire the event too. Am I wrong?

No, you're not wrong.

I do believe that there is no need to add a specific statement about
interpreters within the documentation.

-- 
Matthew Bobrowski

Re: [PATCH v7 2/4] fanotify: introduce new event mask FAN_OPEN_EXEC

[Jan Kara]

On Tue 13-11-18 22:45:28, Matthew Bobrowski wrote:
> On Mon, Nov 12, 2018 at 08:37:25AM -0800, Andy Lutomirski wrote:
> > >> OK.  You should probably add to your documentation that interpreters
> > >> opened as a result of execve() and execveat() also set FAN_OPEN_EXEC.
> > > 
> > > I'm not sure I understand your concern (and thus need for documentation).
> > > In the following I assume you watch the whole system for fanotify events
> > > (you can restrict them to specific files / mount points / superblocks
> > > but that's besides the point of this discussion).
> > > If you do:
> > > 
> > > ~> /bin/echo
> > > 
> > > Then you get FAN_OPEN_EXEC event for '/bin/echo' file and nothing more.
> > 
> > If indeed that’s what the code does, then documenting it as such seems fine.
> > But, by inspection, ELF interpreters are opened with open_exec(), so they
> > should fire the event too. Am I wrong?
> 
> No, you're not wrong.
> 
> I do believe that there is no need to add a specific statement about
> interpreters within the documentation.

So I think what Andy means is that if I watch / for FAN_OPEN_EXEC, then
people may not immediately realize that if they do /bin/echo, they'll
actually get events for

/bin/echo
/lib64/ld-2.22.so

At least I didn't immediately realize that (and just compiled test kernel
with your patches to verify). So I think this clarification would be worth
it as a note in the manpage. Changelog can IMO stay as is.

								Honza 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v7 1/4] fanotify: return only user requested event types in event mask

[Jan Kara]

On Thu 08-11-18 14:05:49, Matthew Bobrowski wrote:
> @@ -131,11 +137,7 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
>  	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
>  		return false;

Looking into this before merge, this hunk has apparently slipped during
some rebase (missing false->0 conversion).

> -	if (event_mask & FANOTIFY_OUTGOING_EVENTS &
> -	    marks_mask & ~marks_ignored_mask)
> -		return true;
> -
> -	return false;
> +	return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask;

And here we miss & ~marks_ignored_mask, right?

I've changed both in the patch I've merged but I'm checking just to be
sure...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v7 1/4] fanotify: return only user requested event types in event mask

[Amir Goldstein]

On Tue, Nov 13, 2018 at 7:38 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 08-11-18 14:05:49, Matthew Bobrowski wrote:
> > @@ -131,11 +137,7 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
> >           !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
> >               return false;
>
> Looking into this before merge, this hunk has apparently slipped during
> some rebase (missing false->0 conversion).
>
> > -     if (event_mask & FANOTIFY_OUTGOING_EVENTS &
> > -         marks_mask & ~marks_ignored_mask)
> > -             return true;
> > -
> > -     return false;
> > +     return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask;
>
> And here we miss & ~marks_ignored_mask, right?
>
> I've changed both in the patch I've merged but I'm checking just to be
> sure...
>
FWIW, changes look correct to me.

Thanks,
Amir.

Re: [PATCH v7 0/4] fanotify: introduce new event mask FAN_OPEN_EXEC and FAN_OPEN_EXEC_PERM

[Jan Kara]

On Thu 08-11-18 14:04:10, Matthew Bobrowski wrote:
> Currently, the fanotify API does not provide a means for user space
> applications to receive events when a file has been opened specifically
> for execution. New event masks FAN_OPEN_EXEC and FAN_OPEN_EXEC_PERM have
> been introduced in order to provide users with this capability.
> 
> These event types, when either are explicitly requested by the user, will
> be returned within the event mask when a marked file object is being
> opened has __FMODE_EXEC set as one of the flags for open_flag.
> 
> Linux is used as an operating system in some products, with an environment
> that can be certified under the Common Criteria Operating System
> Protection Profile (OSPP). This is a formal threat model for a class of
> technology. It requires specific countermeasures to mitigate threats. It
> requires documentation to explain how a product implements these
> countermeasures. It requires proof via a test suite to demonstrate that
> the requirements are met, observed and checked by an independent qualified
> third party. The latest set of requirements for OSPP v4.2 can be found
> here:
> 
> https://www.niap-ccevs.org/Profile/Info.cfm?PPID=424&id=424
> 
> If you look on page 58, you will see the following requirement:
> 
> FPT_SRP_EXT.1 Software Restriction Policies FPT_SRP_EXT.1.1
> administrator specified [selection:
>         file path,
>         file digital signature,
>         version,
>         hash,
>         [assignment: other characteristics]
> ]
> 
> This patch is to help aid in meeting this requirement.
> 
> Changes since v6:
> 	* Reordered patches within the patch series for the sake of
> 	  correctness.
> 	* Updated patch that contains FAN_OPEN_EXEC_PERM functionality to
> 	  include separate call to fsnotify_parent()/fsnotify() which is
> 	  used to mitigate merging of permission events.
> 
> Note that this set of patches are based on Amir's fsnotify-fixes branch,
> which has already been posted through for review. For those interested,
> this branch can be found here:
> https://github.com/amir73il/linux/commits/fsnotify-fixes
> 
> LTP tests for this feature are on my 'fanotify-exec' branch here:
> https://github.com/matthewbobrowski/ltp/commits/fanotify_exec. The files
> that contains the test cases are provided below:
> 
> syscalls/fanotify03: test cases have been updated to cover
>                      FAN_OPEN_EXEC_PERM events
> syscalls/fanotify12: newly introduced LTP test file to cover
>                      FAN_OPEN_EXEC events

I have been wondering for a while why the testcases passed when ignore mask
hasn't been properly treated in fanotify_group_event_mask() but then I
realized that the generic code will not even call to fanotify if ignore
masks result in clearing the event. Anyway, the rest of the series looks OK
(I've updated the changelog of 4/4 as requested by Amir) and tests pass
fine so once I get confirmation from you regarding 1/4, I'll push
everything out.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v7 0/4] fanotify: introduce new event mask FAN_OPEN_EXEC and FAN_OPEN_EXEC_PERM

[Amir Goldstein]

> > LTP tests for this feature are on my 'fanotify-exec' branch here:
> > https://github.com/matthewbobrowski/ltp/commits/fanotify_exec. The files
> > that contains the test cases are provided below:
> >
> > syscalls/fanotify03: test cases have been updated to cover
> >                      FAN_OPEN_EXEC_PERM events
> > syscalls/fanotify12: newly introduced LTP test file to cover
> >                      FAN_OPEN_EXEC events
>
> I have been wondering for a while why the testcases passed when ignore mask
> hasn't been properly treated in fanotify_group_event_mask() but then I
> realized that the generic code will not even call to fanotify if ignore
> masks result in clearing the event.

So does that means we have missing test coverage?

I think the idea of this patch was that
FAN_MARK_INODE, FAN_OPEN | FAN_OPEN_EXEC
+
FAN_MARK_MOUNT,  FAN_MARK_IGNORED_MASK | FAN_OPEN_EXEC

Will result with event with mask FAN_OPEN without FAN_OPEN_EXEC
in spite the implementation of patch 2/4 using  mask |= FS_OPEN_EXEC.

No?

Thanks,
Amir.

Re: [PATCH v7 2/4] fanotify: introduce new event mask FAN_OPEN_EXEC

[Matthew Bobrowski]

On Tue, Nov 13, 2018 at 06:35:03PM +0100, Jan Kara wrote:
> > > >> OK.  You should probably add to your documentation that interpreters
> > > >> opened as a result of execve() and execveat() also set FAN_OPEN_EXEC.
> > > > 
> > > > I'm not sure I understand your concern (and thus need for documentation).
> > > > In the following I assume you watch the whole system for fanotify events
> > > > (you can restrict them to specific files / mount points / superblocks
> > > > but that's besides the point of this discussion).
> > > > If you do:
> > > > 
> > > > ~> /bin/echo
> > > > 
> > > > Then you get FAN_OPEN_EXEC event for '/bin/echo' file and nothing more.
> > > 
> > > If indeed that’s what the code does, then documenting it as such seems fine.
> > > But, by inspection, ELF interpreters are opened with open_exec(), so they
> > > should fire the event too. Am I wrong?
> > 
> > No, you're not wrong.
> > 
> > I do believe that there is no need to add a specific statement about
> > interpreters within the documentation.
> 
> So I think what Andy means is that if I watch / for FAN_OPEN_EXEC, then
> people may not immediately realize that if they do /bin/echo, they'll
> actually get events for
> 
> /bin/echo
> /lib64/ld-2.22.so
> 
> At least I didn't immediately realize that (and just compiled test kernel
> with your patches to verify). So I think this clarification would be worth
> it as a note in the manpage. Changelog can IMO stay as is.

OK, sure, I will add it.

-- 
Matthew Bobrowski

Re: [PATCH v7 1/4] fanotify: return only user requested event types in event mask

[Matthew Bobrowski]

On Tue, Nov 13, 2018 at 07:53:07PM +0200, Amir Goldstein wrote:
> > > @@ -131,11 +137,7 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
> > >           !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
> > >               return false;
> >
> > Looking into this before merge, this hunk has apparently slipped during
> > some rebase (missing false->0 conversion).
> >
> > > -     if (event_mask & FANOTIFY_OUTGOING_EVENTS &
> > > -         marks_mask & ~marks_ignored_mask)
> > > -             return true;
> > > -
> > > -     return false;
> > > +     return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask;
> >
> > And here we miss & ~marks_ignored_mask, right?
> >
> > I've changed both in the patch I've merged but I'm checking just to be
> > sure...
> >
> FWIW, changes look correct to me.

Yes, that would be right Jan. 

-- 
Matthew Bobrowski

Re: [PATCH v7 0/4] fanotify: introduce new event mask FAN_OPEN_EXEC and FAN_OPEN_EXEC_PERM

[Amir Goldstein]

On Tue, Nov 13, 2018 at 8:01 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> > > LTP tests for this feature are on my 'fanotify-exec' branch here:
> > > https://github.com/matthewbobrowski/ltp/commits/fanotify_exec. The files
> > > that contains the test cases are provided below:
> > >
> > > syscalls/fanotify03: test cases have been updated to cover
> > >                      FAN_OPEN_EXEC_PERM events
> > > syscalls/fanotify12: newly introduced LTP test file to cover
> > >                      FAN_OPEN_EXEC events
> >
> > I have been wondering for a while why the testcases passed when ignore mask
> > hasn't been properly treated in fanotify_group_event_mask() but then I
> > realized that the generic code will not even call to fanotify if ignore
> > masks result in clearing the event.
>
> So does that means we have missing test coverage?
>

This is covered by test case #3 of Matthew's proposed LTP test.
https://github.com/matthewbobrowski/ltp/commit/9e350fe15a5423d896ed0e8e147edc15bee13b42#diff-2bb8ddff24b3a031be0f64354262e587R76

> I think the idea of this patch was that
> FAN_MARK_INODE, FAN_OPEN | FAN_OPEN_EXEC
> +
> FAN_MARK_MOUNT,  FAN_MARK_IGNORED_MASK | FAN_OPEN_EXEC
>

Not even mount mark. ignore mask on the same inode mark as well.

Thanks,
Amir.

Re: [PATCH v7 0/4] fanotify: introduce new event mask FAN_OPEN_EXEC and FAN_OPEN_EXEC_PERM

[Jan Kara]

On Wed 14-11-18 05:43:25, Amir Goldstein wrote:
> On Tue, Nov 13, 2018 at 8:01 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > > > LTP tests for this feature are on my 'fanotify-exec' branch here:
> > > > https://github.com/matthewbobrowski/ltp/commits/fanotify_exec. The files
> > > > that contains the test cases are provided below:
> > > >
> > > > syscalls/fanotify03: test cases have been updated to cover
> > > >                      FAN_OPEN_EXEC_PERM events
> > > > syscalls/fanotify12: newly introduced LTP test file to cover
> > > >                      FAN_OPEN_EXEC events
> > >
> > > I have been wondering for a while why the testcases passed when ignore mask
> > > hasn't been properly treated in fanotify_group_event_mask() but then I
> > > realized that the generic code will not even call to fanotify if ignore
> > > masks result in clearing the event.
> >
> > So does that means we have missing test coverage?
> >
> 
> This is covered by test case #3 of Matthew's proposed LTP test.
> https://github.com/matthewbobrowski/ltp/commit/9e350fe15a5423d896ed0e8e147edc15bee13b42#diff-2bb8ddff24b3a031be0f64354262e587R76

This testcase does not catch the bug we had in fanotify_group_event_mask()
because the masking by mark->mask already hides the fact that we failed to
apply the ignore mask.

What does catch this kind of bug (tested) is a testcase (admittedly
somewhat silly) like this:

{
        "inode mark, FAN_OPEN | FAN_OPEN_EXEC events, ignore FAN_OPEN_EXEC",
        INIT_FANOTIFY_MARK_TYPE(INODE),
        FAN_OPEN | FAN_OPEN_EXEC,
        FAN_OPEN_EXEC,
        2,
        {FAN_OPEN, FAN_OPEN}
},

A real variant of this would be FAN_OPEN | FAN_OPEN_EXEC on mount, ignore
FAN_OPEN on inode. Then we should just get one FAN_OPEN_EXEC but with the bug
we'd get FAN_OPEN | FAN_OPEN_EXEC.

But creating such test would be slightly more involved. But probably it is
worth it. Matthew?

Also I have noticed that fanotify12 test has a bug that it reports:

fanotify12.c:220: FAIL: Received event: mask=1020, pid=5142 (expected 5142), fd=5

i.e., it reports expected pid instead of expected mask when mask does not
match. Can you please fix it Matthew?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v7 1/4] fanotify: return only user requested event types in event mask

[Jan Kara]

On Wed 14-11-18 10:54:46, Matthew Bobrowski wrote:
> On Tue, Nov 13, 2018 at 07:53:07PM +0200, Amir Goldstein wrote:
> > > > @@ -131,11 +137,7 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
> > > >           !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
> > > >               return false;
> > >
> > > Looking into this before merge, this hunk has apparently slipped during
> > > some rebase (missing false->0 conversion).
> > >
> > > > -     if (event_mask & FANOTIFY_OUTGOING_EVENTS &
> > > > -         marks_mask & ~marks_ignored_mask)
> > > > -             return true;
> > > > -
> > > > -     return false;
> > > > +     return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask;
> > >
> > > And here we miss & ~marks_ignored_mask, right?
> > >
> > > I've changed both in the patch I've merged but I'm checking just to be
> > > sure...
> > >
> > FWIW, changes look correct to me.
> 
> Yes, that would be right Jan. 

Thanks for confirmation. Patches pushed out. Thanks for your work and also
to Amir for his review and comments!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v7 0/4] fanotify: introduce new event mask FAN_OPEN_EXEC and FAN_OPEN_EXEC_PERM

[Amir Goldstein]

On Wed, Nov 14, 2018 at 2:02 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 14-11-18 05:43:25, Amir Goldstein wrote:
> > On Tue, Nov 13, 2018 at 8:01 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > > > LTP tests for this feature are on my 'fanotify-exec' branch here:
> > > > > https://github.com/matthewbobrowski/ltp/commits/fanotify_exec. The files
> > > > > that contains the test cases are provided below:
> > > > >
> > > > > syscalls/fanotify03: test cases have been updated to cover
> > > > >                      FAN_OPEN_EXEC_PERM events
> > > > > syscalls/fanotify12: newly introduced LTP test file to cover
> > > > >                      FAN_OPEN_EXEC events
> > > >
> > > > I have been wondering for a while why the testcases passed when ignore mask
> > > > hasn't been properly treated in fanotify_group_event_mask() but then I
> > > > realized that the generic code will not even call to fanotify if ignore
> > > > masks result in clearing the event.
> > >
> > > So does that means we have missing test coverage?
> > >
> >
> > This is covered by test case #3 of Matthew's proposed LTP test.
> > https://github.com/matthewbobrowski/ltp/commit/9e350fe15a5423d896ed0e8e147edc15bee13b42#diff-2bb8ddff24b3a031be0f64354262e587R76
>
> This testcase does not catch the bug we had in fanotify_group_event_mask()
> because the masking by mark->mask already hides the fact that we failed to
> apply the ignore mask.
>
> What does catch this kind of bug (tested) is a testcase (admittedly
> somewhat silly) like this:
>
> {
>         "inode mark, FAN_OPEN | FAN_OPEN_EXEC events, ignore FAN_OPEN_EXEC",
>         INIT_FANOTIFY_MARK_TYPE(INODE),
>         FAN_OPEN | FAN_OPEN_EXEC,
>         FAN_OPEN_EXEC,
>         2,
>         {FAN_OPEN, FAN_OPEN}
> },
>

Yah. that is simple to add.
Matthew please add it to your test.

> A real variant of this would be FAN_OPEN | FAN_OPEN_EXEC on mount, ignore
> FAN_OPEN on inode. Then we should just get one FAN_OPEN_EXEC but with the bug
> we'd get FAN_OPEN | FAN_OPEN_EXEC.
>
> But creating such test would be slightly more involved. But probably it is
> worth it. Matthew?

Not a problem to add a test case to fanotify10 which checks combination
of different mark type with ignore mask.
Matthew, you should have no problem to extend fanotify10 by making the
mask and ignore mask and result mask variables of the test case.
Please make that change based on fanotify_sb branch, which extends
fanotify10 to filesystem mark test cases.

Thanks,
Amir.

Re: [PATCH v7 0/4] fanotify: introduce new event mask FAN_OPEN_EXEC and FAN_OPEN_EXEC_PERM

[Matthew Bobrowski]

On Wed, Nov 14, 2018 at 01:02:47PM +0100, Jan Kara wrote:
> > > > > LTP tests for this feature are on my 'fanotify-exec' branch here:
> > > > > https://github.com/matthewbobrowski/ltp/commits/fanotify_exec. The files
> > > > > that contains the test cases are provided below:
> > > > >
> > > > > syscalls/fanotify03: test cases have been updated to cover
> > > > >                      FAN_OPEN_EXEC_PERM events
> > > > > syscalls/fanotify12: newly introduced LTP test file to cover
> > > > >                      FAN_OPEN_EXEC events
> > > >
> > > > I have been wondering for a while why the testcases passed when ignore mask
> > > > hasn't been properly treated in fanotify_group_event_mask() but then I
> > > > realized that the generic code will not even call to fanotify if ignore
> > > > masks result in clearing the event.
> > >
> > > So does that means we have missing test coverage?
> > >
> > 
> > This is covered by test case #3 of Matthew's proposed LTP test.
> > https://github.com/matthewbobrowski/ltp/commit/9e350fe15a5423d896ed0e8e147edc15bee13b42#diff-2bb8ddff24b3a031be0f64354262e587R76
> 
> This testcase does not catch the bug we had in fanotify_group_event_mask()
> because the masking by mark->mask already hides the fact that we failed to
> apply the ignore mask.
> 
> What does catch this kind of bug (tested) is a testcase (admittedly
> somewhat silly) like this:
> 
> {
>         "inode mark, FAN_OPEN | FAN_OPEN_EXEC events, ignore FAN_OPEN_EXEC",
>         INIT_FANOTIFY_MARK_TYPE(INODE),
>         FAN_OPEN | FAN_OPEN_EXEC,
>         FAN_OPEN_EXEC,
>         2,
>         {FAN_OPEN, FAN_OPEN}
> },

I've incorporated this^ test as part of my test cases. All tests, this one
included, are passing on kernel built on your 'fsnotify' branch. You can find
the updated test case file here:

https://github.com/matthewbobrowski/ltp/commit/d1d57d5bda8db49a26624c7737c2db88ea90f9db

> A real variant of this would be FAN_OPEN | FAN_OPEN_EXEC on mount, ignore
> FAN_OPEN on inode. Then we should just get one FAN_OPEN_EXEC but with the bug
> we'd get FAN_OPEN | FAN_OPEN_EXEC.
> 
> But creating such test would be slightly more involved. But probably it is
> worth it. Matthew?

Yeah, this shouldn't be too difficult to add at all, but as Amir pointed out,
I'd probably be in favour of putting this into a different test case i.e. one
which deals with mounts/filesystem mark types.
 
> Also I have noticed that fanotify12 test has a bug that it reports:
> 
> fanotify12.c:220: FAIL: Received event: mask=1020, pid=5142 (expected 5142), fd=5
> 
> i.e., it reports expected pid instead of expected mask when mask does not
> match. Can you please fix it Matthew?

Sure, a fix for this has also been applied here:

https://github.com/matthewbobrowski/ltp/commit/d1d57d5bda8db49a26624c7737c2db88ea90f9db

-- 
Matthew Bobrowski