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 patch defines 2 additional fields within the response structure
returned from user space on a permission event. The first field is 32
bits for the context type. The context type will describe what the
meaning is of the second field. The audit system will separate the
pieces and log them individually.
The audit function was updated to log the additional information in the
AUDIT_FANOTIFY record. The following is an example of the new record
format:
type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17
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/r/[email protected]
Richard Guy Briggs (3):
fanotify: Ensure consistent variable type for response
fanotify: define struct members to hold response decision context
fanotify: Allow audit to use the full permission event response
fs/notify/fanotify/fanotify.c | 6 ++-
fs/notify/fanotify/fanotify.h | 4 +-
fs/notify/fanotify/fanotify_user.c | 76 +++++++++++++++++++-----------
include/linux/audit.h | 9 ++--
include/linux/fanotify.h | 3 ++
include/uapi/linux/fanotify.h | 22 ++++++++-
kernel/auditsc.c | 18 +++++--
7 files changed, 100 insertions(+), 38 deletions(-)
--
2.27.0
This patch adds 2 structure members to the response returned from user
space on a permission event. The first field is 32 bits for the context
type. The context type will describe what the meaning is of the second
field. The default is none. The patch defines one additional context
type which means that the second field is a union containing a 32-bit
rule number. This will allow for the creation of other context types in
the future if other users of the API identify different needs. The
second field size is defined by the context type and can be used to pass
along the data described by the context.
To support this, there is a macro for user space to check that the data
being sent is valid. Of course, without this check, anything that
overflows the bit field will trigger an EINVAL based on the use of
FAN_INVALID_RESPONSE_MASK in process_access_response().
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 | 2 +-
fs/notify/fanotify/fanotify.h | 2 +
fs/notify/fanotify/fanotify_user.c | 74 +++++++++++++++++++-----------
include/linux/fanotify.h | 3 ++
include/uapi/linux/fanotify.h | 22 ++++++++-
5 files changed, 75 insertions(+), 28 deletions(-)
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 985e995d2a39..ea0e60488f12 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 & ~(FAN_AUDIT | FAN_EXTRA)) {
case FAN_ALLOW:
ret = 0;
break;
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index d66668e06bee..eb7ec1f2a26e 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -426,8 +426,10 @@ struct fanotify_perm_event {
struct fanotify_event fae;
struct path path;
u32 response; /* userspace answer to the event */
+ u32 extra_info_type;
unsigned short state; /* state of the event */
int fd; /* fd we passed to userspace for this event */
+ union fanotify_response_extra extra_info;
};
static inline struct fanotify_perm_event *
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 721e777ea90b..1c4067e29f2e 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -289,13 +289,22 @@ static int create_fd(struct fsnotify_group *group, struct path *path,
*/
static void finish_permission_event(struct fsnotify_group *group,
struct fanotify_perm_event *event,
- u32 response)
+ struct fanotify_response *response)
__releases(&group->notification_lock)
{
bool destroy = false;
assert_spin_locked(&group->notification_lock);
- event->response = response;
+ event->response = response->response & ~FAN_EXTRA;
+ if (response->response & FAN_EXTRA) {
+ event->extra_info_type = response->extra_info_type;
+ switch (event->extra_info_type) {
+ case FAN_RESPONSE_INFO_AUDIT_RULE:
+ event->extra_info.audit_rule = response->extra_info.audit_rule;
+ }
+ } else {
+ event->extra_info_type = FAN_RESPONSE_INFO_NONE;
+ }
if (event->state == FAN_EVENT_CANCELED)
destroy = true;
else
@@ -306,33 +315,40 @@ 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,
+ size_t count)
{
struct fanotify_perm_event *event;
int fd = response_struct->fd;
u32 response = response_struct->response;
- pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
- fd, response);
+ pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
+ group, fd, response, response_struct->extra_info_type, count);
+ if (fd < 0)
+ return -EINVAL;
/*
* 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) {
- case FAN_ALLOW:
- case FAN_DENY:
- break;
- default:
- return -EINVAL;
- }
-
- if (fd < 0)
+ if (FAN_INVALID_RESPONSE_MASK(response))
return -EINVAL;
-
if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
return -EINVAL;
-
+ if (response & FAN_EXTRA) {
+ if (count < offsetofend(struct fanotify_response, extra_info_type))
+ return -EINVAL;
+ switch (response_struct->extra_info_type) {
+ case FAN_RESPONSE_INFO_NONE:
+ break;
+ case FAN_RESPONSE_INFO_AUDIT_RULE:
+ if (count < offsetofend(struct fanotify_response, extra_info))
+ return -EINVAL;
+ break;
+ default:
+ return -EINVAL;
+ }
+ }
spin_lock(&group->notification_lock);
list_for_each_entry(event, &group->fanotify_data.access_list,
fae.fse.list) {
@@ -340,7 +356,7 @@ 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_struct);
wake_up(&group->fanotify_data.access_waitq);
return 0;
}
@@ -802,9 +818,13 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
fsnotify_destroy_event(group, &event->fse);
} else {
if (ret <= 0) {
+ struct fanotify_response response = {
+ .fd = FAN_NOFD,
+ .response = FAN_DENY };
+
spin_lock(&group->notification_lock);
finish_permission_event(group,
- FANOTIFY_PERM(event), FAN_DENY);
+ FANOTIFY_PERM(event), &response);
wake_up(&group->fanotify_data.access_waitq);
} else {
spin_lock(&group->notification_lock);
@@ -827,26 +847,25 @@ 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;
+ size_t size = min(count, sizeof(struct fanotify_response));
if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
return -EINVAL;
group = file->private_data;
- if (count < sizeof(response))
+ if (count < offsetofend(struct fanotify_response, 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, size))
return -EFAULT;
- ret = process_access_response(group, &response);
+ ret = process_access_response(group, &response, count);
if (ret < 0)
count = ret;
@@ -857,6 +876,9 @@ static int fanotify_release(struct inode *ignored, struct file *file)
{
struct fsnotify_group *group = file->private_data;
struct fsnotify_event *fsn_event;
+ struct fanotify_response response = {
+ .fd = FAN_NOFD,
+ .response = FAN_ALLOW };
/*
* Stop new events from arriving in the notification queue. since
@@ -876,7 +898,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, &response);
spin_lock(&group->notification_lock);
}
@@ -893,7 +915,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);
+ &response);
}
spin_lock(&group->notification_lock);
}
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index 419cadcd7ff5..63a8494e782e 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -113,6 +113,9 @@
#define ALL_FANOTIFY_EVENT_BITS (FANOTIFY_OUTGOING_EVENTS | \
FANOTIFY_EVENT_FLAGS)
+/* This mask is to check for invalid bits of a user space permission response */
+#define FAN_INVALID_RESPONSE_MASK(x) ((x) & ~(FAN_ALLOW | FAN_DENY | FAN_AUDIT | FAN_EXTRA))
+
/* 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 e8ac38cc2fd6..a94f4143601f 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -179,15 +179,35 @@ 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 informaion buffer whose
+ * size is determined by the extra information type.
+ *
+ * If the context type is Rule, then the context following is the rule number
+ * that triggered the user space decision.
+ */
+
+#define FAN_RESPONSE_INFO_NONE 0
+#define FAN_RESPONSE_INFO_AUDIT_RULE 1
+
+union fanotify_response_extra {
+ __u32 audit_rule;
+};
+
struct fanotify_response {
__s32 fd;
__u32 response;
+ __u32 extra_info_type;
+ union fanotify_response_extra extra_info;
};
/* 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_EXTRA 0x20 /* Bitmask to indicate additional information */
/* No fd set in event */
#define FAN_NOFD -1
--
2.27.0
This patch passes the full value 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. The following is an example of the new record
format:
type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17
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 | 4 +++-
include/linux/audit.h | 9 +++++----
kernel/auditsc.c | 18 +++++++++++++++---
3 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index ea0e60488f12..85ce36e59e0c 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -273,7 +273,9 @@ 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->extra_info_type,
+ &event->extra_info);
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 217784d602b3..737f1c109aa1 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)
@@ -419,7 +420,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, u32 type, union fanotify_response_extra *info);
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,
@@ -526,10 +527,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, u32 type, union fanotify_response_extra *info)
{
if (!audit_dummy_context())
- __audit_fanotify(response);
+ __audit_fanotify(response, type, info);
}
static inline void audit_tk_injoffset(struct timespec64 offset)
@@ -686,7 +687,7 @@ static inline void audit_log_kern_module(char *name)
{
}
-static inline void audit_fanotify(u32 response)
+static inline void audit_fanotify(u32 response, u32 type, union fanotify_response_extra *info)
{ }
static inline void audit_tk_injoffset(struct timespec64 offset)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 6973be0bf6c9..cb93c6ed07cd 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"
@@ -2893,10 +2894,21 @@ void __audit_log_kern_module(char *name)
context->type = AUDIT_KERN_MODULE;
}
-void __audit_fanotify(u32 response)
+void __audit_fanotify(u32 response, u32 type, union fanotify_response_extra *info)
{
- audit_log(audit_context(), GFP_KERNEL,
- AUDIT_FANOTIFY, "resp=%u", response);
+ switch (type) {
+ case FAN_RESPONSE_INFO_AUDIT_RULE:
+ audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
+ "resp=%u fan_type=%u fan_ctx=%u",
+ response, type, info->audit_rule);
+ break;
+ case FAN_RESPONSE_INFO_NONE:
+ default:
+ audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
+ "resp=%u fan_type=%u fan_ctx=?",
+ response, type);
+ break;
+ }
}
void __audit_tk_injoffset(struct timespec64 offset)
--
2.27.0
On Mon, May 16, 2022 at 4:22 PM Richard Guy Briggs <[email protected]> wrote:
>
> This patch passes the full value 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. The following is an example of the new record
> format:
>
> type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17
>
> 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 | 4 +++-
> include/linux/audit.h | 9 +++++----
> kernel/auditsc.c | 18 +++++++++++++++---
> 3 files changed, 23 insertions(+), 8 deletions(-)
...
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 6973be0bf6c9..cb93c6ed07cd 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2893,10 +2894,21 @@ void __audit_log_kern_module(char *name)
> context->type = AUDIT_KERN_MODULE;
> }
>
> -void __audit_fanotify(u32 response)
> +void __audit_fanotify(u32 response, u32 type, union fanotify_response_extra *info)
> {
> - audit_log(audit_context(), GFP_KERNEL,
> - AUDIT_FANOTIFY, "resp=%u", response);
> + switch (type) {
> + case FAN_RESPONSE_INFO_AUDIT_RULE:
> + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> + "resp=%u fan_type=%u fan_ctx=%u",
> + response, type, info->audit_rule);
> + break;
> + case FAN_RESPONSE_INFO_NONE:
> + default:
> + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> + "resp=%u fan_type=%u fan_ctx=?",
> + response, type);
> + break;
> + }
> }
Two things:
* Instead of "fan_ctx=", would it make sense to call it "fan_extra="
to better match the UAPI struct? I don't feel strongly either way,
but it did occur to me just now while looking at the code so I thought
I would mention it.
* I'm also wondering if there is a way to be a bit proactive about
future proofing this field. Since we already hex encode some fields
with "bad" characters, would it make sense to hex encode this field
too? Not for the "bad" character reason, but more as a way of
marshalling the fanotify_response_extra union into an audit record. I
can't see far enough into the future to know if this would be a good
idea or not, but like the other point above, it popped into my head
while looking at the code so I thought I would put it in the email :)
--
paul-moore.com
Hi Richard,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jack-fs/fsnotify]
[also build test WARNING on linux/master linus/master v5.18-rc7 next-20220516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: m68k-defconfig (https://download.01.org/0day-ci/archive/20220517/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
git checkout 4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash fs/notify/fanotify/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from include/asm-generic/bug.h:22,
from arch/m68k/include/asm/bug.h:32,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from arch/m68k/include/asm/irqflags.h:6,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:7,
from include/linux/rcupdate.h:25,
from include/linux/sysctl.h:26,
from include/linux/fanotify.h:5,
from fs/notify/fanotify/fanotify_user.c:2:
fs/notify/fanotify/fanotify_user.c: In function 'process_access_response':
>> include/linux/kern_levels.h:5:25: warning: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:418:25: note: in definition of macro 'printk_index_wrap'
418 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:132:17: note: in expansion of macro 'printk'
132 | printk(fmt, ##__VA_ARGS__); \
| ^~~~~~
include/linux/printk.h:576:9: note: in expansion of macro 'no_printk'
576 | no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~
include/linux/kern_levels.h:15:25: note: in expansion of macro 'KERN_SOH'
15 | #define KERN_DEBUG KERN_SOH "7" /* debug-level messages */
| ^~~~~~~~
include/linux/printk.h:576:19: note: in expansion of macro 'KERN_DEBUG'
576 | no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~
fs/notify/fanotify/fanotify_user.c:325:9: note: in expansion of macro 'pr_debug'
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
| ^~~~~~~~
vim +5 include/linux/kern_levels.h
314ba3520e513a Joe Perches 2012-07-30 4
04d2c8c83d0e3a Joe Perches 2012-07-30 @5 #define KERN_SOH "\001" /* ASCII Start Of Header */
04d2c8c83d0e3a Joe Perches 2012-07-30 6 #define KERN_SOH_ASCII '\001'
04d2c8c83d0e3a Joe Perches 2012-07-30 7
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Tue, May 17, 2022 at 1:32 PM Jan Kara <[email protected]> wrote:
>
> On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> > On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs <[email protected]> wrote:
> > >
> > > This patch adds 2 structure members to the response returned from user
> > > space on a permission event. The first field is 32 bits for the context
> > > type. The context type will describe what the meaning is of the second
> > > field. The default is none. The patch defines one additional context
> > > type which means that the second field is a union containing a 32-bit
> > > rule number. This will allow for the creation of other context types in
> > > the future if other users of the API identify different needs. The
> > > second field size is defined by the context type and can be used to pass
> > > along the data described by the context.
> > >
> > > To support this, there is a macro for user space to check that the data
> > > being sent is valid. Of course, without this check, anything that
> > > overflows the bit field will trigger an EINVAL based on the use of
> > > FAN_INVALID_RESPONSE_MASK in process_access_response().
> > >
> > > 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]>
>
> ...
> > > static int process_access_response(struct fsnotify_group *group,
> > > - struct fanotify_response *response_struct)
> > > + struct fanotify_response *response_struct,
> > > + size_t count)
> > > {
> > > struct fanotify_perm_event *event;
> > > int fd = response_struct->fd;
> > > u32 response = response_struct->response;
> > >
> > > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > > - fd, response);
> > > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > > + group, fd, response, response_struct->extra_info_type, count);
> > > + if (fd < 0)
> > > + return -EINVAL;
> > > /*
> > > * 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) {
> > > - case FAN_ALLOW:
> > > - case FAN_DENY:
> > > - break;
> > > - default:
> > > - return -EINVAL;
> > > - }
> > > -
> > > - if (fd < 0)
> > > + if (FAN_INVALID_RESPONSE_MASK(response))
> >
> > That is a logic change, because now the response value of 0 becomes valid.
> >
> > Since you did not document this change in the commit message I assume this was
> > non intentional?
> > However, this behavior change is something that I did ask for, but it should be
> > done is a separate commit:
> >
> > /* These are NOT bitwise flags. Both bits can be used together. */
> > #define FAN_TEST 0x00
> > #define FAN_ALLOW 0x01
> > #define FAN_DENY 0x02
> > #define FANOTIFY_RESPONSE_ACCESS \
> > (FAN_TEST|FAN_ALLOW | FAN_DENY)
> >
> > ...
> > int access = response & FANOTIFY_RESPONSE_ACCESS;
> >
> > 1. Do return EINVAL for access == 0
> > 2. Let all the rest of the EINVAL checks run (including extra type)
> > 3. Move if (fd < 0) to last check
> > 4. Add if (!access) return 0 before if (fd < 0)
> >
> > That will provide a mechanism for userspace to probe the
> > kernel support for extra types in general and specific types
> > that it may respond with.
>
> I have to admit I didn't quite grok your suggestion here although I
> understand (and agree with) the general direction of the proposal :). Maybe
> code would explain it better what you have in mind?
>
+/* These are NOT bitwise flags. Both bits can be used together. */
+#define FAN_TEST 0x00
#define FAN_ALLOW 0x01
#define FAN_DENY 0x02
#define FAN_AUDIT 0x10 /* Bit mask to create audit record for result */
+#define FANOTIFY_RESPONSE_ACCESS \
+ (FAN_TEST|FAN_ALLOW | FAN_DENY)
...
@@ -311,6 +314,7 @@ static int process_access_response(struct
fsnotify_group *group,
struct fanotify_perm_event *event;
int fd = response_struct->fd;
int response = response_struct->response;
+ int access = response & FANOTIFY_RESPONSE_ACCESS;
pr_debug("%s: group=%p fd=%d response=%d\n", __func__, group,
fd, response);
@@ -319,18 +323,33 @@ static int process_access_response(struct
fsnotify_group *group,
* userspace can send a valid response or we will clean it up after the
* timeout
*/
- switch (response & ~FAN_AUDIT) {
+ if (!response)
+ return -EINVAL;
+
+ switch (access) {
case FAN_ALLOW:
case FAN_DENY:
+ case FAN_TEST:
break;
default:
return -EINVAL;
}
- if (fd < 0)
- return -EINVAL;
-
if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
return -EINVAL;
+ /*
+ * FAN_TEST|FAN_AUDIT response can be written during setup time to probe
+ * if the kernel has support for FAN_AUDIT.
+ * For FAN_TEST, fd must not be valid.
+ */
+ if (access == FAN_TEST) {
+ if (fd >= 0)
+ return -EINVAL;
+ return 0;
+ }
+
+ if (fd < 0)
+ return -EINVAL;
Thanks,
Amir.
On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs <[email protected]> wrote:
> >
> > This patch adds 2 structure members to the response returned from user
> > space on a permission event. The first field is 32 bits for the context
> > type. The context type will describe what the meaning is of the second
> > field. The default is none. The patch defines one additional context
> > type which means that the second field is a union containing a 32-bit
> > rule number. This will allow for the creation of other context types in
> > the future if other users of the API identify different needs. The
> > second field size is defined by the context type and can be used to pass
> > along the data described by the context.
> >
> > To support this, there is a macro for user space to check that the data
> > being sent is valid. Of course, without this check, anything that
> > overflows the bit field will trigger an EINVAL based on the use of
> > FAN_INVALID_RESPONSE_MASK in process_access_response().
> >
> > 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]>
...
> > static int process_access_response(struct fsnotify_group *group,
> > - struct fanotify_response *response_struct)
> > + struct fanotify_response *response_struct,
> > + size_t count)
> > {
> > struct fanotify_perm_event *event;
> > int fd = response_struct->fd;
> > u32 response = response_struct->response;
> >
> > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > - fd, response);
> > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > + group, fd, response, response_struct->extra_info_type, count);
> > + if (fd < 0)
> > + return -EINVAL;
> > /*
> > * 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) {
> > - case FAN_ALLOW:
> > - case FAN_DENY:
> > - break;
> > - default:
> > - return -EINVAL;
> > - }
> > -
> > - if (fd < 0)
> > + if (FAN_INVALID_RESPONSE_MASK(response))
>
> That is a logic change, because now the response value of 0 becomes valid.
>
> Since you did not document this change in the commit message I assume this was
> non intentional?
> However, this behavior change is something that I did ask for, but it should be
> done is a separate commit:
>
> /* These are NOT bitwise flags. Both bits can be used together. */
> #define FAN_TEST 0x00
> #define FAN_ALLOW 0x01
> #define FAN_DENY 0x02
> #define FANOTIFY_RESPONSE_ACCESS \
> (FAN_TEST|FAN_ALLOW | FAN_DENY)
>
> ...
> int access = response & FANOTIFY_RESPONSE_ACCESS;
>
> 1. Do return EINVAL for access == 0
> 2. Let all the rest of the EINVAL checks run (including extra type)
> 3. Move if (fd < 0) to last check
> 4. Add if (!access) return 0 before if (fd < 0)
>
> That will provide a mechanism for userspace to probe the
> kernel support for extra types in general and specific types
> that it may respond with.
I have to admit I didn't quite grok your suggestion here although I
understand (and agree with) the general direction of the proposal :). Maybe
code would explain it better what you have in mind?
> > +/*
> > + * 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 informaion buffer whose
>
> typo: informaion
>
> > + * size is determined by the extra information type.
> > + *
> > + * If the context type is Rule, then the context following is the rule number
> > + * that triggered the user space decision.
> > + */
> > +
> > +#define FAN_RESPONSE_INFO_NONE 0
> > +#define FAN_RESPONSE_INFO_AUDIT_RULE 1
> > +
> > +union fanotify_response_extra {
> > + __u32 audit_rule;
> > +};
> > +
> > struct fanotify_response {
> > __s32 fd;
> > __u32 response;
> > + __u32 extra_info_type;
> > + union fanotify_response_extra extra_info;
>
> IIRC, Jan wanted this to be a variable size record with info_type and info_len.
> I don't know if we want to make this flexible enough to allow for multiple
> records in the future like we do in events, but the common wisdom of
> the universe says that if we don't do it, we will need it.
Yes, please no unions in the API, that is always painful with the
alignment, size etc. What I had in mind was:
Keep fanotify_response as is:
struct fanotify_response {
__s32 fd;
__u32 response;
};
Define extra info header:
struct fanotify_response_info_header {
__u8 info_type;
__u8 pad;
__u16 len;
};
And then struct for your audit rule:
struct fanotify_response_info_audit_rule {
struct fanotify_response_info_header hdr;
__u32 audit_rule;
};
The verification in fanotify_write() then goes like:
struct fanotify_response response;
char extra_info_buf[sizeof(struct fanotify_response_info_audit_rule)];
if (copy_from_user(&response, buf, sizeof(response)))
return -EFAULT;
if (!(response.response & FAN_EXTRA_INFO)) {
count = 0;
} else {
count -= sizeof(response);
/* Simplistic parsing for now */
if (count != sizeof(struct fanotify_response_info_audit_rule))
return -EINVAL;
if (copy_from_user(extra_info_buf, buf, count)
return -EFAULT;
}
ret = process_access_response(group, &response, extra_info_buf, count);
And we pass extra_info_buf and count to audit_fanotify() where we need to do
further validation like:
struct fanotify_response_info_audit_rule *audit_response = NULL;
if (count > 0) {
/* Just one possible info type for now */
audit_response = (struct fanotify_response_info_audit_rule *)extra_info_buf;
if (audit_response->info_type != FAN_RESPONSE_INFO_AUDIT_RULE)
return -EINVAL;
if (audit_response->pad != 0)
return -EINVAL;
if (audit_response->len != sizeof(*audit_response))
return -EINVAL;
}
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
Hi Richard,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jack-fs/fsnotify]
[also build test WARNING on pcmoore-audit/next linux/master linus/master v5.18-rc7 next-20220516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20220517/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
git checkout 4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash fs/notify/fanotify/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from include/asm-generic/bug.h:22,
from arch/m68k/include/asm/bug.h:32,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from arch/m68k/include/asm/irqflags.h:6,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:7,
from include/linux/rcupdate.h:25,
from include/linux/sysctl.h:26,
from include/linux/fanotify.h:5,
from fs/notify/fanotify/fanotify_user.c:2:
fs/notify/fanotify/fanotify_user.c: In function 'process_access_response':
>> fs/notify/fanotify/fanotify_user.c:325:18: warning: format '%lu' expects argument of type 'long unsigned int', but argument 8 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/printk.h:336:21: note: in definition of macro 'pr_fmt'
336 | #define pr_fmt(fmt) fmt
| ^~~
include/linux/dynamic_debug.h:152:9: note: in expansion of macro '__dynamic_func_call'
152 | __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:162:9: note: in expansion of macro '_dynamic_func_call'
162 | _dynamic_func_call(fmt, __dynamic_pr_debug, \
| ^~~~~~~~~~~~~~~~~~
include/linux/printk.h:570:9: note: in expansion of macro 'dynamic_pr_debug'
570 | dynamic_pr_debug(fmt, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~
fs/notify/fanotify/fanotify_user.c:325:9: note: in expansion of macro 'pr_debug'
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
| ^~~~~~~~
fs/notify/fanotify/fanotify_user.c:325:65: note: format string is defined here
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
| ~~^
| |
| long unsigned int
| %u
vim +325 fs/notify/fanotify/fanotify_user.c
316
317 static int process_access_response(struct fsnotify_group *group,
318 struct fanotify_response *response_struct,
319 size_t count)
320 {
321 struct fanotify_perm_event *event;
322 int fd = response_struct->fd;
323 u32 response = response_struct->response;
324
> 325 pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
326 group, fd, response, response_struct->extra_info_type, count);
327 if (fd < 0)
328 return -EINVAL;
329 /*
330 * make sure the response is valid, if invalid we do nothing and either
331 * userspace can send a valid response or we will clean it up after the
332 * timeout
333 */
334 if (FAN_INVALID_RESPONSE_MASK(response))
335 return -EINVAL;
336 if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
337 return -EINVAL;
338 if (response & FAN_EXTRA) {
339 if (count < offsetofend(struct fanotify_response, extra_info_type))
340 return -EINVAL;
341 switch (response_struct->extra_info_type) {
342 case FAN_RESPONSE_INFO_NONE:
343 break;
344 case FAN_RESPONSE_INFO_AUDIT_RULE:
345 if (count < offsetofend(struct fanotify_response, extra_info))
346 return -EINVAL;
347 break;
348 default:
349 return -EINVAL;
350 }
351 }
352 spin_lock(&group->notification_lock);
353 list_for_each_entry(event, &group->fanotify_data.access_list,
354 fae.fse.list) {
355 if (event->fd != fd)
356 continue;
357
358 list_del_init(&event->fae.fse.list);
359 finish_permission_event(group, event, response_struct);
360 wake_up(&group->fanotify_data.access_waitq);
361 return 0;
362 }
363 spin_unlock(&group->notification_lock);
364
365 return -ENOENT;
366 }
367
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On 2022-05-16 21:42, Paul Moore wrote:
> On Mon, May 16, 2022 at 4:22 PM Richard Guy Briggs <[email protected]> wrote:
> >
> > This patch passes the full value 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. The following is an example of the new record
> > format:
> >
> > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17
> >
> > 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 | 4 +++-
> > include/linux/audit.h | 9 +++++----
> > kernel/auditsc.c | 18 +++++++++++++++---
> > 3 files changed, 23 insertions(+), 8 deletions(-)
>
> ...
>
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 6973be0bf6c9..cb93c6ed07cd 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2893,10 +2894,21 @@ void __audit_log_kern_module(char *name)
> > context->type = AUDIT_KERN_MODULE;
> > }
> >
> > -void __audit_fanotify(u32 response)
> > +void __audit_fanotify(u32 response, u32 type, union fanotify_response_extra *info)
> > {
> > - audit_log(audit_context(), GFP_KERNEL,
> > - AUDIT_FANOTIFY, "resp=%u", response);
> > + switch (type) {
> > + case FAN_RESPONSE_INFO_AUDIT_RULE:
> > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > + "resp=%u fan_type=%u fan_ctx=%u",
> > + response, type, info->audit_rule);
> > + break;
> > + case FAN_RESPONSE_INFO_NONE:
> > + default:
> > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > + "resp=%u fan_type=%u fan_ctx=?",
> > + response, type);
> > + break;
> > + }
> > }
>
> Two things:
>
> * Instead of "fan_ctx=", would it make sense to call it "fan_extra="
> to better match the UAPI struct? I don't feel strongly either way,
> but it did occur to me just now while looking at the code so I thought
> I would mention it.
Yes, this is a good point. This is the reason I changed from
FAN_RESPONSE_INFO_AUDIT_NONE to FAN_RESPONSE_INFO_NONE, anticipating
that the extra information could have nothing to do with audit.
> * I'm also wondering if there is a way to be a bit proactive about
> future proofing this field. Since we already hex encode some fields
> with "bad" characters, would it make sense to hex encode this field
> too? Not for the "bad" character reason, but more as a way of
> marshalling the fanotify_response_extra union into an audit record. I
> can't see far enough into the future to know if this would be a good
> idea or not, but like the other point above, it popped into my head
> while looking at the code so I thought I would put it in the email :)
I resisted that idea because it adds overhead and makes it more complex
than currently necessary. I'm open to it, but would like to hear
Steve's input on this.
Thanks for the quick response.
> 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 Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs <[email protected]> wrote:
>
> This patch adds 2 structure members to the response returned from user
> space on a permission event. The first field is 32 bits for the context
> type. The context type will describe what the meaning is of the second
> field. The default is none. The patch defines one additional context
> type which means that the second field is a union containing a 32-bit
> rule number. This will allow for the creation of other context types in
> the future if other users of the API identify different needs. The
> second field size is defined by the context type and can be used to pass
> along the data described by the context.
>
> To support this, there is a macro for user space to check that the data
> being sent is valid. Of course, without this check, anything that
> overflows the bit field will trigger an EINVAL based on the use of
> FAN_INVALID_RESPONSE_MASK in process_access_response().
>
> 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 | 2 +-
> fs/notify/fanotify/fanotify.h | 2 +
> fs/notify/fanotify/fanotify_user.c | 74 +++++++++++++++++++-----------
> include/linux/fanotify.h | 3 ++
> include/uapi/linux/fanotify.h | 22 ++++++++-
> 5 files changed, 75 insertions(+), 28 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 985e995d2a39..ea0e60488f12 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 & ~(FAN_AUDIT | FAN_EXTRA)) {
> case FAN_ALLOW:
> ret = 0;
> break;
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index d66668e06bee..eb7ec1f2a26e 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -426,8 +426,10 @@ struct fanotify_perm_event {
> struct fanotify_event fae;
> struct path path;
> u32 response; /* userspace answer to the event */
> + u32 extra_info_type;
> unsigned short state; /* state of the event */
> int fd; /* fd we passed to userspace for this event */
> + union fanotify_response_extra extra_info;
> };
>
> static inline struct fanotify_perm_event *
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 721e777ea90b..1c4067e29f2e 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -289,13 +289,22 @@ static int create_fd(struct fsnotify_group *group, struct path *path,
> */
> static void finish_permission_event(struct fsnotify_group *group,
> struct fanotify_perm_event *event,
> - u32 response)
> + struct fanotify_response *response)
> __releases(&group->notification_lock)
> {
> bool destroy = false;
>
> assert_spin_locked(&group->notification_lock);
> - event->response = response;
> + event->response = response->response & ~FAN_EXTRA;
> + if (response->response & FAN_EXTRA) {
> + event->extra_info_type = response->extra_info_type;
> + switch (event->extra_info_type) {
> + case FAN_RESPONSE_INFO_AUDIT_RULE:
> + event->extra_info.audit_rule = response->extra_info.audit_rule;
> + }
> + } else {
> + event->extra_info_type = FAN_RESPONSE_INFO_NONE;
> + }
> if (event->state == FAN_EVENT_CANCELED)
> destroy = true;
> else
> @@ -306,33 +315,40 @@ 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,
> + size_t count)
> {
> struct fanotify_perm_event *event;
> int fd = response_struct->fd;
> u32 response = response_struct->response;
>
> - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> - fd, response);
> + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> + group, fd, response, response_struct->extra_info_type, count);
> + if (fd < 0)
> + return -EINVAL;
> /*
> * 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) {
> - case FAN_ALLOW:
> - case FAN_DENY:
> - break;
> - default:
> - return -EINVAL;
> - }
> -
> - if (fd < 0)
> + if (FAN_INVALID_RESPONSE_MASK(response))
That is a logic change, because now the response value of 0 becomes valid.
Since you did not document this change in the commit message I assume this was
non intentional?
However, this behavior change is something that I did ask for, but it should be
done is a separate commit:
/* These are NOT bitwise flags. Both bits can be used together. */
#define FAN_TEST 0x00
#define FAN_ALLOW 0x01
#define FAN_DENY 0x02
#define FANOTIFY_RESPONSE_ACCESS \
(FAN_TEST|FAN_ALLOW | FAN_DENY)
...
int access = response & FANOTIFY_RESPONSE_ACCESS;
1. Do return EINVAL for access == 0
2. Let all the rest of the EINVAL checks run (including extra type)
3. Move if (fd < 0) to last check
4. Add if (!access) return 0 before if (fd < 0)
That will provide a mechanism for userspace to probe the
kernel support for extra types in general and specific types
that it may respond with.
> return -EINVAL;
> -
> if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> return -EINVAL;
> -
> + if (response & FAN_EXTRA) {
> + if (count < offsetofend(struct fanotify_response, extra_info_type))
> + return -EINVAL;
> + switch (response_struct->extra_info_type) {
> + case FAN_RESPONSE_INFO_NONE:
> + break;
> + case FAN_RESPONSE_INFO_AUDIT_RULE:
> + if (count < offsetofend(struct fanotify_response, extra_info))
That's a trap right there.
In future kernel, if someone adds a 64bit member to the extra_info union
existing binaries will start failing.
Also since struct fanotify_response is not packed, a 64bit member in the
union will change the alignment of extra_info union.
The use of a union in UAPI seems to be asking for trouble.
You should probably follow the pattern of fanotify_event_info_* structs.
It's more work, but I don't see another way.
> + return -EINVAL;
> + break;
> + default:
> + return -EINVAL;
> + }
> + }
> spin_lock(&group->notification_lock);
> list_for_each_entry(event, &group->fanotify_data.access_list,
> fae.fse.list) {
> @@ -340,7 +356,7 @@ 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_struct);
> wake_up(&group->fanotify_data.access_waitq);
> return 0;
> }
> @@ -802,9 +818,13 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
> fsnotify_destroy_event(group, &event->fse);
> } else {
> if (ret <= 0) {
> + struct fanotify_response response = {
> + .fd = FAN_NOFD,
> + .response = FAN_DENY };
> +
> spin_lock(&group->notification_lock);
> finish_permission_event(group,
> - FANOTIFY_PERM(event), FAN_DENY);
> + FANOTIFY_PERM(event), &response);
> wake_up(&group->fanotify_data.access_waitq);
> } else {
> spin_lock(&group->notification_lock);
> @@ -827,26 +847,25 @@ 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;
> + size_t size = min(count, sizeof(struct fanotify_response));
>
> if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
> return -EINVAL;
>
> group = file->private_data;
>
> - if (count < sizeof(response))
> + if (count < offsetofend(struct fanotify_response, 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, size))
> return -EFAULT;
>
> - ret = process_access_response(group, &response);
> + ret = process_access_response(group, &response, count);
We did not copy count bytes of response. We copied size bytes.
> if (ret < 0)
> count = ret;
>
> @@ -857,6 +876,9 @@ static int fanotify_release(struct inode *ignored, struct file *file)
> {
> struct fsnotify_group *group = file->private_data;
> struct fsnotify_event *fsn_event;
> + struct fanotify_response response = {
> + .fd = FAN_NOFD,
> + .response = FAN_ALLOW };
>
> /*
> * Stop new events from arriving in the notification queue. since
> @@ -876,7 +898,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, &response);
> spin_lock(&group->notification_lock);
> }
>
> @@ -893,7 +915,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);
> + &response);
> }
> spin_lock(&group->notification_lock);
> }
> diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> index 419cadcd7ff5..63a8494e782e 100644
> --- a/include/linux/fanotify.h
> +++ b/include/linux/fanotify.h
> @@ -113,6 +113,9 @@
> #define ALL_FANOTIFY_EVENT_BITS (FANOTIFY_OUTGOING_EVENTS | \
> FANOTIFY_EVENT_FLAGS)
>
> +/* This mask is to check for invalid bits of a user space permission response */
> +#define FAN_INVALID_RESPONSE_MASK(x) ((x) & ~(FAN_ALLOW | FAN_DENY | FAN_AUDIT | FAN_EXTRA))
> +
Please drop this macro and follow the pattern of FANOTIFY_{INIT,MARK,EVENT}_*
#define FANOTIFY_RESPONSE_ACCESS \
(FAN_ALLOW | FAN_DENY)
#define FANOTIFY_RESPONSE_FLAGS \
(FAN_AUDIT | FAN_EXTRA)
#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 e8ac38cc2fd6..a94f4143601f 100644
> --- a/include/uapi/linux/fanotify.h
> +++ b/include/uapi/linux/fanotify.h
> @@ -179,15 +179,35 @@ 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 informaion buffer whose
typo: informaion
> + * size is determined by the extra information type.
> + *
> + * If the context type is Rule, then the context following is the rule number
> + * that triggered the user space decision.
> + */
> +
> +#define FAN_RESPONSE_INFO_NONE 0
> +#define FAN_RESPONSE_INFO_AUDIT_RULE 1
> +
> +union fanotify_response_extra {
> + __u32 audit_rule;
> +};
> +
> struct fanotify_response {
> __s32 fd;
> __u32 response;
> + __u32 extra_info_type;
> + union fanotify_response_extra extra_info;
IIRC, Jan wanted this to be a variable size record with info_type and info_len.
I don't know if we want to make this flexible enough to allow for multiple
records in the future like we do in events, but the common wisdom of
the universe says that if we don't do it, we will need it.
Thanks,
Amir.
On Tue, May 17, 2022 at 2:31 PM Amir Goldstein <[email protected]> wrote:
>
> On Tue, May 17, 2022 at 1:32 PM Jan Kara <[email protected]> wrote:
> >
> > On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> > > On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs <[email protected]> wrote:
> > > >
> > > > This patch adds 2 structure members to the response returned from user
> > > > space on a permission event. The first field is 32 bits for the context
> > > > type. The context type will describe what the meaning is of the second
> > > > field. The default is none. The patch defines one additional context
> > > > type which means that the second field is a union containing a 32-bit
> > > > rule number. This will allow for the creation of other context types in
> > > > the future if other users of the API identify different needs. The
> > > > second field size is defined by the context type and can be used to pass
> > > > along the data described by the context.
> > > >
> > > > To support this, there is a macro for user space to check that the data
> > > > being sent is valid. Of course, without this check, anything that
> > > > overflows the bit field will trigger an EINVAL based on the use of
> > > > FAN_INVALID_RESPONSE_MASK in process_access_response().
> > > >
> > > > 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]>
> >
> > ...
> > > > static int process_access_response(struct fsnotify_group *group,
> > > > - struct fanotify_response *response_struct)
> > > > + struct fanotify_response *response_struct,
> > > > + size_t count)
> > > > {
> > > > struct fanotify_perm_event *event;
> > > > int fd = response_struct->fd;
> > > > u32 response = response_struct->response;
> > > >
> > > > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > > > - fd, response);
> > > > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > > > + group, fd, response, response_struct->extra_info_type, count);
> > > > + if (fd < 0)
> > > > + return -EINVAL;
> > > > /*
> > > > * 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) {
> > > > - case FAN_ALLOW:
> > > > - case FAN_DENY:
> > > > - break;
> > > > - default:
> > > > - return -EINVAL;
> > > > - }
> > > > -
> > > > - if (fd < 0)
> > > > + if (FAN_INVALID_RESPONSE_MASK(response))
> > >
> > > That is a logic change, because now the response value of 0 becomes valid.
> > >
> > > Since you did not document this change in the commit message I assume this was
> > > non intentional?
> > > However, this behavior change is something that I did ask for, but it should be
> > > done is a separate commit:
> > >
> > > /* These are NOT bitwise flags. Both bits can be used together. */
> > > #define FAN_TEST 0x00
> > > #define FAN_ALLOW 0x01
> > > #define FAN_DENY 0x02
> > > #define FANOTIFY_RESPONSE_ACCESS \
> > > (FAN_TEST|FAN_ALLOW | FAN_DENY)
> > >
> > > ...
> > > int access = response & FANOTIFY_RESPONSE_ACCESS;
> > >
> > > 1. Do return EINVAL for access == 0
> > > 2. Let all the rest of the EINVAL checks run (including extra type)
> > > 3. Move if (fd < 0) to last check
> > > 4. Add if (!access) return 0 before if (fd < 0)
> > >
> > > That will provide a mechanism for userspace to probe the
> > > kernel support for extra types in general and specific types
> > > that it may respond with.
> >
> > I have to admit I didn't quite grok your suggestion here although I
> > understand (and agree with) the general direction of the proposal :). Maybe
> > code would explain it better what you have in mind?
> >
>
> +/* These are NOT bitwise flags. Both bits can be used together. */
I realize when reading this that this comment is weird, because
0x01 and 0x02 cannot currently be used together.
The comment was copied from above FAN_MARK_INODE where it
has the same weirdness.
The meaning is that (response & FANOTIFY_RESPONSE_ACCESS)
is an enum. I am sure that a less confusing phrasing for this comment
can be found.
> +#define FAN_TEST 0x00
> #define FAN_ALLOW 0x01
> #define FAN_DENY 0x02
> #define FAN_AUDIT 0x10 /* Bit mask to create audit record for result */
> +#define FANOTIFY_RESPONSE_ACCESS \
> + (FAN_TEST|FAN_ALLOW | FAN_DENY)
Thanks,
Amir.
On 2022-05-17 08:37, Amir Goldstein wrote:
> On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs <[email protected]> wrote:
> >
> > This patch adds 2 structure members to the response returned from user
> > space on a permission event. The first field is 32 bits for the context
> > type. The context type will describe what the meaning is of the second
> > field. The default is none. The patch defines one additional context
> > type which means that the second field is a union containing a 32-bit
> > rule number. This will allow for the creation of other context types in
> > the future if other users of the API identify different needs. The
> > second field size is defined by the context type and can be used to pass
> > along the data described by the context.
> >
> > To support this, there is a macro for user space to check that the data
> > being sent is valid. Of course, without this check, anything that
> > overflows the bit field will trigger an EINVAL based on the use of
> > FAN_INVALID_RESPONSE_MASK in process_access_response().
> >
> > 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 | 2 +-
> > fs/notify/fanotify/fanotify.h | 2 +
> > fs/notify/fanotify/fanotify_user.c | 74 +++++++++++++++++++-----------
> > include/linux/fanotify.h | 3 ++
> > include/uapi/linux/fanotify.h | 22 ++++++++-
> > 5 files changed, 75 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index 985e995d2a39..ea0e60488f12 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 & ~(FAN_AUDIT | FAN_EXTRA)) {
> > case FAN_ALLOW:
> > ret = 0;
> > break;
> > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> > index d66668e06bee..eb7ec1f2a26e 100644
> > --- a/fs/notify/fanotify/fanotify.h
> > +++ b/fs/notify/fanotify/fanotify.h
> > @@ -426,8 +426,10 @@ struct fanotify_perm_event {
> > struct fanotify_event fae;
> > struct path path;
> > u32 response; /* userspace answer to the event */
> > + u32 extra_info_type;
> > unsigned short state; /* state of the event */
> > int fd; /* fd we passed to userspace for this event */
> > + union fanotify_response_extra extra_info;
> > };
> >
> > static inline struct fanotify_perm_event *
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index 721e777ea90b..1c4067e29f2e 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -289,13 +289,22 @@ static int create_fd(struct fsnotify_group *group, struct path *path,
> > */
> > static void finish_permission_event(struct fsnotify_group *group,
> > struct fanotify_perm_event *event,
> > - u32 response)
> > + struct fanotify_response *response)
> > __releases(&group->notification_lock)
> > {
> > bool destroy = false;
> >
> > assert_spin_locked(&group->notification_lock);
> > - event->response = response;
> > + event->response = response->response & ~FAN_EXTRA;
> > + if (response->response & FAN_EXTRA) {
> > + event->extra_info_type = response->extra_info_type;
> > + switch (event->extra_info_type) {
> > + case FAN_RESPONSE_INFO_AUDIT_RULE:
> > + event->extra_info.audit_rule = response->extra_info.audit_rule;
> > + }
> > + } else {
> > + event->extra_info_type = FAN_RESPONSE_INFO_NONE;
> > + }
> > if (event->state == FAN_EVENT_CANCELED)
> > destroy = true;
> > else
> > @@ -306,33 +315,40 @@ 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,
> > + size_t count)
> > {
> > struct fanotify_perm_event *event;
> > int fd = response_struct->fd;
> > u32 response = response_struct->response;
> >
> > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > - fd, response);
> > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > + group, fd, response, response_struct->extra_info_type, count);
> > + if (fd < 0)
> > + return -EINVAL;
> > /*
> > * 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) {
> > - case FAN_ALLOW:
> > - case FAN_DENY:
> > - break;
> > - default:
> > - return -EINVAL;
> > - }
> > -
> > - if (fd < 0)
> > + if (FAN_INVALID_RESPONSE_MASK(response))
>
> That is a logic change, because now the response value of 0 becomes valid.
>
> Since you did not document this change in the commit message I assume this was
> non intentional?
It was not intentional. In hindsight, I should have restored the
original code, or at least looked at the original much more carefully to
duplicate its behaviour.
> However, this behavior change is something that I did ask for, but it should be
> done is a separate commit:
>
> /* These are NOT bitwise flags. Both bits can be used together. */
> #define FAN_TEST 0x00
> #define FAN_ALLOW 0x01
> #define FAN_DENY 0x02
> #define FANOTIFY_RESPONSE_ACCESS \
> (FAN_TEST|FAN_ALLOW | FAN_DENY)
>
> ...
> int access = response & FANOTIFY_RESPONSE_ACCESS;
>
> 1. Do return EINVAL for access == 0
Going back to the original code will do that.
> 2. Let all the rest of the EINVAL checks run (including extra type)
> 3. Move if (fd < 0) to last check
> 4. Add if (!access) return 0 before if (fd < 0)
>
> That will provide a mechanism for userspace to probe the
> kernel support for extra types in general and specific types
> that it may respond with.
I'm still resisting the idea of the TEST flag... It seems like an
unneeded extra step and complication...
The simple presence of the FAN_EXTRA flag should sort it out and could
even make TEST one of the types.
> > return -EINVAL;
> > -
> > if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > return -EINVAL;
> > -
> > + if (response & FAN_EXTRA) {
> > + if (count < offsetofend(struct fanotify_response, extra_info_type))
> > + return -EINVAL;
> > + switch (response_struct->extra_info_type) {
> > + case FAN_RESPONSE_INFO_NONE:
> > + break;
> > + case FAN_RESPONSE_INFO_AUDIT_RULE:
> > + if (count < offsetofend(struct fanotify_response, extra_info))
>
> That's a trap right there.
> In future kernel, if someone adds a 64bit member to the extra_info union
> existing binaries will start failing.
In hindsight, agreed. It should have aimed for the end of "__u32
audit_rule" for FAN_RESPONSE_INFO_AUDIT_RULE.
> Also since struct fanotify_response is not packed, a 64bit member in the
> union will change the alignment of extra_info union.
> The use of a union in UAPI seems to be asking for trouble.
I'll have to take your word for it.
> You should probably follow the pattern of fanotify_event_info_* structs.
> It's more work, but I don't see another way.
I was thinking this would be fine until it was expanded and could be
separated then, but the issue above demonstrates that is false.
> > + return -EINVAL;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + }
> > spin_lock(&group->notification_lock);
> > list_for_each_entry(event, &group->fanotify_data.access_list,
> > fae.fse.list) {
> > @@ -340,7 +356,7 @@ 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_struct);
> > wake_up(&group->fanotify_data.access_waitq);
> > return 0;
> > }
> > @@ -802,9 +818,13 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
> > fsnotify_destroy_event(group, &event->fse);
> > } else {
> > if (ret <= 0) {
> > + struct fanotify_response response = {
> > + .fd = FAN_NOFD,
> > + .response = FAN_DENY };
> > +
> > spin_lock(&group->notification_lock);
> > finish_permission_event(group,
> > - FANOTIFY_PERM(event), FAN_DENY);
> > + FANOTIFY_PERM(event), &response);
> > wake_up(&group->fanotify_data.access_waitq);
> > } else {
> > spin_lock(&group->notification_lock);
> > @@ -827,26 +847,25 @@ 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;
> > + size_t size = min(count, sizeof(struct fanotify_response));
> >
> > if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
> > return -EINVAL;
> >
> > group = file->private_data;
> >
> > - if (count < sizeof(response))
> > + if (count < offsetofend(struct fanotify_response, 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, size))
> > return -EFAULT;
> >
> > - ret = process_access_response(group, &response);
> > + ret = process_access_response(group, &response, count);
>
> We did not copy count bytes of response. We copied size bytes.
This was intentional as a safeguard to not overflow the struct, but also
not take garbage from userspace. If it is an old userspace, the padding
is blank and meaningless. If userspace sends more, it won't trample
beyond the struct. The types involved would take care of that later.
> > if (ret < 0)
> > count = ret;
> >
> > @@ -857,6 +876,9 @@ static int fanotify_release(struct inode *ignored, struct file *file)
> > {
> > struct fsnotify_group *group = file->private_data;
> > struct fsnotify_event *fsn_event;
> > + struct fanotify_response response = {
> > + .fd = FAN_NOFD,
> > + .response = FAN_ALLOW };
> >
> > /*
> > * Stop new events from arriving in the notification queue. since
> > @@ -876,7 +898,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, &response);
> > spin_lock(&group->notification_lock);
> > }
> >
> > @@ -893,7 +915,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);
> > + &response);
> > }
> > spin_lock(&group->notification_lock);
> > }
> > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> > index 419cadcd7ff5..63a8494e782e 100644
> > --- a/include/linux/fanotify.h
> > +++ b/include/linux/fanotify.h
> > @@ -113,6 +113,9 @@
> > #define ALL_FANOTIFY_EVENT_BITS (FANOTIFY_OUTGOING_EVENTS | \
> > FANOTIFY_EVENT_FLAGS)
> >
> > +/* This mask is to check for invalid bits of a user space permission response */
> > +#define FAN_INVALID_RESPONSE_MASK(x) ((x) & ~(FAN_ALLOW | FAN_DENY | FAN_AUDIT | FAN_EXTRA))
> > +
>
> Please drop this macro and follow the pattern of FANOTIFY_{INIT,MARK,EVENT}_*
>
> #define FANOTIFY_RESPONSE_ACCESS \
> (FAN_ALLOW | FAN_DENY)
> #define FANOTIFY_RESPONSE_FLAGS \
> (FAN_AUDIT | FAN_EXTRA)
> #define FANOTIFY_RESPONSE_VALID_MASK \
> (FANOTIFY_RESPONSE_ACCESS | \
> FANOTIFY_RESPONSE_FLAGS)
This seems like a reasonable approach.
> > /* 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 e8ac38cc2fd6..a94f4143601f 100644
> > --- a/include/uapi/linux/fanotify.h
> > +++ b/include/uapi/linux/fanotify.h
> > @@ -179,15 +179,35 @@ 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 informaion buffer whose
>
> typo: informaion
Thanks.
> > + * size is determined by the extra information type.
> > + *
> > + * If the context type is Rule, then the context following is the rule number
> > + * that triggered the user space decision.
> > + */
> > +
> > +#define FAN_RESPONSE_INFO_NONE 0
> > +#define FAN_RESPONSE_INFO_AUDIT_RULE 1
> > +
> > +union fanotify_response_extra {
> > + __u32 audit_rule;
> > +};
> > +
> > struct fanotify_response {
> > __s32 fd;
> > __u32 response;
> > + __u32 extra_info_type;
> > + union fanotify_response_extra extra_info;
>
> IIRC, Jan wanted this to be a variable size record with info_type and info_len.
Again, the intent was to make it fixed for now and change it later if
needed, but that was a shortsighted approach...
I don't see a need for a len in all response types. _NONE doesn't need
any. _AUDIT_RULE is known to be 32 bits. Other types can define their
size and layout as needed, including a len field if it is needed.
> I don't know if we want to make this flexible enough to allow for multiple
> records in the future like we do in events, but the common wisdom of
> the universe says that if we don't do it, we will need it.
It did occur to me that this could be used for other than audit, hence
the renaming of the ..."_NONE" macro.
We should be able in the future to define a type that is extensible or
has multiple records. We have (2^32) - 2 types left to work with.
> Thanks,
> Amir.
- 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
> > However, this behavior change is something that I did ask for, but it should be
> > done is a separate commit:
> >
> > /* These are NOT bitwise flags. Both bits can be used together. */
> > #define FAN_TEST 0x00
> > #define FAN_ALLOW 0x01
> > #define FAN_DENY 0x02
> > #define FANOTIFY_RESPONSE_ACCESS \
> > (FAN_TEST|FAN_ALLOW | FAN_DENY)
> >
> > ...
> > int access = response & FANOTIFY_RESPONSE_ACCESS;
> >
> > 1. Do return EINVAL for access == 0
>
> Going back to the original code will do that.
Oops, this was supposed to be Do NOT return EINVAL for access == 0
this is the case of FAN_TEST.
The patch I posted later explains that better.
>
> > 2. Let all the rest of the EINVAL checks run (including extra type)
> > 3. Move if (fd < 0) to last check
> > 4. Add if (!access) return 0 before if (fd < 0)
> >
> > That will provide a mechanism for userspace to probe the
> > kernel support for extra types in general and specific types
> > that it may respond with.
>
> I'm still resisting the idea of the TEST flag... It seems like an
> unneeded extra step and complication...
Please reply to the patch I posted as a reply as point
at said complication. There is no extra step.
>
> The simple presence of the FAN_EXTRA flag should sort it out and could
> even make TEST one of the types.
>
I think you've missed the point of the TEST response code.
The point of the TEST response code is to test whether the
extra type is supported, so TESTS cannot be a type.
You should not think of FAN_TEST as a flag at all, in
fact, it is semantic and can be omitted altogether.
The core of the idea is that:
int access = response & FANOTIFY_RESPONSE_ACCESS;
access is an enum, not a bitwise mask, much like:
unsigned int class = flags & FANOTIFY_CLASS_BITS;
unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
At the moment, userspace must provide a valid access code
either ALLOW or DENY.
Providing no access code (0) is not valid.
I suggest making FAN_EXTRA with no access code a valid
response for testing the EXTRA types support.
(please refer to the patch)
[...]
> > > + * size is determined by the extra information type.
> > > + *
> > > + * If the context type is Rule, then the context following is the rule number
> > > + * that triggered the user space decision.
> > > + */
> > > +
> > > +#define FAN_RESPONSE_INFO_NONE 0
> > > +#define FAN_RESPONSE_INFO_AUDIT_RULE 1
> > > +
> > > +union fanotify_response_extra {
> > > + __u32 audit_rule;
> > > +};
> > > +
> > > struct fanotify_response {
> > > __s32 fd;
> > > __u32 response;
> > > + __u32 extra_info_type;
> > > + union fanotify_response_extra extra_info;
> >
> > IIRC, Jan wanted this to be a variable size record with info_type and info_len.
>
> Again, the intent was to make it fixed for now and change it later if
> needed, but that was a shortsighted approach...
>
> I don't see a need for a len in all response types. _NONE doesn't need
> any. _AUDIT_RULE is known to be 32 bits. Other types can define their
> size and layout as needed, including a len field if it is needed.
>
len is part of a common response info header.
It is meant to make writing generic code.
So Jan's email.
> > I don't know if we want to make this flexible enough to allow for multiple
> > records in the future like we do in events, but the common wisdom of
> > the universe says that if we don't do it, we will need it.
>
> It did occur to me that this could be used for other than audit, hence
> the renaming of the ..."_NONE" macro.
>
> We should be able in the future to define a type that is extensible or
> has multiple records. We have (2^32) - 2 types left to work with.
>
The way this was done when we first introduced event info
records was the same. We only allowed one type of record
and a single record to begin with, but the format allowed for
extending to multiple records.
struct fanotify_event_metadata already had event_len and
metadata_len, so that was convenient. Supporting multi
records only required that every record has a header with its
own len.
As far as I can tell, the case of fanotify_response is different
because we have the count argument of write(), which serves
as the total response_len.
If we ever want to be able to extend the base fanotify_response,
add fields to it not as extra info records, then we need to add
response_metadata_len to struct fanotify_response, but I think that
would be over design.
Thanks,
Amir.
On Thu 19-05-22 09:03:51, Amir Goldstein wrote:
> > > > + * size is determined by the extra information type.
> > > > + *
> > > > + * If the context type is Rule, then the context following is the rule number
> > > > + * that triggered the user space decision.
> > > > + */
> > > > +
> > > > +#define FAN_RESPONSE_INFO_NONE 0
> > > > +#define FAN_RESPONSE_INFO_AUDIT_RULE 1
> > > > +
> > > > +union fanotify_response_extra {
> > > > + __u32 audit_rule;
> > > > +};
> > > > +
> > > > struct fanotify_response {
> > > > __s32 fd;
> > > > __u32 response;
> > > > + __u32 extra_info_type;
> > > > + union fanotify_response_extra extra_info;
> > >
> > > IIRC, Jan wanted this to be a variable size record with info_type and info_len.
> >
> > Again, the intent was to make it fixed for now and change it later if
> > needed, but that was a shortsighted approach...
> >
> > I don't see a need for a len in all response types. _NONE doesn't need
> > any. _AUDIT_RULE is known to be 32 bits. Other types can define their
> > size and layout as needed, including a len field if it is needed.
> >
>
> len is part of a common response info header.
> It is meant to make writing generic code.
> So Jan's email.
Yes. The reason why I want 'type' + 'len' information for every extra
response type is so that the code can be layered properly. Fanotify has no
bussiness in understanding the details of the additional info (or its
expected length) passed from userspace. That is the knowledge that should
stay within the subsystem this info is for. So the length of info record
needs to be passed in the generic info header.
To give an example imagine a situation when we'd like to attach two
different info records to a response, each for a different subsystem. Then
fanotify has to split response buffer and pass each info to the target
subsystem or maybe we'd just pass all info to both subsystems and define
they should ignore info they don't understand but in either case we need to
have a way to be able to separate different info records without apriori
knowledge what they actually mean or what is their expected length.
> > > I don't know if we want to make this flexible enough to allow for multiple
> > > records in the future like we do in events, but the common wisdom of
> > > the universe says that if we don't do it, we will need it.
> >
> > It did occur to me that this could be used for other than audit, hence
> > the renaming of the ..."_NONE" macro.
> >
> > We should be able in the future to define a type that is extensible or
> > has multiple records. We have (2^32) - 2 types left to work with.
> >
>
> The way this was done when we first introduced event info
> records was the same. We only allowed one type of record
> and a single record to begin with, but the format allowed for
> extending to multiple records.
>
> struct fanotify_event_metadata already had event_len and
> metadata_len, so that was convenient. Supporting multi
> records only required that every record has a header with its
> own len.
>
> As far as I can tell, the case of fanotify_response is different
> because we have the count argument of write(), which serves
> as the total response_len.
Yes.
> If we ever want to be able to extend the base fanotify_response,
> add fields to it not as extra info records, then we need to add
> response_metadata_len to struct fanotify_response, but I think that
> would be over design.
Yeah, I don't think that will happen. The standard response metadata is
basically fixed by backward compatibility constraints. If we need to extend
it in the future, I would prefer the extension to be in a form of an extra
info record.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR