linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/3] fanotify: Allow user space to pass back additional audit info
@ 2022-12-12 14:06 Richard Guy Briggs
  2022-12-12 14:06 ` [PATCH v5 1/3] fanotify: Ensure consistent variable type for response Richard Guy Briggs
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Richard Guy Briggs @ 2022-12-12 14:06 UTC (permalink / raw)
  To: Linux-Audit Mailing List, LKML, linux-fsdevel, linux-api
  Cc: Paul Moore, Eric Paris, Steve Grubb, Richard Guy Briggs,
	Jan Kara, Amir Goldstein

The Fanotify API can be used for access control by requesting permission
event notification. The user space tooling that uses it may have a
complicated policy that inherently contains additional context for the
decision. If this information were available in the audit trail, policy
writers can close the loop on debugging policy. Also, if this additional
information were available, it would enable the creation of tools that
can suggest changes to the policy similar to how audit2allow can help
refine labeled security.

This patchset defines a new flag (FAN_INFO) and new extensions that
define additional information which are appended after the response
structure returned from user space on a permission event.  The appended
information is organized with headers containing a type and size that
can be delegated to interested subsystems.  One new information type is
defined to audit the triggering rule number.  

A newer kernel will work with an older userspace and an older kernel
will behave as expected and reject a newer userspace, leaving it up to
the newer userspace to test appropriately and adapt as necessary.

The audit function was updated to log the additional information in the
AUDIT_FANOTIFY record. The following are examples of the new record
format:
  type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_info=3137 subj_trust=3 obj_trust=5
  type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F subj_trust=? obj_trust=?

changelog:
v1:
- first version by Steve Grubb <sgrubb@redhat.com>
Link: https://lore.kernel.org/r/2042449.irdbgypaU6@x2

v2:
- enhancements suggested by Jan Kara <jack@suse.cz>
- 1/3 change %d to %u in pr_debug
- 2/3 change response from __u32 to __u16
- mod struct fanotify_response and fanotify_perm_event add extra_info_type, extra_info_buf
- extra_info_buf size max FANOTIFY_MAX_RESPONSE_EXTRA_LEN, add struct fanotify_response_audit_rule
- extend debug statements
- remove unneeded macros
- [internal] change interface to finish_permission_event() and process_access_response()
- 3/3 update format of extra information
- [internal] change interface to audit_fanotify()
- change ctx_type= to fan_type=
Link: https://lore.kernel.org/r/cover.1651174324.git.rgb@redhat.com

v3:
- 1/3 switch {,__}audit_fanotify() from uint to u32
- 2/3 re-add fanotify_get_response switch case FAN_DENY: to avoid unnecessary churn
- add FAN_EXTRA flag to indicate more info and break with old kernel
- change response from u16 to u32 to avoid endian issues
- change extra_info_buf to union
- move low-cost fd check earlier
- change FAN_RESPONSE_INFO_AUDIT_NONE to FAN_RESPONSE_INFO_NONE
- switch to u32 for internal and __u32 for uapi
Link: https://lore.kernel.org/all/cover.1652730821.git.rgb@redhat.com

v4:
- scrap FAN_INVALID_RESPONSE_MASK in favour of original to catch invalid response == 0
- introduce FANOTIFY_RESPONSE_* macros
- uapi: remove union
- keep original struct fanotify_response, add fan_info infra starting with audit reason
- uapi add struct fanotify_response_info_header{type/pad/len} and struct fanotify_response_info_audit_rule{hdr/rule}
- rename fan_ctx= to fan_info=, FAN_EXTRA to FAN_INFO
- change event struct from type/buf to len/buf
- enable multiple info extensions in one message
- hex encode fan_info in __audit_fanotify()
- record type FANOTIFY extended to "type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F"                                                                                                                     
Link: https://lore.kernel.org/all/cover.1659996830.git.rgb@redhat.com

v5:
- fixed warnings in p2/4 and p3/4 found by <lkp@intel.com>
- restore original behaviour for !FAN_INFO case and fanotify_get_response()
- rename member audit_rule to rule_number
- eliminate memory leak of info_buf on failure (no longer dynamic)
- rename buf:info, count:info_len, c:remain, ib:infop
- fix pr_debug
- return -ENOENT on FAN_INFO and fd==FAN_NOFD to signal new kernel
- fanotify_write() remove redundant size check
- add u32 subj_trust obj_trust fields with unknown value "2"
- split out to helper process_access_response_info()
- restore finish_permission_event() response_struct to u32
- assume and enforce one rule to audit, pass struct directly to __audit_fanotify()
- change fanotify_perm_event struct to union hdr/audir_rule
- add vspace to fanotify_write() and process_access_response_info()
- squash 3/4 with 4/4
- fix v3 and v4 links
Link: https://lore.kernel.org/all/cover.1670606054.git.rgb@redhat.com

Richard Guy Briggs (3):
  fanotify: Ensure consistent variable type for response
  fanotify: define struct members to hold response decision context
  fanotify,audit: Allow audit to use the full permission event response

 fs/notify/fanotify/fanotify.c      |  8 ++-
 fs/notify/fanotify/fanotify.h      |  6 +-
 fs/notify/fanotify/fanotify_user.c | 88 ++++++++++++++++++++++--------
 include/linux/audit.h              |  9 +--
 include/linux/fanotify.h           |  5 ++
 include/uapi/linux/fanotify.h      | 30 +++++++++-
 kernel/auditsc.c                   | 25 ++++++++-
 7 files changed, 138 insertions(+), 33 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v5 1/3] fanotify: Ensure consistent variable type for response
  2022-12-12 14:06 [PATCH v5 0/3] fanotify: Allow user space to pass back additional audit info Richard Guy Briggs
@ 2022-12-12 14:06 ` Richard Guy Briggs
  2022-12-20 23:30   ` Paul Moore
  2022-12-12 14:06 ` [PATCH v5 2/3] fanotify: define struct members to hold response decision context Richard Guy Briggs
  2022-12-12 14:06 ` [PATCH v5 3/3] fanotify,audit: Allow audit to use the full permission event response Richard Guy Briggs
  2 siblings, 1 reply; 20+ messages in thread
From: Richard Guy Briggs @ 2022-12-12 14:06 UTC (permalink / raw)
  To: Linux-Audit Mailing List, LKML, linux-fsdevel, linux-api
  Cc: Paul Moore, Eric Paris, Steve Grubb, Richard Guy Briggs,
	Jan Kara, Amir Goldstein

The user space API for the response variable is __u32. This patch makes
sure that the whole path through the kernel uses u32 so that there is
no sign extension or truncation of the user space response.

Suggested-by: Steve Grubb <sgrubb@redhat.com>
Link: https://lore.kernel.org/r/12617626.uLZWGnKmhe@x2
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 fs/notify/fanotify/fanotify.h      | 2 +-
 fs/notify/fanotify/fanotify_user.c | 6 +++---
 include/linux/audit.h              | 6 +++---
 kernel/auditsc.c                   | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 57f51a9a3015..f899d610bc08 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -425,7 +425,7 @@ FANOTIFY_PE(struct fanotify_event *event)
 struct fanotify_perm_event {
 	struct fanotify_event fae;
 	struct path path;
-	unsigned short response;	/* userspace answer to the event */
+	u32 response;			/* userspace answer to the event */
 	unsigned short state;		/* state of the event */
 	int fd;		/* fd we passed to userspace for this event */
 };
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 4546da4a54f9..caa1211bac8c 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -289,7 +289,7 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
  */
 static void finish_permission_event(struct fsnotify_group *group,
 				    struct fanotify_perm_event *event,
-				    unsigned int response)
+				    u32 response)
 				    __releases(&group->notification_lock)
 {
 	bool destroy = false;
@@ -310,9 +310,9 @@ static int process_access_response(struct fsnotify_group *group,
 {
 	struct fanotify_perm_event *event;
 	int fd = response_struct->fd;
-	int response = response_struct->response;
+	u32 response = response_struct->response;
 
-	pr_debug("%s: group=%p fd=%d response=%d\n", __func__, group,
+	pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
 		 fd, response);
 	/*
 	 * make sure the response is valid, if invalid we do nothing and either
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 3608992848d3..d6b7d0c7ce43 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -416,7 +416,7 @@ extern void __audit_log_capset(const struct cred *new, const struct cred *old);
 extern void __audit_mmap_fd(int fd, int flags);
 extern void __audit_openat2_how(struct open_how *how);
 extern void __audit_log_kern_module(char *name);
-extern void __audit_fanotify(unsigned int response);
+extern void __audit_fanotify(u32 response);
 extern void __audit_tk_injoffset(struct timespec64 offset);
 extern void __audit_ntp_log(const struct audit_ntp_data *ad);
 extern void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
@@ -523,7 +523,7 @@ static inline void audit_log_kern_module(char *name)
 		__audit_log_kern_module(name);
 }
 
-static inline void audit_fanotify(unsigned int response)
+static inline void audit_fanotify(u32 response)
 {
 	if (!audit_dummy_context())
 		__audit_fanotify(response);
@@ -679,7 +679,7 @@ static inline void audit_log_kern_module(char *name)
 {
 }
 
-static inline void audit_fanotify(unsigned int response)
+static inline void audit_fanotify(u32 response)
 { }
 
 static inline void audit_tk_injoffset(struct timespec64 offset)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 547c88be8a28..d1fb821de104 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2877,7 +2877,7 @@ void __audit_log_kern_module(char *name)
 	context->type = AUDIT_KERN_MODULE;
 }
 
-void __audit_fanotify(unsigned int response)
+void __audit_fanotify(u32 response)
 {
 	audit_log(audit_context(), GFP_KERNEL,
 		AUDIT_FANOTIFY,	"resp=%u", response);
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v5 2/3] fanotify: define struct members to hold response decision context
  2022-12-12 14:06 [PATCH v5 0/3] fanotify: Allow user space to pass back additional audit info Richard Guy Briggs
  2022-12-12 14:06 ` [PATCH v5 1/3] fanotify: Ensure consistent variable type for response Richard Guy Briggs
@ 2022-12-12 14:06 ` Richard Guy Briggs
  2022-12-16 16:43   ` Jan Kara
  2022-12-12 14:06 ` [PATCH v5 3/3] fanotify,audit: Allow audit to use the full permission event response Richard Guy Briggs
  2 siblings, 1 reply; 20+ messages in thread
From: Richard Guy Briggs @ 2022-12-12 14:06 UTC (permalink / raw)
  To: Linux-Audit Mailing List, LKML, linux-fsdevel, linux-api
  Cc: Paul Moore, Eric Paris, Steve Grubb, Richard Guy Briggs,
	Jan Kara, Amir Goldstein

This patch adds a flag, FAN_INFO and an extensible buffer to provide
additional information about response decisions.  The buffer contains
one or more headers defining the information type and the length of the
following information.  The patch defines one additional information
type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number.  This will
allow for the creation of other information types in the future if other
users of the API identify different needs.

Suggested-by: Steve Grubb <sgrubb@redhat.com>
Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
Suggested-by: Jan Kara <jack@suse.cz>
Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 fs/notify/fanotify/fanotify.c      |  5 +-
 fs/notify/fanotify/fanotify.h      |  4 ++
 fs/notify/fanotify/fanotify_user.c | 86 ++++++++++++++++++++++--------
 include/linux/fanotify.h           |  5 ++
 include/uapi/linux/fanotify.h      | 30 ++++++++++-
 5 files changed, 107 insertions(+), 23 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index a2a15bc4df28..24ec1d66d5a8 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -262,7 +262,7 @@ static int fanotify_get_response(struct fsnotify_group *group,
 	}
 
 	/* userspace responded, convert to something usable */
-	switch (event->response & ~FAN_AUDIT) {
+	switch (event->response & FANOTIFY_RESPONSE_ACCESS) {
 	case FAN_ALLOW:
 		ret = 0;
 		break;
@@ -563,6 +563,9 @@ static struct fanotify_event *fanotify_alloc_perm_event(const struct path *path,
 
 	pevent->fae.type = FANOTIFY_EVENT_TYPE_PATH_PERM;
 	pevent->response = 0;
+	pevent->hdr.type = FAN_RESPONSE_INFO_NONE;
+	pevent->hdr.pad = 0;
+	pevent->hdr.len = 0;
 	pevent->state = FAN_EVENT_INIT;
 	pevent->path = *path;
 	path_get(path);
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index f899d610bc08..e8a3c28c5d12 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -428,6 +428,10 @@ struct fanotify_perm_event {
 	u32 response;			/* userspace answer to the event */
 	unsigned short state;		/* state of the event */
 	int fd;		/* fd we passed to userspace for this event */
+	union {
+		struct fanotify_response_info_header hdr;
+		struct fanotify_response_info_audit_rule audit_rule;
+	};
 };
 
 static inline struct fanotify_perm_event *
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index caa1211bac8c..cf3584351e00 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -283,19 +283,44 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
 	return client_fd;
 }
 
+static int process_access_response_info(int fd, const char __user *info, size_t info_len,
+					struct fanotify_response_info_audit_rule *friar)
+{
+	if (fd == FAN_NOFD)
+		return -ENOENT;
+
+	if (info_len != sizeof(*friar))
+		return -EINVAL;
+
+	if (copy_from_user(friar, info, sizeof(*friar)))
+		return -EFAULT;
+
+	if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
+		return -EINVAL;
+	if (friar->hdr.pad != 0)
+		return -EINVAL;
+	if (friar->hdr.len != sizeof(*friar))
+		return -EINVAL;
+
+	return info_len;
+}
+
 /*
  * Finish processing of permission event by setting it to ANSWERED state and
  * drop group->notification_lock.
  */
 static void finish_permission_event(struct fsnotify_group *group,
-				    struct fanotify_perm_event *event,
-				    u32 response)
+				    struct fanotify_perm_event *event, u32 response,
+				    struct fanotify_response_info_audit_rule *friar)
 				    __releases(&group->notification_lock)
 {
 	bool destroy = false;
 
 	assert_spin_locked(&group->notification_lock);
-	event->response = response;
+	event->response = response & ~FAN_INFO;
+	if (response & FAN_INFO)
+		memcpy(&event->audit_rule, friar, sizeof(*friar));
+
 	if (event->state == FAN_EVENT_CANCELED)
 		destroy = true;
 	else
@@ -306,20 +331,27 @@ static void finish_permission_event(struct fsnotify_group *group,
 }
 
 static int process_access_response(struct fsnotify_group *group,
-				   struct fanotify_response *response_struct)
+				   struct fanotify_response *response_struct,
+				   const char __user *info,
+				   size_t info_len)
 {
 	struct fanotify_perm_event *event;
 	int fd = response_struct->fd;
 	u32 response = response_struct->response;
+	int ret = info_len;
+	struct fanotify_response_info_audit_rule friar;
 
-	pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
-		 fd, response);
+	pr_debug("%s: group=%p fd=%d response=%u buf=%p size=%zu\n", __func__,
+		 group, fd, response, info, info_len);
 	/*
 	 * make sure the response is valid, if invalid we do nothing and either
 	 * userspace can send a valid response or we will clean it up after the
 	 * timeout
 	 */
-	switch (response & ~FAN_AUDIT) {
+	if (response & ~FANOTIFY_RESPONSE_VALID_MASK)
+		return -EINVAL;
+
+	switch (response & FANOTIFY_RESPONSE_ACCESS) {
 	case FAN_ALLOW:
 	case FAN_DENY:
 		break;
@@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
 		return -EINVAL;
 	}
 
-	if (fd < 0)
+	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
 		return -EINVAL;
 
-	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
+	if (response & FAN_INFO) {
+		ret = process_access_response_info(fd, info, info_len, &friar);
+		if (ret < 0)
+			return ret;
+	} else {
+		ret = 0;
+	}
+
+	if (fd < 0)
 		return -EINVAL;
 
 	spin_lock(&group->notification_lock);
@@ -340,9 +380,9 @@ static int process_access_response(struct fsnotify_group *group,
 			continue;
 
 		list_del_init(&event->fae.fse.list);
-		finish_permission_event(group, event, response);
+		finish_permission_event(group, event, response, &friar);
 		wake_up(&group->fanotify_data.access_waitq);
-		return 0;
+		return ret;
 	}
 	spin_unlock(&group->notification_lock);
 
@@ -804,7 +844,7 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
 			if (ret <= 0) {
 				spin_lock(&group->notification_lock);
 				finish_permission_event(group,
-					FANOTIFY_PERM(event), FAN_DENY);
+					FANOTIFY_PERM(event), FAN_DENY, NULL);
 				wake_up(&group->fanotify_data.access_waitq);
 			} else {
 				spin_lock(&group->notification_lock);
@@ -827,28 +867,32 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
 
 static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos)
 {
-	struct fanotify_response response = { .fd = -1, .response = -1 };
+	struct fanotify_response response;
 	struct fsnotify_group *group;
 	int ret;
+	const char __user *info_buf = buf + sizeof(struct fanotify_response);
+	size_t info_len;
 
 	if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
 		return -EINVAL;
 
 	group = file->private_data;
 
+	pr_debug("%s: group=%p count=%zu\n", __func__, group, count);
+
 	if (count < sizeof(response))
 		return -EINVAL;
 
-	count = sizeof(response);
-
-	pr_debug("%s: group=%p count=%zu\n", __func__, group, count);
-
-	if (copy_from_user(&response, buf, count))
+	if (copy_from_user(&response, buf, sizeof(response)))
 		return -EFAULT;
 
-	ret = process_access_response(group, &response);
+	info_len = count - sizeof(response);
+
+	ret = process_access_response(group, &response, info_buf, info_len);
 	if (ret < 0)
 		count = ret;
+	else
+		count = sizeof(response) + ret;
 
 	return count;
 }
@@ -876,7 +920,7 @@ static int fanotify_release(struct inode *ignored, struct file *file)
 		event = list_first_entry(&group->fanotify_data.access_list,
 				struct fanotify_perm_event, fae.fse.list);
 		list_del_init(&event->fae.fse.list);
-		finish_permission_event(group, event, FAN_ALLOW);
+		finish_permission_event(group, event, FAN_ALLOW, NULL);
 		spin_lock(&group->notification_lock);
 	}
 
@@ -893,7 +937,7 @@ static int fanotify_release(struct inode *ignored, struct file *file)
 			fsnotify_destroy_event(group, fsn_event);
 		} else {
 			finish_permission_event(group, FANOTIFY_PERM(event),
-						FAN_ALLOW);
+						FAN_ALLOW, NULL);
 		}
 		spin_lock(&group->notification_lock);
 	}
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index 8ad743def6f3..4f1c4f603118 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -122,6 +122,11 @@
 #define ALL_FANOTIFY_EVENT_BITS		(FANOTIFY_OUTGOING_EVENTS | \
 					 FANOTIFY_EVENT_FLAGS)
 
+/* These masks check for invalid bits in permission responses. */
+#define FANOTIFY_RESPONSE_ACCESS (FAN_ALLOW | FAN_DENY)
+#define FANOTIFY_RESPONSE_FLAGS (FAN_AUDIT | FAN_INFO)
+#define FANOTIFY_RESPONSE_VALID_MASK (FANOTIFY_RESPONSE_ACCESS | FANOTIFY_RESPONSE_FLAGS)
+
 /* Do not use these old uapi constants internally */
 #undef FAN_ALL_CLASS_BITS
 #undef FAN_ALL_INIT_FLAGS
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index 436258214bb0..cd14c94e9a1e 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -188,15 +188,43 @@ struct fanotify_event_info_error {
 	__u32 error_count;
 };
 
+/*
+ * User space may need to record additional information about its decision.
+ * The extra information type records what kind of information is included.
+ * The default is none. We also define an extra information buffer whose
+ * size is determined by the extra information type.
+ *
+ * If the information type is Audit Rule, then the information following
+ * is the rule number that triggered the user space decision that
+ * requires auditing.
+ */
+
+#define FAN_RESPONSE_INFO_NONE		0
+#define FAN_RESPONSE_INFO_AUDIT_RULE	1
+
 struct fanotify_response {
 	__s32 fd;
 	__u32 response;
 };
 
+struct fanotify_response_info_header {
+	__u8 type;
+	__u8 pad;
+	__u16 len;
+};
+
+struct fanotify_response_info_audit_rule {
+	struct fanotify_response_info_header hdr;
+	__u32 rule_number;
+	__u32 subj_trust;
+	__u32 obj_trust;
+};
+
 /* Legit userspace responses to a _PERM event */
 #define FAN_ALLOW	0x01
 #define FAN_DENY	0x02
-#define FAN_AUDIT	0x10	/* Bit mask to create audit record for result */
+#define FAN_AUDIT	0x10	/* Bitmask to create audit record for result */
+#define FAN_INFO	0x20	/* Bitmask to indicate additional information */
 
 /* No fd set in event */
 #define FAN_NOFD	-1
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v5 3/3] fanotify,audit: Allow audit to use the full permission event response
  2022-12-12 14:06 [PATCH v5 0/3] fanotify: Allow user space to pass back additional audit info Richard Guy Briggs
  2022-12-12 14:06 ` [PATCH v5 1/3] fanotify: Ensure consistent variable type for response Richard Guy Briggs
  2022-12-12 14:06 ` [PATCH v5 2/3] fanotify: define struct members to hold response decision context Richard Guy Briggs
@ 2022-12-12 14:06 ` Richard Guy Briggs
  2022-12-20 23:31   ` Paul Moore
  2023-01-10  0:06   ` Steve Grubb
  2 siblings, 2 replies; 20+ messages in thread
From: Richard Guy Briggs @ 2022-12-12 14:06 UTC (permalink / raw)
  To: Linux-Audit Mailing List, LKML, linux-fsdevel, linux-api
  Cc: Paul Moore, Eric Paris, Steve Grubb, Richard Guy Briggs,
	Jan Kara, Amir Goldstein

This patch passes the full response so that the audit function can use all
of it. The audit function was updated to log the additional information in
the AUDIT_FANOTIFY record.

Currently the only type of fanotify info that is defined is an audit
rule number, but convert it to hex encoding to future-proof the field.
Hex encoding suggested by Paul Moore <paul@paul-moore.com>.

Sample records:
  type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_info=3137 subj_trust=3 obj_trust=5
  type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F subj_trust=2 obj_trust=2

Suggested-by: Steve Grubb <sgrubb@redhat.com>
Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 fs/notify/fanotify/fanotify.c |  3 ++-
 include/linux/audit.h         |  9 +++++----
 kernel/auditsc.c              | 25 ++++++++++++++++++++++---
 3 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 24ec1d66d5a8..29bdd99b29fa 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -273,7 +273,8 @@ static int fanotify_get_response(struct fsnotify_group *group,
 
 	/* Check if the response should be audited */
 	if (event->response & FAN_AUDIT)
-		audit_fanotify(event->response & ~FAN_AUDIT);
+		audit_fanotify(event->response & ~FAN_AUDIT,
+			       &event->audit_rule);
 
 	pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__,
 		 group, event, ret);
diff --git a/include/linux/audit.h b/include/linux/audit.h
index d6b7d0c7ce43..31086a72e32a 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -14,6 +14,7 @@
 #include <linux/audit_arch.h>
 #include <uapi/linux/audit.h>
 #include <uapi/linux/netfilter/nf_tables.h>
+#include <uapi/linux/fanotify.h>
 
 #define AUDIT_INO_UNSET ((unsigned long)-1)
 #define AUDIT_DEV_UNSET ((dev_t)-1)
@@ -416,7 +417,7 @@ extern void __audit_log_capset(const struct cred *new, const struct cred *old);
 extern void __audit_mmap_fd(int fd, int flags);
 extern void __audit_openat2_how(struct open_how *how);
 extern void __audit_log_kern_module(char *name);
-extern void __audit_fanotify(u32 response);
+extern void __audit_fanotify(u32 response, struct fanotify_response_info_audit_rule *friar);
 extern void __audit_tk_injoffset(struct timespec64 offset);
 extern void __audit_ntp_log(const struct audit_ntp_data *ad);
 extern void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
@@ -523,10 +524,10 @@ static inline void audit_log_kern_module(char *name)
 		__audit_log_kern_module(name);
 }
 
-static inline void audit_fanotify(u32 response)
+static inline void audit_fanotify(u32 response, struct fanotify_response_info_audit_rule *friar)
 {
 	if (!audit_dummy_context())
-		__audit_fanotify(response);
+		__audit_fanotify(response, friar);
 }
 
 static inline void audit_tk_injoffset(struct timespec64 offset)
@@ -679,7 +680,7 @@ static inline void audit_log_kern_module(char *name)
 {
 }
 
-static inline void audit_fanotify(u32 response)
+static inline void audit_fanotify(u32 response, struct fanotify_response_info_audit_rule *friar)
 { }
 
 static inline void audit_tk_injoffset(struct timespec64 offset)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index d1fb821de104..8d523066d81f 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -64,6 +64,7 @@
 #include <uapi/linux/limits.h>
 #include <uapi/linux/netfilter/nf_tables.h>
 #include <uapi/linux/openat2.h> // struct open_how
+#include <uapi/linux/fanotify.h>
 
 #include "audit.h"
 
@@ -2877,10 +2878,28 @@ void __audit_log_kern_module(char *name)
 	context->type = AUDIT_KERN_MODULE;
 }
 
-void __audit_fanotify(u32 response)
+void __audit_fanotify(u32 response, struct fanotify_response_info_audit_rule *friar)
 {
-	audit_log(audit_context(), GFP_KERNEL,
-		AUDIT_FANOTIFY,	"resp=%u", response);
+	struct audit_context *ctx = audit_context();
+	struct audit_buffer *ab;
+	char numbuf[12];
+
+	if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) {
+		audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
+			  "resp=%u fan_type=%u fan_info=3F subj_trust=2 obj_trust=2",
+			  response, FAN_RESPONSE_INFO_NONE);
+		return;
+	}
+	ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY);
+	if (ab) {
+		audit_log_format(ab, "resp=%u fan_type=%u fan_info=",
+				 response, friar->hdr.type);
+		snprintf(numbuf, sizeof(numbuf), "%u", friar->rule_number);
+		audit_log_n_hex(ab, numbuf, sizeof(numbuf));
+		audit_log_format(ab, " subj_trust=%u obj_trust=%u",
+				 friar->subj_trust, friar->obj_trust);
+		audit_log_end(ab);
+	}
 }
 
 void __audit_tk_injoffset(struct timespec64 offset)
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 2/3] fanotify: define struct members to hold response decision context
  2022-12-12 14:06 ` [PATCH v5 2/3] fanotify: define struct members to hold response decision context Richard Guy Briggs
@ 2022-12-16 16:43   ` Jan Kara
  2022-12-16 17:05     ` Paul Moore
  2022-12-22 20:47     ` Richard Guy Briggs
  0 siblings, 2 replies; 20+ messages in thread
From: Jan Kara @ 2022-12-16 16:43 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, linux-fsdevel, linux-api,
	Paul Moore, Eric Paris, Steve Grubb, Jan Kara, Amir Goldstein

On Mon 12-12-22 09:06:10, Richard Guy Briggs wrote:
> This patch adds a flag, FAN_INFO and an extensible buffer to provide
> additional information about response decisions.  The buffer contains
> one or more headers defining the information type and the length of the
> following information.  The patch defines one additional information
> type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number.  This will
> allow for the creation of other information types in the future if other
> users of the API identify different needs.
> 
> Suggested-by: Steve Grubb <sgrubb@redhat.com>
> Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> Suggested-by: Jan Kara <jack@suse.cz>
> Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

Thanks for the patches. They look very good to me. Just two nits below. I
can do the small updates on commit if there would be no other changes. But
I'd like to get some review from audit guys for patch 3/3 before I commit
this.

> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index caa1211bac8c..cf3584351e00 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -283,19 +283,44 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
>  	return client_fd;
>  }
>  
> +static int process_access_response_info(int fd, const char __user *info, size_t info_len,
> +					struct fanotify_response_info_audit_rule *friar)

I prefer to keep lines within 80 columns, unless there is really good
reason (like with strings) to have them longer.

BTW, why do you call the info structure 'friar'? I feel some language twist
escapes me ;)

> +{
> +	if (fd == FAN_NOFD)
> +		return -ENOENT;

I would not test 'fd' in this function at all. After all it is not part of
the response info structure and you do check it in
process_access_response() anyway.

> +
> +	if (info_len != sizeof(*friar))
> +		return -EINVAL;
> +
> +	if (copy_from_user(friar, info, sizeof(*friar)))
> +		return -EFAULT;
> +
> +	if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
> +		return -EINVAL;
> +	if (friar->hdr.pad != 0)
> +		return -EINVAL;
> +	if (friar->hdr.len != sizeof(*friar))
> +		return -EINVAL;
> +
> +	return info_len;
> +}
> +

...

> @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
>  		return -EINVAL;
>  	}
>  
> -	if (fd < 0)
> +	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
>  		return -EINVAL;
>  
> -	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> +	if (response & FAN_INFO) {
> +		ret = process_access_response_info(fd, info, info_len, &friar);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		ret = 0;
> +	}
> +
> +	if (fd < 0)
>  		return -EINVAL;

And here I'd do:

	if (fd == FAN_NOFD)
		return 0;
	if (fd < 0)
		return -EINVAL;

As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
extra info is understood by the kernel so that application writing fanotify
responses has a way to check which information it can provide to the
kernel.

								Honza

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 2/3] fanotify: define struct members to hold response decision context
  2022-12-16 16:43   ` Jan Kara
@ 2022-12-16 17:05     ` Paul Moore
  2022-12-19 10:10       ` Jan Kara
  2022-12-22 20:47     ` Richard Guy Briggs
  1 sibling, 1 reply; 20+ messages in thread
From: Paul Moore @ 2022-12-16 17:05 UTC (permalink / raw)
  To: Jan Kara
  Cc: Richard Guy Briggs, Linux-Audit Mailing List, LKML,
	linux-fsdevel, linux-api, Eric Paris, Steve Grubb,
	Amir Goldstein

On Fri, Dec 16, 2022 at 11:43 AM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 12-12-22 09:06:10, Richard Guy Briggs wrote:
> > This patch adds a flag, FAN_INFO and an extensible buffer to provide
> > additional information about response decisions.  The buffer contains
> > one or more headers defining the information type and the length of the
> > following information.  The patch defines one additional information
> > type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number.  This will
> > allow for the creation of other information types in the future if other
> > users of the API identify different needs.
> >
> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > Suggested-by: Jan Kara <jack@suse.cz>
> > Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>
> Thanks for the patches. They look very good to me. Just two nits below. I
> can do the small updates on commit if there would be no other changes. But
> I'd like to get some review from audit guys for patch 3/3 before I commit
> this.

It's in my review queue, but it's a bit lower in the pile as my
understanding is that the linux-next folks don't like to see new
things in the next branches until after the merge window closes.

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 2/3] fanotify: define struct members to hold response decision context
  2022-12-16 17:05     ` Paul Moore
@ 2022-12-19 10:10       ` Jan Kara
  0 siblings, 0 replies; 20+ messages in thread
From: Jan Kara @ 2022-12-19 10:10 UTC (permalink / raw)
  To: Paul Moore
  Cc: Jan Kara, Richard Guy Briggs, Linux-Audit Mailing List, LKML,
	linux-fsdevel, linux-api, Eric Paris, Steve Grubb,
	Amir Goldstein

On Fri 16-12-22 12:05:14, Paul Moore wrote:
> On Fri, Dec 16, 2022 at 11:43 AM Jan Kara <jack@suse.cz> wrote:
> >
> > On Mon 12-12-22 09:06:10, Richard Guy Briggs wrote:
> > > This patch adds a flag, FAN_INFO and an extensible buffer to provide
> > > additional information about response decisions.  The buffer contains
> > > one or more headers defining the information type and the length of the
> > > following information.  The patch defines one additional information
> > > type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number.  This will
> > > allow for the creation of other information types in the future if other
> > > users of the API identify different needs.
> > >
> > > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > > Suggested-by: Jan Kara <jack@suse.cz>
> > > Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >
> > Thanks for the patches. They look very good to me. Just two nits below. I
> > can do the small updates on commit if there would be no other changes. But
> > I'd like to get some review from audit guys for patch 3/3 before I commit
> > this.
> 
> It's in my review queue, but it's a bit lower in the pile as my
> understanding is that the linux-next folks don't like to see new
> things in the next branches until after the merge window closes.

Sure, there's no hurry :). I just wanted to make it clear where the things
stand.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 1/3] fanotify: Ensure consistent variable type for response
  2022-12-12 14:06 ` [PATCH v5 1/3] fanotify: Ensure consistent variable type for response Richard Guy Briggs
@ 2022-12-20 23:30   ` Paul Moore
  0 siblings, 0 replies; 20+ messages in thread
From: Paul Moore @ 2022-12-20 23:30 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, linux-fsdevel, linux-api,
	Eric Paris, Steve Grubb, Jan Kara, Amir Goldstein

On Mon, Dec 12, 2022 at 9:06 AM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> The user space API for the response variable is __u32. This patch makes
> sure that the whole path through the kernel uses u32 so that there is
> no sign extension or truncation of the user space response.
>
> Suggested-by: Steve Grubb <sgrubb@redhat.com>
> Link: https://lore.kernel.org/r/12617626.uLZWGnKmhe@x2
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  fs/notify/fanotify/fanotify.h      | 2 +-
>  fs/notify/fanotify/fanotify_user.c | 6 +++---
>  include/linux/audit.h              | 6 +++---
>  kernel/auditsc.c                   | 2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)

Acked-by: Paul Moore <paul@paul-moore.com> (audit)

--
paul-moore.com

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 3/3] fanotify,audit: Allow audit to use the full permission event response
  2022-12-12 14:06 ` [PATCH v5 3/3] fanotify,audit: Allow audit to use the full permission event response Richard Guy Briggs
@ 2022-12-20 23:31   ` Paul Moore
  2022-12-22 20:42     ` Richard Guy Briggs
  2023-01-10  0:06   ` Steve Grubb
  1 sibling, 1 reply; 20+ messages in thread
From: Paul Moore @ 2022-12-20 23:31 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, linux-fsdevel, linux-api,
	Eric Paris, Steve Grubb, Jan Kara, Amir Goldstein

On Mon, Dec 12, 2022 at 9:06 AM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> This patch passes the full response so that the audit function can use all
> of it. The audit function was updated to log the additional information in
> the AUDIT_FANOTIFY record.
>
> Currently the only type of fanotify info that is defined is an audit
> rule number, but convert it to hex encoding to future-proof the field.
> Hex encoding suggested by Paul Moore <paul@paul-moore.com>.
>
> Sample records:
>   type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_info=3137 subj_trust=3 obj_trust=5
>   type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F subj_trust=2 obj_trust=2
>
> Suggested-by: Steve Grubb <sgrubb@redhat.com>
> Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  fs/notify/fanotify/fanotify.c |  3 ++-
>  include/linux/audit.h         |  9 +++++----
>  kernel/auditsc.c              | 25 ++++++++++++++++++++++---
>  3 files changed, 29 insertions(+), 8 deletions(-)

...

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d1fb821de104..8d523066d81f 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -64,6 +64,7 @@
>  #include <uapi/linux/limits.h>
>  #include <uapi/linux/netfilter/nf_tables.h>
>  #include <uapi/linux/openat2.h> // struct open_how
> +#include <uapi/linux/fanotify.h>
>
>  #include "audit.h"
>
> @@ -2877,10 +2878,28 @@ void __audit_log_kern_module(char *name)
>         context->type = AUDIT_KERN_MODULE;
>  }
>
> -void __audit_fanotify(u32 response)
> +void __audit_fanotify(u32 response, struct fanotify_response_info_audit_rule *friar)
>  {
> -       audit_log(audit_context(), GFP_KERNEL,
> -               AUDIT_FANOTIFY, "resp=%u", response);
> +       struct audit_context *ctx = audit_context();
> +       struct audit_buffer *ab;
> +       char numbuf[12];
> +
> +       if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) {
> +               audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> +                         "resp=%u fan_type=%u fan_info=3F subj_trust=2 obj_trust=2",
> +                         response, FAN_RESPONSE_INFO_NONE);

The fan_info, subj_trust, and obj_trust constant values used here are
awfully magic-numbery and not the usual sentinel values one might
expect for a "none" operation, e.g. zeros/INT_MAX/etc. I believe a
comment here explaining the values would be a good idea.

> +               return;
> +       }
> +       ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY);
> +       if (ab) {
> +               audit_log_format(ab, "resp=%u fan_type=%u fan_info=",
> +                                response, friar->hdr.type);
> +               snprintf(numbuf, sizeof(numbuf), "%u", friar->rule_number);
> +               audit_log_n_hex(ab, numbuf, sizeof(numbuf));

It looks like the kernel's printf format string parsing supports %X so
why not just use that for now, we can always complicate it later if
needed.  It would probably also remove the need for the @ab, @numbuf,
and @ctx variables.  For example:

audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
  "resp=%u fan_type=%u fan_info=%X subj_trust=%u obj_trust=%u",
  response, friar->hdr.type, friar->rule_number,
  friar->subj_trust, friar->obj_trust);

Am I missing something?

> +               audit_log_format(ab, " subj_trust=%u obj_trust=%u",
> +                                friar->subj_trust, friar->obj_trust);
> +               audit_log_end(ab);
> +       }
>  }
>
>  void __audit_tk_injoffset(struct timespec64 offset)
> --
> 2.27.0

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 3/3] fanotify,audit: Allow audit to use the full permission event response
  2022-12-20 23:31   ` Paul Moore
@ 2022-12-22 20:42     ` Richard Guy Briggs
  2022-12-22 21:16       ` Paul Moore
  0 siblings, 1 reply; 20+ messages in thread
From: Richard Guy Briggs @ 2022-12-22 20:42 UTC (permalink / raw)
  To: Paul Moore
  Cc: Jan Kara, linux-api, Amir Goldstein, LKML,
	Linux-Audit Mailing List, linux-fsdevel, Eric Paris

On 2022-12-20 18:31, Paul Moore wrote:
> On Mon, Dec 12, 2022 at 9:06 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > This patch passes the full response so that the audit function can use all
> > of it. The audit function was updated to log the additional information in
> > the AUDIT_FANOTIFY record.
> >
> > Currently the only type of fanotify info that is defined is an audit
> > rule number, but convert it to hex encoding to future-proof the field.
> > Hex encoding suggested by Paul Moore <paul@paul-moore.com>.
> >
> > Sample records:
> >   type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_info=3137 subj_trust=3 obj_trust=5
> >   type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F subj_trust=2 obj_trust=2
> >
> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  fs/notify/fanotify/fanotify.c |  3 ++-
> >  include/linux/audit.h         |  9 +++++----
> >  kernel/auditsc.c              | 25 ++++++++++++++++++++++---
> >  3 files changed, 29 insertions(+), 8 deletions(-)
> 
> ...
> 
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index d1fb821de104..8d523066d81f 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -64,6 +64,7 @@
> >  #include <uapi/linux/limits.h>
> >  #include <uapi/linux/netfilter/nf_tables.h>
> >  #include <uapi/linux/openat2.h> // struct open_how
> > +#include <uapi/linux/fanotify.h>
> >
> >  #include "audit.h"
> >
> > @@ -2877,10 +2878,28 @@ void __audit_log_kern_module(char *name)
> >         context->type = AUDIT_KERN_MODULE;
> >  }
> >
> > -void __audit_fanotify(u32 response)
> > +void __audit_fanotify(u32 response, struct fanotify_response_info_audit_rule *friar)
> >  {
> > -       audit_log(audit_context(), GFP_KERNEL,
> > -               AUDIT_FANOTIFY, "resp=%u", response);
> > +       struct audit_context *ctx = audit_context();
> > +       struct audit_buffer *ab;
> > +       char numbuf[12];
> > +
> > +       if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) {
> > +               audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > +                         "resp=%u fan_type=%u fan_info=3F subj_trust=2 obj_trust=2",
> > +                         response, FAN_RESPONSE_INFO_NONE);
> 
> The fan_info, subj_trust, and obj_trust constant values used here are
> awfully magic-numbery and not the usual sentinel values one might
> expect for a "none" operation, e.g. zeros/INT_MAX/etc. I believe a
> comment here explaining the values would be a good idea.

Ack.  I'll add a comment.  I would have preferred zero for default of
unset, but Steve requested 0/1/2 no/yes/unknown.

> > +               return;
> > +       }
> > +       ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY);
> > +       if (ab) {
> > +               audit_log_format(ab, "resp=%u fan_type=%u fan_info=",
> > +                                response, friar->hdr.type);
> > +               snprintf(numbuf, sizeof(numbuf), "%u", friar->rule_number);
> > +               audit_log_n_hex(ab, numbuf, sizeof(numbuf));
> 
> It looks like the kernel's printf format string parsing supports %X so
> why not just use that for now, we can always complicate it later if
> needed.  It would probably also remove the need for the @ab, @numbuf,
> and @ctx variables.  For example:
> 
> audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
>   "resp=%u fan_type=%u fan_info=%X subj_trust=%u obj_trust=%u",
>   response, friar->hdr.type, friar->rule_number,
>   friar->subj_trust, friar->obj_trust);
> 
> Am I missing something?

No, I am.  Thank you, that's much cleaner.

> > +               audit_log_format(ab, " subj_trust=%u obj_trust=%u",
> > +                                friar->subj_trust, friar->obj_trust);
> > +               audit_log_end(ab);
> > +       }
> >  }
> >
> >  void __audit_tk_injoffset(struct timespec64 offset)
> > --
> > 2.27.0
> 
> -- 
> paul-moore.com
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://listman.redhat.com/mailman/listinfo/linux-audit
> 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 2/3] fanotify: define struct members to hold response decision context
  2022-12-16 16:43   ` Jan Kara
  2022-12-16 17:05     ` Paul Moore
@ 2022-12-22 20:47     ` Richard Guy Briggs
  2023-01-03 12:42       ` Jan Kara
  1 sibling, 1 reply; 20+ messages in thread
From: Richard Guy Briggs @ 2022-12-22 20:47 UTC (permalink / raw)
  To: Jan Kara
  Cc: Linux-Audit Mailing List, LKML, linux-fsdevel, linux-api,
	Paul Moore, Eric Paris, Steve Grubb, Amir Goldstein

On 2022-12-16 17:43, Jan Kara wrote:
> On Mon 12-12-22 09:06:10, Richard Guy Briggs wrote:
> > This patch adds a flag, FAN_INFO and an extensible buffer to provide
> > additional information about response decisions.  The buffer contains
> > one or more headers defining the information type and the length of the
> > following information.  The patch defines one additional information
> > type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number.  This will
> > allow for the creation of other information types in the future if other
> > users of the API identify different needs.
> > 
> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > Suggested-by: Jan Kara <jack@suse.cz>
> > Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> 
> Thanks for the patches. They look very good to me. Just two nits below. I
> can do the small updates on commit if there would be no other changes. But
> I'd like to get some review from audit guys for patch 3/3 before I commit
> this.

I'd prefer to send a followup patch based on your recommendations rather
than have you modify it.  It does save some back and forth though...

> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index caa1211bac8c..cf3584351e00 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -283,19 +283,44 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
> >  	return client_fd;
> >  }
> >  
> > +static int process_access_response_info(int fd, const char __user *info, size_t info_len,
> > +					struct fanotify_response_info_audit_rule *friar)
> 
> I prefer to keep lines within 80 columns, unless there is really good
> reason (like with strings) to have them longer.

Sure.  In this case, it buys us little since the last line is lined up
with the arguments openning bracket and it one long struct name unless I
unalign that argument and back up the indent by one.

> BTW, why do you call the info structure 'friar'? I feel some language twist
> escapes me ;)

Fanotify_Response_Info_Audit_Rule, it is a pronounceable word, and
besides they have a long reputation for making good beer.  :-D

> > +{
> > +	if (fd == FAN_NOFD)
> > +		return -ENOENT;
> 
> I would not test 'fd' in this function at all. After all it is not part of
> the response info structure and you do check it in
> process_access_response() anyway.

I wrestled with that.  I was even tempted to swallow the following fd
check too, but the flow would not have made as much sense for the
non-INFO case.

My understanding from Amir was that FAN_NOFD was only to be sent in in
conjuction with FAN_INFO to test if a newer kernel was present.

I presumed that if FAN_NOFD was present without FAN_INFO that was an
invalid input to an old kernel.

> > +
> > +	if (info_len != sizeof(*friar))
> > +		return -EINVAL;
> > +
> > +	if (copy_from_user(friar, info, sizeof(*friar)))
> > +		return -EFAULT;
> > +
> > +	if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
> > +		return -EINVAL;
> > +	if (friar->hdr.pad != 0)
> > +		return -EINVAL;
> > +	if (friar->hdr.len != sizeof(*friar))
> > +		return -EINVAL;
> > +
> > +	return info_len;
> > +}
> > +
> 
> ...
> 
> > @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (fd < 0)
> > +	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> >  		return -EINVAL;
> >  
> > -	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > +	if (response & FAN_INFO) {
> > +		ret = process_access_response_info(fd, info, info_len, &friar);
> > +		if (ret < 0)
> > +			return ret;
> > +	} else {
> > +		ret = 0;
> > +	}
> > +
> > +	if (fd < 0)
> >  		return -EINVAL;
> 
> And here I'd do:
> 
> 	if (fd == FAN_NOFD)
> 		return 0;
> 	if (fd < 0)
> 		return -EINVAL;
> 
> As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
> extra info is understood by the kernel so that application writing fanotify
> responses has a way to check which information it can provide to the
> kernel.

The reason for including it in process_access_response_info() is to make
sure that it is included in the FAN_INFO case to detect this extension.
If it were included here

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

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 3/3] fanotify,audit: Allow audit to use the full permission event response
  2022-12-22 20:42     ` Richard Guy Briggs
@ 2022-12-22 21:16       ` Paul Moore
  0 siblings, 0 replies; 20+ messages in thread
From: Paul Moore @ 2022-12-22 21:16 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Jan Kara, linux-api, Amir Goldstein, LKML,
	Linux-Audit Mailing List, linux-fsdevel, Eric Paris

On Thu, Dec 22, 2022 at 3:42 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2022-12-20 18:31, Paul Moore wrote:
> > On Mon, Dec 12, 2022 at 9:06 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > >
> > > This patch passes the full response so that the audit function can use all
> > > of it. The audit function was updated to log the additional information in
> > > the AUDIT_FANOTIFY record.
> > >
> > > Currently the only type of fanotify info that is defined is an audit
> > > rule number, but convert it to hex encoding to future-proof the field.
> > > Hex encoding suggested by Paul Moore <paul@paul-moore.com>.
> > >
> > > Sample records:
> > >   type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_info=3137 subj_trust=3 obj_trust=5
> > >   type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F subj_trust=2 obj_trust=2
> > >
> > > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > > Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >  fs/notify/fanotify/fanotify.c |  3 ++-
> > >  include/linux/audit.h         |  9 +++++----
> > >  kernel/auditsc.c              | 25 ++++++++++++++++++++++---
> > >  3 files changed, 29 insertions(+), 8 deletions(-)
> >
> > ...
> >
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index d1fb821de104..8d523066d81f 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -64,6 +64,7 @@
> > >  #include <uapi/linux/limits.h>
> > >  #include <uapi/linux/netfilter/nf_tables.h>
> > >  #include <uapi/linux/openat2.h> // struct open_how
> > > +#include <uapi/linux/fanotify.h>
> > >
> > >  #include "audit.h"
> > >
> > > @@ -2877,10 +2878,28 @@ void __audit_log_kern_module(char *name)
> > >         context->type = AUDIT_KERN_MODULE;
> > >  }
> > >
> > > -void __audit_fanotify(u32 response)
> > > +void __audit_fanotify(u32 response, struct fanotify_response_info_audit_rule *friar)
> > >  {
> > > -       audit_log(audit_context(), GFP_KERNEL,
> > > -               AUDIT_FANOTIFY, "resp=%u", response);
> > > +       struct audit_context *ctx = audit_context();
> > > +       struct audit_buffer *ab;
> > > +       char numbuf[12];
> > > +
> > > +       if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) {
> > > +               audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > > +                         "resp=%u fan_type=%u fan_info=3F subj_trust=2 obj_trust=2",
> > > +                         response, FAN_RESPONSE_INFO_NONE);
> >
> > The fan_info, subj_trust, and obj_trust constant values used here are
> > awfully magic-numbery and not the usual sentinel values one might
> > expect for a "none" operation, e.g. zeros/INT_MAX/etc. I believe a
> > comment here explaining the values would be a good idea.
>
> Ack.  I'll add a comment.  I would have preferred zero for default of
> unset, but Steve requested 0/1/2 no/yes/unknown.

Yeah, if they were zeros I don't think we would need to comment on
them as zeros for unset/unknown/invalid is rather common, 2 ... not so
much.

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 2/3] fanotify: define struct members to hold response decision context
  2022-12-22 20:47     ` Richard Guy Briggs
@ 2023-01-03 12:42       ` Jan Kara
  2023-01-16 20:42         ` Richard Guy Briggs
  0 siblings, 1 reply; 20+ messages in thread
From: Jan Kara @ 2023-01-03 12:42 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Jan Kara, Linux-Audit Mailing List, LKML, linux-fsdevel,
	linux-api, Paul Moore, Eric Paris, Steve Grubb, Amir Goldstein

On Thu 22-12-22 15:47:21, Richard Guy Briggs wrote:
> On 2022-12-16 17:43, Jan Kara wrote:
> > On Mon 12-12-22 09:06:10, Richard Guy Briggs wrote:
> > > This patch adds a flag, FAN_INFO and an extensible buffer to provide
> > > additional information about response decisions.  The buffer contains
> > > one or more headers defining the information type and the length of the
> > > following information.  The patch defines one additional information
> > > type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number.  This will
> > > allow for the creation of other information types in the future if other
> > > users of the API identify different needs.
> > > 
> > > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > > Suggested-by: Jan Kara <jack@suse.cz>
> > > Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > 
> > Thanks for the patches. They look very good to me. Just two nits below. I
> > can do the small updates on commit if there would be no other changes. But
> > I'd like to get some review from audit guys for patch 3/3 before I commit
> > this.
> 
> I'd prefer to send a followup patch based on your recommendations rather
> than have you modify it.  It does save some back and forth though...

OK, since there are updates to patch 3 as well, I agree this is a better
way forward.

> > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > > index caa1211bac8c..cf3584351e00 100644
> > > --- a/fs/notify/fanotify/fanotify_user.c
> > > +++ b/fs/notify/fanotify/fanotify_user.c
> > > @@ -283,19 +283,44 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
> > >  	return client_fd;
> > >  }
> > >  
> > > +static int process_access_response_info(int fd, const char __user *info, size_t info_len,
> > > +					struct fanotify_response_info_audit_rule *friar)
> > 
> > I prefer to keep lines within 80 columns, unless there is really good
> > reason (like with strings) to have them longer.
> 
> Sure.  In this case, it buys us little since the last line is lined up
> with the arguments openning bracket and it one long struct name unless I
> unalign that argument and back up the indent by one.

Yeah, that's what I'd generally do.

> > BTW, why do you call the info structure 'friar'? I feel some language twist
> > escapes me ;)
> 
> Fanotify_Response_Info_Audit_Rule, it is a pronounceable word, and
> besides they have a long reputation for making good beer.  :-D

Aha, ok :) Thanks for explanation.

> > > +{
> > > +	if (fd == FAN_NOFD)
> > > +		return -ENOENT;
> > 
> > I would not test 'fd' in this function at all. After all it is not part of
> > the response info structure and you do check it in
> > process_access_response() anyway.
> 
> I wrestled with that.  I was even tempted to swallow the following fd
> check too, but the flow would not have made as much sense for the
> non-INFO case.
> 
> My understanding from Amir was that FAN_NOFD was only to be sent in in
> conjuction with FAN_INFO to test if a newer kernel was present.

Yes, that is correct. But we not only want to check that FAN_INFO flag is
understood (as you do in your patch) but also whether a particular response
type is understood (which you don't verify for FAN_NOFD). Currently, there
is only one response type (FAN_RESPONSE_INFO_AUDIT_RULE) but if there are
more in the future we need old kernels to refuse new response types even
for FAN_NOFD case.

> I presumed that if FAN_NOFD was present without FAN_INFO that was an
> invalid input to an old kernel.

Yes, that is correct and I agree the conditions I've suggested below are
wrong in that regard and need a bit of tweaking. Thanks for catching it.

> > > +
> > > +	if (info_len != sizeof(*friar))
> > > +		return -EINVAL;
> > > +
> > > +	if (copy_from_user(friar, info, sizeof(*friar)))
> > > +		return -EFAULT;
> > > +
> > > +	if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
> > > +		return -EINVAL;
> > > +	if (friar->hdr.pad != 0)
> > > +		return -EINVAL;
> > > +	if (friar->hdr.len != sizeof(*friar))
> > > +		return -EINVAL;
> > > +
> > > +	return info_len;
> > > +}
> > > +
> > 
> > ...
> > 
> > > @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	if (fd < 0)
> > > +	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > >  		return -EINVAL;
> > >  
> > > -	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > +	if (response & FAN_INFO) {
> > > +		ret = process_access_response_info(fd, info, info_len, &friar);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	} else {
> > > +		ret = 0;
> > > +	}
> > > +
> > > +	if (fd < 0)
> > >  		return -EINVAL;
> > 
> > And here I'd do:
> > 
> > 	if (fd == FAN_NOFD)
> > 		return 0;
> > 	if (fd < 0)
> > 		return -EINVAL;
> > 
> > As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
> > extra info is understood by the kernel so that application writing fanotify
> > responses has a way to check which information it can provide to the
> > kernel.
> 
> The reason for including it in process_access_response_info() is to make
> sure that it is included in the FAN_INFO case to detect this extension.
> If it were included here

I see what you're getting at now. So the condition

 	if (fd == FAN_NOFD)
 		return 0;

needs to be moved into 

	if (response & FAN_INFO)

branch after process_access_response_info(). I still prefer to keep it
outside of the process_access_response_info() function itself as it looks
more logical to me. Does it address your concerns?

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 3/3] fanotify,audit: Allow audit to use the full permission event response
  2022-12-12 14:06 ` [PATCH v5 3/3] fanotify,audit: Allow audit to use the full permission event response Richard Guy Briggs
  2022-12-20 23:31   ` Paul Moore
@ 2023-01-10  0:06   ` Steve Grubb
  2023-01-10  3:08     ` Richard Guy Briggs
  1 sibling, 1 reply; 20+ messages in thread
From: Steve Grubb @ 2023-01-10  0:06 UTC (permalink / raw)
  To: Linux-Audit Mailing List, LKML, linux-fsdevel, linux-api,
	Richard Guy Briggs
  Cc: Paul Moore, Eric Paris, Richard Guy Briggs, Jan Kara, Amir Goldstein

Hello,

Sorry to take so long. Holidays and kernel build problems. However, I have 
built a kernel with these patches. I only have 2 comments. When I use an 
application that expected the old API, meaning it simply does:

        response.fd = metadata->fd;
        response.response = reply;
        close(metadata->fd);
        write(fd, &response, sizeof(struct fanotify_response));

I get access denials. Every time. If the program is using the new API and 
sets FAN_INFO, then it works as expected. I'll do some more testing but I 
think there is something wrong in the compatibility path.

On Monday, December 12, 2022 9:06:11 AM EST Richard Guy Briggs wrote:
> This patch passes the full response so that the audit function can use all
> of it. The audit function was updated to log the additional information in
> the AUDIT_FANOTIFY record.

What I'm seeing is:

type=FANOTIFY msg=audit(01/09/2023 18:43:16.306:366) : resp=deny fan_type=1 
fan_info=313300000000000000000000 subj_trust=0 obj_trust=0

Where fan_info was supposed to be 13 decimal. More below...

> Currently the only type of fanotify info that is defined is an audit
> rule number, but convert it to hex encoding to future-proof the field.
> Hex encoding suggested by Paul Moore <paul@paul-moore.com>.
> 
> Sample records:
>   type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1
> fan_info=3137 subj_trust=3 obj_trust=5 type=FANOTIFY
> msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F subj_trust=2
> obj_trust=2
> 
> Suggested-by: Steve Grubb <sgrubb@redhat.com>
> Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  fs/notify/fanotify/fanotify.c |  3 ++-
>  include/linux/audit.h         |  9 +++++----
>  kernel/auditsc.c              | 25 ++++++++++++++++++++++---
>  3 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 24ec1d66d5a8..29bdd99b29fa 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -273,7 +273,8 @@ static int fanotify_get_response(struct fsnotify_group
> *group,
> 
>  	/* Check if the response should be audited */
>  	if (event->response & FAN_AUDIT)
> -		audit_fanotify(event->response & ~FAN_AUDIT);
> +		audit_fanotify(event->response & ~FAN_AUDIT,
> +			       &event->audit_rule);
> 
>  	pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__,
>  		 group, event, ret);
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index d6b7d0c7ce43..31086a72e32a 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -14,6 +14,7 @@
>  #include <linux/audit_arch.h>
>  #include <uapi/linux/audit.h>
>  #include <uapi/linux/netfilter/nf_tables.h>
> +#include <uapi/linux/fanotify.h>
> 
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -416,7 +417,7 @@ extern void __audit_log_capset(const struct cred *new,
> const struct cred *old); extern void __audit_mmap_fd(int fd, int flags);
>  extern void __audit_openat2_how(struct open_how *how);
>  extern void __audit_log_kern_module(char *name);
> -extern void __audit_fanotify(u32 response);
> +extern void __audit_fanotify(u32 response, struct
> fanotify_response_info_audit_rule *friar); extern void
> __audit_tk_injoffset(struct timespec64 offset);
>  extern void __audit_ntp_log(const struct audit_ntp_data *ad);
>  extern void __audit_log_nfcfg(const char *name, u8 af, unsigned int
> nentries, @@ -523,10 +524,10 @@ static inline void
> audit_log_kern_module(char *name) __audit_log_kern_module(name);
>  }
> 
> -static inline void audit_fanotify(u32 response)
> +static inline void audit_fanotify(u32 response, struct
> fanotify_response_info_audit_rule *friar) {
>  	if (!audit_dummy_context())
> -		__audit_fanotify(response);
> +		__audit_fanotify(response, friar);
>  }
> 
>  static inline void audit_tk_injoffset(struct timespec64 offset)
> @@ -679,7 +680,7 @@ static inline void audit_log_kern_module(char *name)
>  {
>  }
> 
> -static inline void audit_fanotify(u32 response)
> +static inline void audit_fanotify(u32 response, struct
> fanotify_response_info_audit_rule *friar) { }
> 
>  static inline void audit_tk_injoffset(struct timespec64 offset)
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d1fb821de104..8d523066d81f 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -64,6 +64,7 @@
>  #include <uapi/linux/limits.h>
>  #include <uapi/linux/netfilter/nf_tables.h>
>  #include <uapi/linux/openat2.h> // struct open_how
> +#include <uapi/linux/fanotify.h>
> 
>  #include "audit.h"
> 
> @@ -2877,10 +2878,28 @@ void __audit_log_kern_module(char *name)
>  	context->type = AUDIT_KERN_MODULE;
>  }
> 
> -void __audit_fanotify(u32 response)
> +void __audit_fanotify(u32 response, struct
> fanotify_response_info_audit_rule *friar) {
> -	audit_log(audit_context(), GFP_KERNEL,
> -		AUDIT_FANOTIFY,	"resp=%u", response);
> +	struct audit_context *ctx = audit_context();
> +	struct audit_buffer *ab;
> +	char numbuf[12];
> +
> +	if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) {
> +		audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> +			  "resp=%u fan_type=%u fan_info=3F subj_trust=2 
obj_trust=2",
> +			  response, FAN_RESPONSE_INFO_NONE);
> +		return;
> +	}
> +	ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY);
> +	if (ab) {
> +		audit_log_format(ab, "resp=%u fan_type=%u fan_info=",
> +				 response, friar->hdr.type);
> +		snprintf(numbuf, sizeof(numbuf), "%u", friar->rule_number);
> +		audit_log_n_hex(ab, numbuf, sizeof(numbuf));

I don't think it needs to be converted to ascii and then hexencoded. As Paul 
said, probably %X is all we need here.

-Steve


> +		audit_log_format(ab, " subj_trust=%u obj_trust=%u",
> +				 friar->subj_trust, friar->obj_trust);
> +		audit_log_end(ab);
> +	}
>  }
> 
>  void __audit_tk_injoffset(struct timespec64 offset)





^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 3/3] fanotify,audit: Allow audit to use the full permission event response
  2023-01-10  0:06   ` Steve Grubb
@ 2023-01-10  3:08     ` Richard Guy Briggs
  2023-01-10 15:26       ` Steve Grubb
  0 siblings, 1 reply; 20+ messages in thread
From: Richard Guy Briggs @ 2023-01-10  3:08 UTC (permalink / raw)
  To: Steve Grubb
  Cc: Linux-Audit Mailing List, LKML, linux-fsdevel, linux-api,
	Paul Moore, Eric Paris, Jan Kara, Amir Goldstein

On 2023-01-09 19:06, Steve Grubb wrote:
> Hello,
> 
> Sorry to take so long. Holidays and kernel build problems. However, I have 
> built a kernel with these patches. I only have 2 comments. When I use an 
> application that expected the old API, meaning it simply does:
> 
>         response.fd = metadata->fd;
>         response.response = reply;
>         close(metadata->fd);
>         write(fd, &response, sizeof(struct fanotify_response));
> 
> I get access denials. Every time. If the program is using the new API and 
> sets FAN_INFO, then it works as expected. I'll do some more testing but I 
> think there is something wrong in the compatibility path.

I'll have a closer look, because this wasn't the intended behaviour.

> On Monday, December 12, 2022 9:06:11 AM EST Richard Guy Briggs wrote:
> > This patch passes the full response so that the audit function can use all
> > of it. The audit function was updated to log the additional information in
> > the AUDIT_FANOTIFY record.
> 
> What I'm seeing is:
> 
> type=FANOTIFY msg=audit(01/09/2023 18:43:16.306:366) : resp=deny fan_type=1 
> fan_info=313300000000000000000000 subj_trust=0 obj_trust=0
> 
> Where fan_info was supposed to be 13 decimal. More below...

Well, it *is* 13 decimal, expressed as a hex-encoded string as was
requested for future-proofing, albeit longer than necessary...

> > Currently the only type of fanotify info that is defined is an audit
> > rule number, but convert it to hex encoding to future-proof the field.
> > Hex encoding suggested by Paul Moore <paul@paul-moore.com>.
> > 
> > Sample records:
> >   type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1
> > fan_info=3137 subj_trust=3 obj_trust=5 type=FANOTIFY
> > msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F subj_trust=2
> > obj_trust=2
> > 
> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  fs/notify/fanotify/fanotify.c |  3 ++-
> >  include/linux/audit.h         |  9 +++++----
> >  kernel/auditsc.c              | 25 ++++++++++++++++++++++---
> >  3 files changed, 29 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index 24ec1d66d5a8..29bdd99b29fa 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -273,7 +273,8 @@ static int fanotify_get_response(struct fsnotify_group
> > *group,
> > 
> >  	/* Check if the response should be audited */
> >  	if (event->response & FAN_AUDIT)
> > -		audit_fanotify(event->response & ~FAN_AUDIT);
> > +		audit_fanotify(event->response & ~FAN_AUDIT,
> > +			       &event->audit_rule);
> > 
> >  	pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__,
> >  		 group, event, ret);
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index d6b7d0c7ce43..31086a72e32a 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -14,6 +14,7 @@
> >  #include <linux/audit_arch.h>
> >  #include <uapi/linux/audit.h>
> >  #include <uapi/linux/netfilter/nf_tables.h>
> > +#include <uapi/linux/fanotify.h>
> > 
> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > @@ -416,7 +417,7 @@ extern void __audit_log_capset(const struct cred *new,
> > const struct cred *old); extern void __audit_mmap_fd(int fd, int flags);
> >  extern void __audit_openat2_how(struct open_how *how);
> >  extern void __audit_log_kern_module(char *name);
> > -extern void __audit_fanotify(u32 response);
> > +extern void __audit_fanotify(u32 response, struct
> > fanotify_response_info_audit_rule *friar); extern void
> > __audit_tk_injoffset(struct timespec64 offset);
> >  extern void __audit_ntp_log(const struct audit_ntp_data *ad);
> >  extern void __audit_log_nfcfg(const char *name, u8 af, unsigned int
> > nentries, @@ -523,10 +524,10 @@ static inline void
> > audit_log_kern_module(char *name) __audit_log_kern_module(name);
> >  }
> > 
> > -static inline void audit_fanotify(u32 response)
> > +static inline void audit_fanotify(u32 response, struct
> > fanotify_response_info_audit_rule *friar) {
> >  	if (!audit_dummy_context())
> > -		__audit_fanotify(response);
> > +		__audit_fanotify(response, friar);
> >  }
> > 
> >  static inline void audit_tk_injoffset(struct timespec64 offset)
> > @@ -679,7 +680,7 @@ static inline void audit_log_kern_module(char *name)
> >  {
> >  }
> > 
> > -static inline void audit_fanotify(u32 response)
> > +static inline void audit_fanotify(u32 response, struct
> > fanotify_response_info_audit_rule *friar) { }
> > 
> >  static inline void audit_tk_injoffset(struct timespec64 offset)
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index d1fb821de104..8d523066d81f 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -64,6 +64,7 @@
> >  #include <uapi/linux/limits.h>
> >  #include <uapi/linux/netfilter/nf_tables.h>
> >  #include <uapi/linux/openat2.h> // struct open_how
> > +#include <uapi/linux/fanotify.h>
> > 
> >  #include "audit.h"
> > 
> > @@ -2877,10 +2878,28 @@ void __audit_log_kern_module(char *name)
> >  	context->type = AUDIT_KERN_MODULE;
> >  }
> > 
> > -void __audit_fanotify(u32 response)
> > +void __audit_fanotify(u32 response, struct
> > fanotify_response_info_audit_rule *friar) {
> > -	audit_log(audit_context(), GFP_KERNEL,
> > -		AUDIT_FANOTIFY,	"resp=%u", response);
> > +	struct audit_context *ctx = audit_context();
> > +	struct audit_buffer *ab;
> > +	char numbuf[12];
> > +
> > +	if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) {
> > +		audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > +			  "resp=%u fan_type=%u fan_info=3F subj_trust=2 
> obj_trust=2",
> > +			  response, FAN_RESPONSE_INFO_NONE);
> > +		return;
> > +	}
> > +	ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY);
> > +	if (ab) {
> > +		audit_log_format(ab, "resp=%u fan_type=%u fan_info=",
> > +				 response, friar->hdr.type);
> > +		snprintf(numbuf, sizeof(numbuf), "%u", friar->rule_number);
> > +		audit_log_n_hex(ab, numbuf, sizeof(numbuf));
> 
> I don't think it needs to be converted to ascii and then hexencoded. As Paul 
> said, probably %X is all we need here.
> 
> -Steve
> 
> 
> > +		audit_log_format(ab, " subj_trust=%u obj_trust=%u",
> > +				 friar->subj_trust, friar->obj_trust);
> > +		audit_log_end(ab);
> > +	}
> >  }
> > 
> >  void __audit_tk_injoffset(struct timespec64 offset)
> 
> 
> 
> 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 3/3] fanotify,audit: Allow audit to use the full permission event response
  2023-01-10  3:08     ` Richard Guy Briggs
@ 2023-01-10 15:26       ` Steve Grubb
  2023-01-10 18:32         ` [PATCH v5 3/3] fanotify, audit: " Richard Guy Briggs
  0 siblings, 1 reply; 20+ messages in thread
From: Steve Grubb @ 2023-01-10 15:26 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, linux-fsdevel, linux-api,
	Paul Moore, Eric Paris, Jan Kara, Amir Goldstein

Hello Richard,

On Monday, January 9, 2023 10:08:04 PM EST Richard Guy Briggs wrote:
> When I use an application that expected the old API, meaning it simply
> does:
> > 
> > response.fd = metadata->fd;
> > response.response = reply;
> > close(metadata->fd);
> > write(fd, &response, sizeof(struct fanotify_response));
> > 
> > I get access denials. Every time. If the program is using the new API and
> > sets FAN_INFO, then it works as expected. I'll do some more testing but I
> > think there is something wrong in the compatibility path.
> 
> I'll have a closer look, because this wasn't the intended behaviour.

I have done more testing. I think what I saw might have been caused by a 
stale selinux label (label exists, policy is deleted). With selinux in 
permissive mode it's all working as expected - both old and new API.

-Steve



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 3/3] fanotify, audit: Allow audit to use the full permission event response
  2023-01-10 15:26       ` Steve Grubb
@ 2023-01-10 18:32         ` Richard Guy Briggs
  0 siblings, 0 replies; 20+ messages in thread
From: Richard Guy Briggs @ 2023-01-10 18:32 UTC (permalink / raw)
  To: Steve Grubb
  Cc: Jan Kara, linux-api, Amir Goldstein, LKML,
	Linux-Audit Mailing List, linux-fsdevel, Eric Paris

On 2023-01-10 10:26, Steve Grubb wrote:
> Hello Richard,
> 
> On Monday, January 9, 2023 10:08:04 PM EST Richard Guy Briggs wrote:
> > When I use an application that expected the old API, meaning it simply
> > does:
> > > 
> > > response.fd = metadata->fd;
> > > response.response = reply;
> > > close(metadata->fd);
> > > write(fd, &response, sizeof(struct fanotify_response));
> > > 
> > > I get access denials. Every time. If the program is using the new API and
> > > sets FAN_INFO, then it works as expected. I'll do some more testing but I
> > > think there is something wrong in the compatibility path.
> > 
> > I'll have a closer look, because this wasn't the intended behaviour.
> 
> I have done more testing. I think what I saw might have been caused by a 
> stale selinux label (label exists, policy is deleted). With selinux in 
> permissive mode it's all working as expected - both old and new API.

Ah good, thank you.

> -Steve

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 2/3] fanotify: define struct members to hold response decision context
  2023-01-03 12:42       ` Jan Kara
@ 2023-01-16 20:42         ` Richard Guy Briggs
  2023-01-17  8:27           ` Jan Kara
  0 siblings, 1 reply; 20+ messages in thread
From: Richard Guy Briggs @ 2023-01-16 20:42 UTC (permalink / raw)
  To: Jan Kara
  Cc: Linux-Audit Mailing List, LKML, linux-fsdevel, linux-api,
	Paul Moore, Eric Paris, Steve Grubb, Amir Goldstein

On 2023-01-03 13:42, Jan Kara wrote:
> On Thu 22-12-22 15:47:21, Richard Guy Briggs wrote:
> > On 2022-12-16 17:43, Jan Kara wrote:
> > > On Mon 12-12-22 09:06:10, Richard Guy Briggs wrote:
> > > > This patch adds a flag, FAN_INFO and an extensible buffer to provide
> > > > additional information about response decisions.  The buffer contains
> > > > one or more headers defining the information type and the length of the
> > > > following information.  The patch defines one additional information
> > > > type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number.  This will
> > > > allow for the creation of other information types in the future if other
> > > > users of the API identify different needs.
> > > > 
> > > > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > > > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > > > Suggested-by: Jan Kara <jack@suse.cz>
> > > > Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

> > > > +{
> > > > +	if (fd == FAN_NOFD)
> > > > +		return -ENOENT;
> > > 
> > > I would not test 'fd' in this function at all. After all it is not part of
> > > the response info structure and you do check it in
> > > process_access_response() anyway.
> > 
> > I wrestled with that.  I was even tempted to swallow the following fd
> > check too, but the flow would not have made as much sense for the
> > non-INFO case.
> > 
> > My understanding from Amir was that FAN_NOFD was only to be sent in in
> > conjuction with FAN_INFO to test if a newer kernel was present.
> 
> Yes, that is correct. But we not only want to check that FAN_INFO flag is
> understood (as you do in your patch) but also whether a particular response
> type is understood (which you don't verify for FAN_NOFD). Currently, there
> is only one response type (FAN_RESPONSE_INFO_AUDIT_RULE) but if there are
> more in the future we need old kernels to refuse new response types even
> for FAN_NOFD case.

Ok, I agree the NOFD check should be after.

> > I presumed that if FAN_NOFD was present without FAN_INFO that was an
> > invalid input to an old kernel.
> 
> Yes, that is correct and I agree the conditions I've suggested below are
> wrong in that regard and need a bit of tweaking. Thanks for catching it.
> 
> > > > +
> > > > +	if (info_len != sizeof(*friar))
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (copy_from_user(friar, info, sizeof(*friar)))
> > > > +		return -EFAULT;
> > > > +
> > > > +	if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
> > > > +		return -EINVAL;
> > > > +	if (friar->hdr.pad != 0)
> > > > +		return -EINVAL;
> > > > +	if (friar->hdr.len != sizeof(*friar))
> > > > +		return -EINVAL;
> > > > +
> > > > +	return info_len;
> > > > +}
> > > > +
> > > 
> > > ...
> > > 
> > > > @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
> > > >  		return -EINVAL;
> > > >  	}
> > > >  
> > > > -	if (fd < 0)
> > > > +	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > >  		return -EINVAL;
> > > >  
> > > > -	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > > +	if (response & FAN_INFO) {
> > > > +		ret = process_access_response_info(fd, info, info_len, &friar);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +	} else {
> > > > +		ret = 0;
> > > > +	}
> > > > +
> > > > +	if (fd < 0)
> > > >  		return -EINVAL;
> > > 
> > > And here I'd do:
> > > 
> > > 	if (fd == FAN_NOFD)
> > > 		return 0;
> > > 	if (fd < 0)
> > > 		return -EINVAL;
> > > 
> > > As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
> > > extra info is understood by the kernel so that application writing fanotify
> > > responses has a way to check which information it can provide to the
> > > kernel.
> > 
> > The reason for including it in process_access_response_info() is to make
> > sure that it is included in the FAN_INFO case to detect this extension.
> > If it were included here
> 
> I see what you're getting at now. So the condition
> 
>  	if (fd == FAN_NOFD)
>  		return 0;
> 
> needs to be moved into 
> 
> 	if (response & FAN_INFO)
> 
> branch after process_access_response_info(). I still prefer to keep it
> outside of the process_access_response_info() function itself as it looks
> more logical to me. Does it address your concerns?

Ok.  Note that this does not return zero to userspace, since this
function's return value is added to the size of the struct
fanotify_response when there is no error.

For that reason, I think it makes more sense to return -ENOENT, or some
other unused error code that fits, unless you think it is acceptable to
return sizeof(struct fanotify_response) when FAN_INFO is set to indicate
this.

> Jan Kara <jack@suse.com>

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 2/3] fanotify: define struct members to hold response decision context
  2023-01-16 20:42         ` Richard Guy Briggs
@ 2023-01-17  8:27           ` Jan Kara
  2023-01-17 19:33             ` Richard Guy Briggs
  0 siblings, 1 reply; 20+ messages in thread
From: Jan Kara @ 2023-01-17  8:27 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Jan Kara, Linux-Audit Mailing List, LKML, linux-fsdevel,
	linux-api, Paul Moore, Eric Paris, Steve Grubb, Amir Goldstein

On Mon 16-01-23 15:42:29, Richard Guy Briggs wrote:
> On 2023-01-03 13:42, Jan Kara wrote:
> > On Thu 22-12-22 15:47:21, Richard Guy Briggs wrote:
> > > > > +
> > > > > +	if (info_len != sizeof(*friar))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	if (copy_from_user(friar, info, sizeof(*friar)))
> > > > > +		return -EFAULT;
> > > > > +
> > > > > +	if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
> > > > > +		return -EINVAL;
> > > > > +	if (friar->hdr.pad != 0)
> > > > > +		return -EINVAL;
> > > > > +	if (friar->hdr.len != sizeof(*friar))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	return info_len;
> > > > > +}
> > > > > +
> > > > 
> > > > ...
> > > > 
> > > > > @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
> > > > >  		return -EINVAL;
> > > > >  	}
> > > > >  
> > > > > -	if (fd < 0)
> > > > > +	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > > >  		return -EINVAL;
> > > > >  
> > > > > -	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > > > +	if (response & FAN_INFO) {
> > > > > +		ret = process_access_response_info(fd, info, info_len, &friar);
> > > > > +		if (ret < 0)
> > > > > +			return ret;
> > > > > +	} else {
> > > > > +		ret = 0;
> > > > > +	}
> > > > > +
> > > > > +	if (fd < 0)
> > > > >  		return -EINVAL;
> > > > 
> > > > And here I'd do:
> > > > 
> > > > 	if (fd == FAN_NOFD)
> > > > 		return 0;
> > > > 	if (fd < 0)
> > > > 		return -EINVAL;
> > > > 
> > > > As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
> > > > extra info is understood by the kernel so that application writing fanotify
> > > > responses has a way to check which information it can provide to the
> > > > kernel.
> > > 
> > > The reason for including it in process_access_response_info() is to make
> > > sure that it is included in the FAN_INFO case to detect this extension.
> > > If it were included here
> > 
> > I see what you're getting at now. So the condition
> > 
> >  	if (fd == FAN_NOFD)
> >  		return 0;
> > 
> > needs to be moved into 
> > 
> > 	if (response & FAN_INFO)
> > 
> > branch after process_access_response_info(). I still prefer to keep it
> > outside of the process_access_response_info() function itself as it looks
> > more logical to me. Does it address your concerns?
> 
> Ok.  Note that this does not return zero to userspace, since this
> function's return value is added to the size of the struct
> fanotify_response when there is no error.

Right, good point. 0 is not a good return value in this case.

> For that reason, I think it makes more sense to return -ENOENT, or some
> other unused error code that fits, unless you think it is acceptable to
> return sizeof(struct fanotify_response) when FAN_INFO is set to indicate
> this.

Yeah, my intention was to indicate "success" to userspace so I'd like to
return whatever we return for the case when struct fanotify_response is
accepted for a normal file descriptor - looks like info_len is the right
value. Thanks!

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 2/3] fanotify: define struct members to hold response decision context
  2023-01-17  8:27           ` Jan Kara
@ 2023-01-17 19:33             ` Richard Guy Briggs
  0 siblings, 0 replies; 20+ messages in thread
From: Richard Guy Briggs @ 2023-01-17 19:33 UTC (permalink / raw)
  To: Jan Kara
  Cc: Linux-Audit Mailing List, LKML, linux-fsdevel, linux-api,
	Paul Moore, Eric Paris, Steve Grubb, Amir Goldstein

On 2023-01-17 09:27, Jan Kara wrote:
> On Mon 16-01-23 15:42:29, Richard Guy Briggs wrote:
> > On 2023-01-03 13:42, Jan Kara wrote:
> > > On Thu 22-12-22 15:47:21, Richard Guy Briggs wrote:
> > > > > > +
> > > > > > +	if (info_len != sizeof(*friar))
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	if (copy_from_user(friar, info, sizeof(*friar)))
> > > > > > +		return -EFAULT;
> > > > > > +
> > > > > > +	if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
> > > > > > +		return -EINVAL;
> > > > > > +	if (friar->hdr.pad != 0)
> > > > > > +		return -EINVAL;
> > > > > > +	if (friar->hdr.len != sizeof(*friar))
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	return info_len;
> > > > > > +}
> > > > > > +
> > > > > 
> > > > > ...
> > > > > 
> > > > > > @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
> > > > > >  		return -EINVAL;
> > > > > >  	}
> > > > > >  
> > > > > > -	if (fd < 0)
> > > > > > +	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > > > >  		return -EINVAL;
> > > > > >  
> > > > > > -	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > > > > +	if (response & FAN_INFO) {
> > > > > > +		ret = process_access_response_info(fd, info, info_len, &friar);
> > > > > > +		if (ret < 0)
> > > > > > +			return ret;
> > > > > > +	} else {
> > > > > > +		ret = 0;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (fd < 0)
> > > > > >  		return -EINVAL;
> > > > > 
> > > > > And here I'd do:
> > > > > 
> > > > > 	if (fd == FAN_NOFD)
> > > > > 		return 0;
> > > > > 	if (fd < 0)
> > > > > 		return -EINVAL;
> > > > > 
> > > > > As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
> > > > > extra info is understood by the kernel so that application writing fanotify
> > > > > responses has a way to check which information it can provide to the
> > > > > kernel.
> > > > 
> > > > The reason for including it in process_access_response_info() is to make
> > > > sure that it is included in the FAN_INFO case to detect this extension.
> > > > If it were included here
> > > 
> > > I see what you're getting at now. So the condition
> > > 
> > >  	if (fd == FAN_NOFD)
> > >  		return 0;
> > > 
> > > needs to be moved into 
> > > 
> > > 	if (response & FAN_INFO)
> > > 
> > > branch after process_access_response_info(). I still prefer to keep it
> > > outside of the process_access_response_info() function itself as it looks
> > > more logical to me. Does it address your concerns?
> > 
> > Ok.  Note that this does not return zero to userspace, since this
> > function's return value is added to the size of the struct
> > fanotify_response when there is no error.
> 
> Right, good point. 0 is not a good return value in this case.
> 
> > For that reason, I think it makes more sense to return -ENOENT, or some
> > other unused error code that fits, unless you think it is acceptable to
> > return sizeof(struct fanotify_response) when FAN_INFO is set to indicate
> > this.
> 
> Yeah, my intention was to indicate "success" to userspace so I'd like to
> return whatever we return for the case when struct fanotify_response is
> accepted for a normal file descriptor - looks like info_len is the right
> value. Thanks!

Ok, I hadn't thought of that.  So, to confirm, when FAN_INFO is set, if
FAN_NOFD is also set, return info_len from process_access_response() and
then immediately return sizeof(struct fanotify_response) plus info_len
to userspace without issuing an audit record should indicate support for
FAN_INFO and the specific info type supplied.

Thanks for helping work through this.

> 								Honza
> -- 
> Jan Kara <jack@suse.com>

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2023-01-17 21:11 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-12 14:06 [PATCH v5 0/3] fanotify: Allow user space to pass back additional audit info Richard Guy Briggs
2022-12-12 14:06 ` [PATCH v5 1/3] fanotify: Ensure consistent variable type for response Richard Guy Briggs
2022-12-20 23:30   ` Paul Moore
2022-12-12 14:06 ` [PATCH v5 2/3] fanotify: define struct members to hold response decision context Richard Guy Briggs
2022-12-16 16:43   ` Jan Kara
2022-12-16 17:05     ` Paul Moore
2022-12-19 10:10       ` Jan Kara
2022-12-22 20:47     ` Richard Guy Briggs
2023-01-03 12:42       ` Jan Kara
2023-01-16 20:42         ` Richard Guy Briggs
2023-01-17  8:27           ` Jan Kara
2023-01-17 19:33             ` Richard Guy Briggs
2022-12-12 14:06 ` [PATCH v5 3/3] fanotify,audit: Allow audit to use the full permission event response Richard Guy Briggs
2022-12-20 23:31   ` Paul Moore
2022-12-22 20:42     ` Richard Guy Briggs
2022-12-22 21:16       ` Paul Moore
2023-01-10  0:06   ` Steve Grubb
2023-01-10  3:08     ` Richard Guy Briggs
2023-01-10 15:26       ` Steve Grubb
2023-01-10 18:32         ` [PATCH v5 3/3] fanotify, audit: " Richard Guy Briggs

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).