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. This
is done by providing a a fully-formed FAN_INFO extension but setting the
fd to FAN_NOFD. On a capable kernel, it will succeed but issue no audit
record, whereas on an older kernel it will fail.
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=0 subj_trust=2 obj_trust=2
changelog:
v1:
- first version by Steve Grubb <[email protected]>
Link: https://lore.kernel.org/r/2042449.irdbgypaU6@x2
v2:
- enhancements suggested by Jan Kara <[email protected]>
- 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/[email protected]
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/[email protected]
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/[email protected]
v5:
- fixed warnings in p2/4 and p3/4 found by <[email protected]>
- 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/[email protected]
v6:
- simplify __audit_fanotify() from audit_log_format/audit_log_n_hex to audit_log/%X
- add comment to clarify {subj,obj}_trust values
- remove fd processing from process_access_response_info()
- return info_len immediately from process_access_response() on FAN_NOFD after process_access_response_info()
Link: https://lore.kernel.org/all/[email protected]
v7:
- change non FAN_INFO case to "0"
- change from if-return to switch(type)-case, which now ignores non-audit info
Link: https://lore.kernel.org/all/[email protected]
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 | 18 +++++-
7 files changed, 131 insertions(+), 33 deletions(-)
--
2.27.0
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 <[email protected]>.
The {subj,obj}_trust values are {0,1,2}, corresponding to no, yes, unknown.
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=0 subj_trust=2 obj_trust=2
Suggested-by: Steve Grubb <[email protected]>
Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2
Signed-off-by: Richard Guy Briggs <[email protected]>
---
fs/notify/fanotify/fanotify.c | 3 ++-
include/linux/audit.h | 9 +++++----
kernel/auditsc.c | 18 +++++++++++++++---
3 files changed, 22 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..5a5994659b44 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,21 @@ 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);
+ /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */
+ switch (friar->hdr.type) {
+ case FAN_RESPONSE_INFO_NONE:
+ audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
+ "resp=%u fan_type=%u fan_info=0 subj_trust=2 obj_trust=2",
+ response, FAN_RESPONSE_INFO_NONE);
+ break;
+ case FAN_RESPONSE_INFO_AUDIT_RULE:
+ 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);
+ }
}
void __audit_tk_injoffset(struct timespec64 offset)
--
2.27.0
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.
The kernel can be tested if it supports a given info type by supplying
the complete info extension but setting fd to FAN_NOFD. It will return
the expected size but not issue an audit record.
Suggested-by: Steve Grubb <[email protected]>
Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
Suggested-by: Jan Kara <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Richard Guy Briggs <[email protected]>
---
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..8f430bfad487 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -283,19 +283,42 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
return client_fd;
}
+static int process_access_response_info(const char __user *info,
+ size_t info_len,
+ struct fanotify_response_info_audit_rule *friar)
+{
+ 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 +329,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 +357,20 @@ 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(info, info_len, &friar);
+ if (ret < 0)
+ return ret;
+ if (fd == FAN_NOFD)
+ 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
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 <[email protected]>
Link: https://lore.kernel.org/r/12617626.uLZWGnKmhe@x2
Signed-off-by: Richard Guy Briggs <[email protected]>
Acked-by: Paul Moore <[email protected]>
---
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
Hello Richard,
On Friday, February 3, 2023 4:35:13 PM EST Richard Guy Briggs wrote:
> 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. This
> is done by providing a a fully-formed FAN_INFO extension but setting the
> fd to FAN_NOFD. On a capable kernel, it will succeed but issue no audit
> record, whereas on an older kernel it will fail.
I have taken the patches and built a new kernel. Everything looks good to me
on the audit side. It's acting as expected. You can put me down for an ACK or
tested-by or whatever is appropriate.
-Steve
> 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=0 subj_trust=2
> obj_trust=2
>
> changelog:
> v1:
> - first version by Steve Grubb <[email protected]>
> Link: https://lore.kernel.org/r/2042449.irdbgypaU6@x2
>
> v2:
> - enhancements suggested by Jan Kara <[email protected]>
> - 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/[email protected]
>
> 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/[email protected]
>
> 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/[email protected]
>
> v5:
> - fixed warnings in p2/4 and p3/4 found by <[email protected]>
> - 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/[email protected]
>
> v6:
> - simplify __audit_fanotify() from audit_log_format/audit_log_n_hex to
> audit_log/%X - add comment to clarify {subj,obj}_trust values
> - remove fd processing from process_access_response_info()
> - return info_len immediately from process_access_response() on FAN_NOFD
> after process_access_response_info() Link:
> https://lore.kernel.org/all/[email protected]
>
> v7:
> - change non FAN_INFO case to "0"
> - change from if-return to switch(type)-case, which now ignores non-audit
> info Link: https://lore.kernel.org/all/[email protected]
>
> 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 | 18 +++++-
> 7 files changed, 131 insertions(+), 33 deletions(-)
On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote:
> 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. This
> is done by providing a a fully-formed FAN_INFO extension but setting the
> fd to FAN_NOFD. On a capable kernel, it will succeed but issue no audit
> record, whereas on an older kernel it will fail.
>
> 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=0 subj_trust=2 obj_trust=2
Thanks! I've applied this series to my tree.
Honza
>
> changelog:
> v1:
> - first version by Steve Grubb <[email protected]>
> Link: https://lore.kernel.org/r/2042449.irdbgypaU6@x2
>
> v2:
> - enhancements suggested by Jan Kara <[email protected]>
> - 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/[email protected]
>
> 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/[email protected]
>
> 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/[email protected]
>
> v5:
> - fixed warnings in p2/4 and p3/4 found by <[email protected]>
> - 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/[email protected]
>
> v6:
> - simplify __audit_fanotify() from audit_log_format/audit_log_n_hex to audit_log/%X
> - add comment to clarify {subj,obj}_trust values
> - remove fd processing from process_access_response_info()
> - return info_len immediately from process_access_response() on FAN_NOFD after process_access_response_info()
> Link: https://lore.kernel.org/all/[email protected]
>
> v7:
> - change non FAN_INFO case to "0"
> - change from if-return to switch(type)-case, which now ignores non-audit info
> Link: https://lore.kernel.org/all/[email protected]
>
> 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 | 18 +++++-
> 7 files changed, 131 insertions(+), 33 deletions(-)
>
> --
> 2.27.0
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue, Feb 7, 2023 at 7:09 AM Jan Kara <[email protected]> wrote:
> On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote:
> > 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. This
> > is done by providing a a fully-formed FAN_INFO extension but setting the
> > fd to FAN_NOFD. On a capable kernel, it will succeed but issue no audit
> > record, whereas on an older kernel it will fail.
> >
> > 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=0 subj_trust=2 obj_trust=2
>
> Thanks! I've applied this series to my tree.
While I think this version of the patchset is fine, for future
reference it would have been nice if you had waited for my ACK on
patch 3/3; while Steve maintains his userspace tools, I'm the one
responsible for maintaining the Linux Kernel's audit subsystem.
--
paul-moore.com
On Tue 07-02-23 09:54:11, Paul Moore wrote:
> On Tue, Feb 7, 2023 at 7:09 AM Jan Kara <[email protected]> wrote:
> > On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote:
> > > 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. This
> > > is done by providing a a fully-formed FAN_INFO extension but setting the
> > > fd to FAN_NOFD. On a capable kernel, it will succeed but issue no audit
> > > record, whereas on an older kernel it will fail.
> > >
> > > 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=0 subj_trust=2 obj_trust=2
> >
> > Thanks! I've applied this series to my tree.
>
> While I think this version of the patchset is fine, for future
> reference it would have been nice if you had waited for my ACK on
> patch 3/3; while Steve maintains his userspace tools, I'm the one
> responsible for maintaining the Linux Kernel's audit subsystem.
Aha, I'm sorry for that. I had the impression that on the last version of
the series you've said you don't see anything for which the series should
be respun so once Steve's objections where addressed and you were silent
for a few days, I thought you consider the thing settled... My bad.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed, Feb 8, 2023 at 7:08 AM Jan Kara <[email protected]> wrote:
> On Tue 07-02-23 09:54:11, Paul Moore wrote:
> > On Tue, Feb 7, 2023 at 7:09 AM Jan Kara <[email protected]> wrote:
> > > On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote:
> > > > 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. This
> > > > is done by providing a a fully-formed FAN_INFO extension but setting the
> > > > fd to FAN_NOFD. On a capable kernel, it will succeed but issue no audit
> > > > record, whereas on an older kernel it will fail.
> > > >
> > > > 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=0 subj_trust=2 obj_trust=2
> > >
> > > Thanks! I've applied this series to my tree.
> >
> > While I think this version of the patchset is fine, for future
> > reference it would have been nice if you had waited for my ACK on
> > patch 3/3; while Steve maintains his userspace tools, I'm the one
> > responsible for maintaining the Linux Kernel's audit subsystem.
>
> Aha, I'm sorry for that. I had the impression that on the last version of
> the series you've said you don't see anything for which the series should
> be respun so once Steve's objections where addressed and you were silent
> for a few days, I thought you consider the thing settled... My bad.
That's understandable, especially given inconsistencies across
subsystems. If it helps, if I'm going to ACK something I make it
explicit with a proper 'Acked-by: ...' line in my reply; if I say
something looks good but there is no explicit ACK, there is usually
something outstanding that needs to be resolved, e.g. questions,
additional testing, etc.
In this particular case I posed some questions in that thread and
never saw a reply with any answers, hence the lack of an ACK. While I
think the patches were reasonable, I withheld my ACK until the
questions were answered ... which they never were from what I can
tell, we just saw a new patchset with changes.
/me shrugs
--
paul-moore.com
On Wednesday, February 8, 2023 10:03:24 AM EST Paul Moore wrote:
> On Wed, Feb 8, 2023 at 7:08 AM Jan Kara <[email protected]> wrote:
> > On Tue 07-02-23 09:54:11, Paul Moore wrote:
> > > On Tue, Feb 7, 2023 at 7:09 AM Jan Kara <[email protected]> wrote:
> > > > On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote:
> > > > > 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.
> > > > > This
> > > > > is done by providing a a fully-formed FAN_INFO extension but
> > > > > setting the
> > > > > fd to FAN_NOFD. On a capable kernel, it will succeed but issue no
> > > > > audit
> > > > > record, whereas on an older kernel it will fail.
> > > > >
> > > > > 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=0
> > > > > subj_trust=2 obj_trust=2> > >
> > > > Thanks! I've applied this series to my tree.
> > >
> > > While I think this version of the patchset is fine, for future
> > > reference it would have been nice if you had waited for my ACK on
> > > patch 3/3; while Steve maintains his userspace tools, I'm the one
> > > responsible for maintaining the Linux Kernel's audit subsystem.
> >
> > Aha, I'm sorry for that. I had the impression that on the last version of
> > the series you've said you don't see anything for which the series should
> > be respun so once Steve's objections where addressed and you were silent
> > for a few days, I thought you consider the thing settled... My bad.
>
> That's understandable, especially given inconsistencies across
> subsystems. If it helps, if I'm going to ACK something I make it
> explicit with a proper 'Acked-by: ...' line in my reply; if I say
> something looks good but there is no explicit ACK, there is usually
> something outstanding that needs to be resolved, e.g. questions,
> additional testing, etc.
>
> In this particular case I posed some questions in that thread and
> never saw a reply with any answers, hence the lack of an ACK. While I
> think the patches were reasonable, I withheld my ACK until the
> questions were answered ... which they never were from what I can
> tell, we just saw a new patchset with changes.
>
> /me shrugs
Paul,
I reread the thread. You only had a request to change if/else to a switch
construct only if there was a respin for the 3F. You otherwise said get
Steve's input and the 3F borders on being overly clever. Both were addressed.
If you had other questions that needed answers on, please restate them to
expedite approval of this set of patches. As far as I can tell, all comments
are addressed.
Best,
-Steve
On Wed, Feb 8, 2023 at 10:27 AM Steve Grubb <[email protected]> wrote:
> On Wednesday, February 8, 2023 10:03:24 AM EST Paul Moore wrote:
> > On Wed, Feb 8, 2023 at 7:08 AM Jan Kara <[email protected]> wrote:
> > > On Tue 07-02-23 09:54:11, Paul Moore wrote:
> > > > On Tue, Feb 7, 2023 at 7:09 AM Jan Kara <[email protected]> wrote:
> > > > > On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote:
> > > > > > 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.
> > > > > > This
> > > > > > is done by providing a a fully-formed FAN_INFO extension but
> > > > > > setting the
> > > > > > fd to FAN_NOFD. On a capable kernel, it will succeed but issue no
> > > > > > audit
> > > > > > record, whereas on an older kernel it will fail.
> > > > > >
> > > > > > 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=0
> > > > > > subj_trust=2 obj_trust=2> > >
> > > > > Thanks! I've applied this series to my tree.
> > > >
> > > > While I think this version of the patchset is fine, for future
> > > > reference it would have been nice if you had waited for my ACK on
> > > > patch 3/3; while Steve maintains his userspace tools, I'm the one
> > > > responsible for maintaining the Linux Kernel's audit subsystem.
> > >
> > > Aha, I'm sorry for that. I had the impression that on the last version of
> > > the series you've said you don't see anything for which the series should
> > > be respun so once Steve's objections where addressed and you were silent
> > > for a few days, I thought you consider the thing settled... My bad.
> >
> > That's understandable, especially given inconsistencies across
> > subsystems. If it helps, if I'm going to ACK something I make it
> > explicit with a proper 'Acked-by: ...' line in my reply; if I say
> > something looks good but there is no explicit ACK, there is usually
> > something outstanding that needs to be resolved, e.g. questions,
> > additional testing, etc.
> >
> > In this particular case I posed some questions in that thread and
> > never saw a reply with any answers, hence the lack of an ACK. While I
> > think the patches were reasonable, I withheld my ACK until the
> > questions were answered ... which they never were from what I can
> > tell, we just saw a new patchset with changes.
> >
> > /me shrugs
>
> Paul,
>
> I reread the thread. You only had a request to change if/else to a switch
> construct only if there was a respin for the 3F. You otherwise said get
> Steve's input and the 3F borders on being overly clever. Both were addressed.
> If you had other questions that needed answers on, please restate them to
> expedite approval of this set of patches. As far as I can tell, all comments
> are addressed.
Steve,
It might be helpful to reread my reply below:
https://lore.kernel.org/linux-audit/CAHC9VhRWDD6Tk6AEmgoobBkcVKRYbVOte7-F0TGJD2dRk7NKxw@mail.gmail.com/
You'll see that I made a comment in that email about not following
Richard's explanation about "encoding the zero" (the patch was
encoding a "?" to the best I could tell). I was hoping for some
clarification from Richard on his comments, and I never saw anything
in my inbox. I just checked the archives on lore and I don't see
anything there either.
--
paul-moore.com
On 2023-02-08 11:24, Paul Moore wrote:
> On Wed, Feb 8, 2023 at 10:27 AM Steve Grubb <[email protected]> wrote:
> > On Wednesday, February 8, 2023 10:03:24 AM EST Paul Moore wrote:
> > > On Wed, Feb 8, 2023 at 7:08 AM Jan Kara <[email protected]> wrote:
> > > > On Tue 07-02-23 09:54:11, Paul Moore wrote:
> > > > > On Tue, Feb 7, 2023 at 7:09 AM Jan Kara <[email protected]> wrote:
> > > > > > On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote:
> > > > > > > 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.
> > > > > > > This
> > > > > > > is done by providing a a fully-formed FAN_INFO extension but
> > > > > > > setting the
> > > > > > > fd to FAN_NOFD. On a capable kernel, it will succeed but issue no
> > > > > > > audit
> > > > > > > record, whereas on an older kernel it will fail.
> > > > > > >
> > > > > > > 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=0
> > > > > > > subj_trust=2 obj_trust=2> > >
> > > > > > Thanks! I've applied this series to my tree.
> > > > >
> > > > > While I think this version of the patchset is fine, for future
> > > > > reference it would have been nice if you had waited for my ACK on
> > > > > patch 3/3; while Steve maintains his userspace tools, I'm the one
> > > > > responsible for maintaining the Linux Kernel's audit subsystem.
> > > >
> > > > Aha, I'm sorry for that. I had the impression that on the last version of
> > > > the series you've said you don't see anything for which the series should
> > > > be respun so once Steve's objections where addressed and you were silent
> > > > for a few days, I thought you consider the thing settled... My bad.
> > >
> > > That's understandable, especially given inconsistencies across
> > > subsystems. If it helps, if I'm going to ACK something I make it
> > > explicit with a proper 'Acked-by: ...' line in my reply; if I say
> > > something looks good but there is no explicit ACK, there is usually
> > > something outstanding that needs to be resolved, e.g. questions,
> > > additional testing, etc.
> > >
> > > In this particular case I posed some questions in that thread and
> > > never saw a reply with any answers, hence the lack of an ACK. While I
> > > think the patches were reasonable, I withheld my ACK until the
> > > questions were answered ... which they never were from what I can
> > > tell, we just saw a new patchset with changes.
> > >
> > > /me shrugs
> >
> > Paul,
> >
> > I reread the thread. You only had a request to change if/else to a switch
> > construct only if there was a respin for the 3F. You otherwise said get
> > Steve's input and the 3F borders on being overly clever. Both were addressed.
> > If you had other questions that needed answers on, please restate them to
> > expedite approval of this set of patches. As far as I can tell, all comments
> > are addressed.
>
> Steve,
>
> It might be helpful to reread my reply below:
>
> https://lore.kernel.org/linux-audit/CAHC9VhRWDD6Tk6AEmgoobBkcVKRYbVOte7-F0TGJD2dRk7NKxw@mail.gmail.com/
>
> You'll see that I made a comment in that email about not following
> Richard's explanation about "encoding the zero" (the patch was
> encoding a "?" to the best I could tell). I was hoping for some
> clarification from Richard on his comments, and I never saw anything
> in my inbox. I just checked the archives on lore and I don't see
> anything there either.
Well, it could have been any of:
?
"?"
3F
30
0
I can't answer that. My preference is for 3F but good arguments can be
made for any of these. I defer to Steve since it is his tools and
customers that have to deal with it.
> paul-moore.com
- RGB
--
Richard Guy Briggs <[email protected]>
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
On Wed, Feb 8, 2023 at 12:37 PM Richard Guy Briggs <[email protected]> wrote:
> On 2023-02-08 11:24, Paul Moore wrote:
> > On Wed, Feb 8, 2023 at 10:27 AM Steve Grubb <[email protected]> wrote:
> > > On Wednesday, February 8, 2023 10:03:24 AM EST Paul Moore wrote:
> > > > On Wed, Feb 8, 2023 at 7:08 AM Jan Kara <[email protected]> wrote:
> > > > > On Tue 07-02-23 09:54:11, Paul Moore wrote:
> > > > > > On Tue, Feb 7, 2023 at 7:09 AM Jan Kara <[email protected]> wrote:
> > > > > > > On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote:
> > > > > > > > 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.
> > > > > > > > This
> > > > > > > > is done by providing a a fully-formed FAN_INFO extension but
> > > > > > > > setting the
> > > > > > > > fd to FAN_NOFD. On a capable kernel, it will succeed but issue no
> > > > > > > > audit
> > > > > > > > record, whereas on an older kernel it will fail.
> > > > > > > >
> > > > > > > > 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=0
> > > > > > > > subj_trust=2 obj_trust=2> > >
> > > > > > > Thanks! I've applied this series to my tree.
> > > > > >
> > > > > > While I think this version of the patchset is fine, for future
> > > > > > reference it would have been nice if you had waited for my ACK on
> > > > > > patch 3/3; while Steve maintains his userspace tools, I'm the one
> > > > > > responsible for maintaining the Linux Kernel's audit subsystem.
> > > > >
> > > > > Aha, I'm sorry for that. I had the impression that on the last version of
> > > > > the series you've said you don't see anything for which the series should
> > > > > be respun so once Steve's objections where addressed and you were silent
> > > > > for a few days, I thought you consider the thing settled... My bad.
> > > >
> > > > That's understandable, especially given inconsistencies across
> > > > subsystems. If it helps, if I'm going to ACK something I make it
> > > > explicit with a proper 'Acked-by: ...' line in my reply; if I say
> > > > something looks good but there is no explicit ACK, there is usually
> > > > something outstanding that needs to be resolved, e.g. questions,
> > > > additional testing, etc.
> > > >
> > > > In this particular case I posed some questions in that thread and
> > > > never saw a reply with any answers, hence the lack of an ACK. While I
> > > > think the patches were reasonable, I withheld my ACK until the
> > > > questions were answered ... which they never were from what I can
> > > > tell, we just saw a new patchset with changes.
> > > >
> > > > /me shrugs
> > >
> > > Paul,
> > >
> > > I reread the thread. You only had a request to change if/else to a switch
> > > construct only if there was a respin for the 3F. You otherwise said get
> > > Steve's input and the 3F borders on being overly clever. Both were addressed.
> > > If you had other questions that needed answers on, please restate them to
> > > expedite approval of this set of patches. As far as I can tell, all comments
> > > are addressed.
> >
> > Steve,
> >
> > It might be helpful to reread my reply below:
> >
> > https://lore.kernel.org/linux-audit/CAHC9VhRWDD6Tk6AEmgoobBkcVKRYbVOte7-F0TGJD2dRk7NKxw@mail.gmail.com/
> >
> > You'll see that I made a comment in that email about not following
> > Richard's explanation about "encoding the zero" (the patch was
> > encoding a "?" to the best I could tell). I was hoping for some
> > clarification from Richard on his comments, and I never saw anything
> > in my inbox. I just checked the archives on lore and I don't see
> > anything there either.
>
> Well, it could have been any of:
> ?
> "?"
> 3F
> 30
> 0
>
> I can't answer that. My preference is for 3F but good arguments can be
> made for any of these. I defer to Steve since it is his tools and
> customers that have to deal with it.
Yeah, that's the kind of discussion I was hoping to have in that
thread. Like I said, the patch is fine, but it's nice to see some
discussion around things like this, and not just new patchset
revisions, so there is some understanding of why things are the way
they are.
/me shrugs again
--
paul-moore.com
On Wed 08-02-23 10:03:24, Paul Moore wrote:
> On Wed, Feb 8, 2023 at 7:08 AM Jan Kara <[email protected]> wrote:
> > On Tue 07-02-23 09:54:11, Paul Moore wrote:
> > > On Tue, Feb 7, 2023 at 7:09 AM Jan Kara <[email protected]> wrote:
> > > > On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote:
> > > > > 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. This
> > > > > is done by providing a a fully-formed FAN_INFO extension but setting the
> > > > > fd to FAN_NOFD. On a capable kernel, it will succeed but issue no audit
> > > > > record, whereas on an older kernel it will fail.
> > > > >
> > > > > 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=0 subj_trust=2 obj_trust=2
> > > >
> > > > Thanks! I've applied this series to my tree.
> > >
> > > While I think this version of the patchset is fine, for future
> > > reference it would have been nice if you had waited for my ACK on
> > > patch 3/3; while Steve maintains his userspace tools, I'm the one
> > > responsible for maintaining the Linux Kernel's audit subsystem.
> >
> > Aha, I'm sorry for that. I had the impression that on the last version of
> > the series you've said you don't see anything for which the series should
> > be respun so once Steve's objections where addressed and you were silent
> > for a few days, I thought you consider the thing settled... My bad.
>
> That's understandable, especially given inconsistencies across
> subsystems. If it helps, if I'm going to ACK something I make it
> explicit with a proper 'Acked-by: ...' line in my reply; if I say
> something looks good but there is no explicit ACK, there is usually
> something outstanding that needs to be resolved, e.g. questions,
> additional testing, etc.
Ok, thanks for letting me now. Next time I'll wait for an explicit ack from
you. This time, since everybody is fine with the actual patch, let's just
move on ;).
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR