| Message ID | 20231207123825.4011620-5-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Prepare for fsnotify pre-content permission events | expand |
On Thu, Dec 07, 2023 at 02:38:25PM +0200, Amir Goldstein wrote: > In preparation for pre-content permission events with file access range, > move fsnotify_file_perm() hook out of security_file_permission() and into > the callers that have the access range information and pass the access > range to fsnotify_file_perm(). Not pleasant tbh. I really dislike that we have all this extra hook machinery. In some places we have LSM hook, then IMA, then EVM, then fsnotify. Luckily there's a push to move IMA and EVM into the LSM hooks which is at least better and gets rid of some of that stuff. But not too fond that we're moving fsnotify out of the hooks. But since there's no obvious way to consolidate fsnotify and that LSM stuff it's ok as far as I'm concerned.
On Thu 07-12-23 14:38:25, Amir Goldstein wrote: > In preparation for pre-content permission events with file access range, > move fsnotify_file_perm() hook out of security_file_permission() and into > the callers that have the access range information and pass the access > range to fsnotify_file_perm(). > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> So why don't you pass the range into security_file_permission() instead of pulling fsnotify out of the hook? I mean conceptually the accessed range makes sense for the hook as well although no LSM currently cares about it. Also it would address the Christian's concern. > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > index 0a9d6a8a747a..45e6ecbca057 100644 > --- a/include/linux/fsnotify.h > +++ b/include/linux/fsnotify.h > @@ -103,7 +103,8 @@ static inline int fsnotify_file(struct file *file, __u32 mask) > /* > * fsnotify_file_perm - permission hook before file access > */ > -static inline int fsnotify_file_perm(struct file *file, int perm_mask) > +static inline int fsnotify_file_perm(struct file *file, int perm_mask, > + const loff_t *ppos, size_t count) > { > __u32 fsnotify_mask = FS_ACCESS_PERM; Also why do you actually pass in loff_t * instead of plain loff_t? You don't plan to change it, do you? Honza
On Fri, Dec 8, 2023 at 8:53 PM Jan Kara <jack@suse.cz> wrote: > > On Thu 07-12-23 14:38:25, Amir Goldstein wrote: > > In preparation for pre-content permission events with file access range, > > move fsnotify_file_perm() hook out of security_file_permission() and into > > the callers that have the access range information and pass the access > > range to fsnotify_file_perm(). > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > So why don't you pass the range into security_file_permission() instead of > pulling fsnotify out of the hook? I mean conceptually the accessed range > makes sense for the hook as well although no LSM currently cares about it. > Also it would address the Christian's concern. > I don't think it would be nice if the signature of security_file_permission() did not match the signature of the LSM methods of the same name (i.e. call_int_hook(file_permission, 0, file, mask); I think that calling fsnotify_perm(), which is not an LSM, from security_file_permission()/security_file_open() was a hack to begin with and that decoupling fsnotify_perm() hooks from security hooks is a good thing. Also, it is needed for future work. If you look at commit "fs: hold s_write_srcu for pre-modify permission events on write" on my sb_write_barrier branch, you will see that the fsnotify modify permission hook moves (again) into start_file_write_area(). So yes, fsnotify permission hooks become first class vfs hooks and not LSM hooks, but then again, fsnotify_xxx hooks for async events are already first class vfs hooks, and many times in the same vfs_xxx helpers that will have the fsnotify permission hooks in them. > > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > > index 0a9d6a8a747a..45e6ecbca057 100644 > > --- a/include/linux/fsnotify.h > > +++ b/include/linux/fsnotify.h > > @@ -103,7 +103,8 @@ static inline int fsnotify_file(struct file *file, __u32 mask) > > /* > > * fsnotify_file_perm - permission hook before file access > > */ > > -static inline int fsnotify_file_perm(struct file *file, int perm_mask) > > +static inline int fsnotify_file_perm(struct file *file, int perm_mask, > > + const loff_t *ppos, size_t count) > > { > > __u32 fsnotify_mask = FS_ACCESS_PERM; > > Also why do you actually pass in loff_t * instead of plain loff_t? You > don't plan to change it, do you? No I don't. I used NULL to communicate "no range info" to fanotify. It is currently only used from iterate_dir(), but filesystems may need to use that to report other cases of pre-content access with no clear range info. I could leave fsnotify_file_perm(file, mask) for reporting events without range info and add fsnotify_file_area(file, mask, pos, count) for reporting access permission with range info. Do you think that would be better? Thanks, Amir.
> > > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > > > index 0a9d6a8a747a..45e6ecbca057 100644 > > > --- a/include/linux/fsnotify.h > > > +++ b/include/linux/fsnotify.h > > > @@ -103,7 +103,8 @@ static inline int fsnotify_file(struct file *file, __u32 mask) > > > /* > > > * fsnotify_file_perm - permission hook before file access > > > */ > > > -static inline int fsnotify_file_perm(struct file *file, int perm_mask) > > > +static inline int fsnotify_file_perm(struct file *file, int perm_mask, > > > + const loff_t *ppos, size_t count) > > > { > > > __u32 fsnotify_mask = FS_ACCESS_PERM; > > > > Also why do you actually pass in loff_t * instead of plain loff_t? You > > don't plan to change it, do you? > > No I don't. Please note that the pointer is to const loff_t. > > I used NULL to communicate "no range info" to fanotify. > It is currently only used from iterate_dir(), but filesystems may need to > use that to report other cases of pre-content access with no clear range info. Correction. iterate_dir() is not the only case. The callers that use file_ppos(), namely ksys_{read,write}, do_{readv,writev}() will pass a NULL ppos for an FMODE_STREAM file. The only sane behavior I could come up with for those cases is to not report range_info with the FAN_PRE_ACCESS event. > > I could leave fsnotify_file_perm(file, mask) for reporting events without > range info and add fsnotify_file_area(file, mask, pos, count) for reporting > access permission with range info. > I renamed the hook in v2 to fsnotify_file_area_perm() and added a wrapper: static inline int fsnotify_file_perm(struct file *file, int perm_mask) { return fsnotify_file_area_perm(file, perm_mask, NULL, 0); } Thanks, Amir.
On Sun 10-12-23 15:24:00, Amir Goldstein wrote: > > > > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > > > > index 0a9d6a8a747a..45e6ecbca057 100644 > > > > --- a/include/linux/fsnotify.h > > > > +++ b/include/linux/fsnotify.h > > > > @@ -103,7 +103,8 @@ static inline int fsnotify_file(struct file *file, __u32 mask) > > > > /* > > > > * fsnotify_file_perm - permission hook before file access > > > > */ > > > > -static inline int fsnotify_file_perm(struct file *file, int perm_mask) > > > > +static inline int fsnotify_file_perm(struct file *file, int perm_mask, > > > > + const loff_t *ppos, size_t count) > > > > { > > > > __u32 fsnotify_mask = FS_ACCESS_PERM; > > > > > > Also why do you actually pass in loff_t * instead of plain loff_t? You > > > don't plan to change it, do you? > > > > No I don't. > > Please note that the pointer is to const loff_t. > > > > > I used NULL to communicate "no range info" to fanotify. > > It is currently only used from iterate_dir(), but filesystems may need to > > use that to report other cases of pre-content access with no clear range info. > > Correction. iterate_dir() is not the only case. > The callers that use file_ppos(), namely ksys_{read,write}, do_{readv,writev}() > will pass a NULL ppos for an FMODE_STREAM file. > The only sane behavior I could come up with for those cases > is to not report range_info with the FAN_PRE_ACCESS event. OK, understood. But isn't anything with len == 0 in fact "no valid range provided" case? So we could use that to identify a case where we simply don't report any range with the event without a need to pass the pointer? > > I could leave fsnotify_file_perm(file, mask) for reporting events without > > range info and add fsnotify_file_area(file, mask, pos, count) for reporting > > access permission with range info. > > > > I renamed the hook in v2 to fsnotify_file_area_perm() and added a wrapper: > > static inline int fsnotify_file_perm(struct file *file, int perm_mask) > { > return fsnotify_file_area_perm(file, perm_mask, NULL, 0); > } Otherwise this works for me as well. Honza
On Mon, Dec 11, 2023 at 1:49 PM Jan Kara <jack@suse.cz> wrote: > > On Sun 10-12-23 15:24:00, Amir Goldstein wrote: > > > > > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > > > > > index 0a9d6a8a747a..45e6ecbca057 100644 > > > > > --- a/include/linux/fsnotify.h > > > > > +++ b/include/linux/fsnotify.h > > > > > @@ -103,7 +103,8 @@ static inline int fsnotify_file(struct file *file, __u32 mask) > > > > > /* > > > > > * fsnotify_file_perm - permission hook before file access > > > > > */ > > > > > -static inline int fsnotify_file_perm(struct file *file, int perm_mask) > > > > > +static inline int fsnotify_file_perm(struct file *file, int perm_mask, > > > > > + const loff_t *ppos, size_t count) > > > > > { > > > > > __u32 fsnotify_mask = FS_ACCESS_PERM; > > > > > > > > Also why do you actually pass in loff_t * instead of plain loff_t? You > > > > don't plan to change it, do you? > > > > > > No I don't. > > > > Please note that the pointer is to const loff_t. > > > > > > > > I used NULL to communicate "no range info" to fanotify. > > > It is currently only used from iterate_dir(), but filesystems may need to > > > use that to report other cases of pre-content access with no clear range info. > > > > Correction. iterate_dir() is not the only case. > > The callers that use file_ppos(), namely ksys_{read,write}, do_{readv,writev}() > > will pass a NULL ppos for an FMODE_STREAM file. > > The only sane behavior I could come up with for those cases > > is to not report range_info with the FAN_PRE_ACCESS event. > > OK, understood. But isn't anything with len == 0 in fact "no valid range > provided" case? So we could use that to identify a case where we simply > don't report any range with the event without a need to pass the pointer? > IDK. read(2) and pread(2) with count=0 is a valid use case. and we have reported FAN_ACCESS_PERM for those 0 length calls so far. reporting those call with no range would be bad for HSM, because all HSM can do with these events is to fill the entire file content. Filling the entire file content is a valid action if the backing file does not support seek or if it is a directory, but it is not a valid action for an application induced pread() with zero length. Did I misunderstand what you meant? Thanks, Amir.
On Mon 11-12-23 14:00:58, Amir Goldstein wrote: > On Mon, Dec 11, 2023 at 1:49 PM Jan Kara <jack@suse.cz> wrote: > > > > On Sun 10-12-23 15:24:00, Amir Goldstein wrote: > > > > > > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > > > > > > index 0a9d6a8a747a..45e6ecbca057 100644 > > > > > > --- a/include/linux/fsnotify.h > > > > > > +++ b/include/linux/fsnotify.h > > > > > > @@ -103,7 +103,8 @@ static inline int fsnotify_file(struct file *file, __u32 mask) > > > > > > /* > > > > > > * fsnotify_file_perm - permission hook before file access > > > > > > */ > > > > > > -static inline int fsnotify_file_perm(struct file *file, int perm_mask) > > > > > > +static inline int fsnotify_file_perm(struct file *file, int perm_mask, > > > > > > + const loff_t *ppos, size_t count) > > > > > > { > > > > > > __u32 fsnotify_mask = FS_ACCESS_PERM; > > > > > > > > > > Also why do you actually pass in loff_t * instead of plain loff_t? You > > > > > don't plan to change it, do you? > > > > > > > > No I don't. > > > > > > Please note that the pointer is to const loff_t. > > > > > > > > > > > I used NULL to communicate "no range info" to fanotify. > > > > It is currently only used from iterate_dir(), but filesystems may need to > > > > use that to report other cases of pre-content access with no clear range info. > > > > > > Correction. iterate_dir() is not the only case. > > > The callers that use file_ppos(), namely ksys_{read,write}, do_{readv,writev}() > > > will pass a NULL ppos for an FMODE_STREAM file. > > > The only sane behavior I could come up with for those cases > > > is to not report range_info with the FAN_PRE_ACCESS event. > > > > OK, understood. But isn't anything with len == 0 in fact "no valid range > > provided" case? So we could use that to identify a case where we simply > > don't report any range with the event without a need to pass the pointer? > > > > IDK. read(2) and pread(2) with count=0 is a valid use case. > and we have reported FAN_ACCESS_PERM for those 0 length calls so far. > reporting those call with no range would be bad for HSM, because all > HSM can do with these events is to fill the entire file content. Yes, reading or writing 0 bytes is valid but I was wondering whether we are issuing event for that. Now that I've checked we indeed do so let's not complicate this further... > Filling the entire file content is a valid action if the backing file does not > support seek or if it is a directory, but it is not a valid action for > an application > induced pread() with zero length. > > Did I misunderstand what you meant? Yeah, ok, let's leave the calling convention as you have it now. We can always change it when we come up with something more elegant. Honza
diff --git a/fs/open.c b/fs/open.c index 02dc608d40d8..530f70da69e1 100644 --- a/fs/open.c +++ b/fs/open.c @@ -304,6 +304,10 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len) if (ret) return ret; + ret = fsnotify_file_perm(file, MAY_WRITE, &offset, len); + if (ret) + return ret; + if (S_ISFIFO(inode->i_mode)) return -ESPIPE; diff --git a/fs/read_write.c b/fs/read_write.c index 97a9d5c7ad96..1b5b0883edba 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -354,6 +354,9 @@ SYSCALL_DEFINE5(llseek, unsigned int, fd, unsigned long, offset_high, int rw_verify_area(int read_write, struct file *file, const loff_t *ppos, size_t count) { + int mask = read_write == READ ? MAY_READ : MAY_WRITE; + int ret; + if (unlikely((ssize_t) count < 0)) return -EINVAL; @@ -371,8 +374,11 @@ int rw_verify_area(int read_write, struct file *file, const loff_t *ppos, size_t } } - return security_file_permission(file, - read_write == READ ? MAY_READ : MAY_WRITE); + ret = security_file_permission(file, mask); + if (ret) + return ret; + + return fsnotify_file_perm(file, mask, ppos, count); } EXPORT_SYMBOL(rw_verify_area); diff --git a/fs/readdir.c b/fs/readdir.c index c8c46e294431..684ae75d94a4 100644 --- a/fs/readdir.c +++ b/fs/readdir.c @@ -96,6 +96,10 @@ int iterate_dir(struct file *file, struct dir_context *ctx) if (res) goto out; + res = fsnotify_file_perm(file, MAY_READ, NULL, 0); + if (res) + goto out; + res = down_read_killable(&inode->i_rwsem); if (res) goto out; diff --git a/fs/remap_range.c b/fs/remap_range.c index 12131f2a6c9e..ee4729aafbde 100644 --- a/fs/remap_range.c +++ b/fs/remap_range.c @@ -102,7 +102,9 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in, static int remap_verify_area(struct file *file, loff_t pos, loff_t len, bool write) { + int mask = write ? MAY_WRITE : MAY_READ; loff_t tmp; + int ret; if (unlikely(pos < 0 || len < 0)) return -EINVAL; @@ -110,7 +112,11 @@ static int remap_verify_area(struct file *file, loff_t pos, loff_t len, if (unlikely(check_add_overflow(pos, len, &tmp))) return -EINVAL; - return security_file_permission(file, write ? MAY_WRITE : MAY_READ); + ret = security_file_permission(file, mask); + if (ret) + return ret; + + return fsnotify_file_perm(file, mask, &pos, len); } /* diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h index 0a9d6a8a747a..45e6ecbca057 100644 --- a/include/linux/fsnotify.h +++ b/include/linux/fsnotify.h @@ -103,7 +103,8 @@ static inline int fsnotify_file(struct file *file, __u32 mask) /* * fsnotify_file_perm - permission hook before file access */ -static inline int fsnotify_file_perm(struct file *file, int perm_mask) +static inline int fsnotify_file_perm(struct file *file, int perm_mask, + const loff_t *ppos, size_t count) { __u32 fsnotify_mask = FS_ACCESS_PERM; diff --git a/security/security.c b/security/security.c index d7f3703c5905..2a7fc7881cbc 100644 --- a/security/security.c +++ b/security/security.c @@ -2580,13 +2580,7 @@ int security_kernfs_init_security(struct kernfs_node *kn_dir, */ int security_file_permission(struct file *file, int mask) { - int ret; - - ret = call_int_hook(file_permission, 0, file, mask); - if (ret) - return ret; - - return fsnotify_file_perm(file, mask); + return call_int_hook(file_permission, 0, file, mask); } /**
In preparation for pre-content permission events with file access range, move fsnotify_file_perm() hook out of security_file_permission() and into the callers that have the access range information and pass the access range to fsnotify_file_perm(). Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/open.c | 4 ++++ fs/read_write.c | 10 ++++++++-- fs/readdir.c | 4 ++++ fs/remap_range.c | 8 +++++++- include/linux/fsnotify.h | 3 ++- security/security.c | 8 +------- 6 files changed, 26 insertions(+), 11 deletions(-)