| Message ID | 20210812214010.3197279-13-krisman@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | File system wide monitoring | expand |
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi <krisman@collabora.com> wrote: > > Instead of failing, encode an invalid file handle in fanotify_encode_fh > if no inode is provided. This bogus file handle will be reported by > FAN_FS_ERROR for non-inode errors. > > When being reported to userspace, the length information is actually > reset and the handle cleaned up, such that userspace don't have the > visibility of the internal kernel representation of this null handle. > > Also adjust the single caller that might rely on failure after passing > an empty inode. > > Suggested-by: Amir Goldstein <amir73il@gmail.com> > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > > --- > Changes since v5: > - Preserve flags initialization (jan) > - Add BUILD_BUG_ON (amir) > - Require minimum of FANOTIFY_NULL_FH_LEN for fh_len(amir) > - Improve comment to explain the null FH length (jan) > - Simplify logic > --- > fs/notify/fanotify/fanotify.c | 27 ++++++++++++++++++----- > fs/notify/fanotify/fanotify_user.c | 35 +++++++++++++++++------------- > 2 files changed, 41 insertions(+), 21 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 50fce4fec0d6..2b1ab031fbe5 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -334,6 +334,8 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > return test_mask & user_mask; > } > > +#define FANOTIFY_NULL_FH_LEN 4 > + > /* > * Check size needed to encode fanotify_fh. > * > @@ -345,7 +347,7 @@ static int fanotify_encode_fh_len(struct inode *inode) > int dwords = 0; > > if (!inode) > - return 0; > + return FANOTIFY_NULL_FH_LEN; > > exportfs_encode_inode_fh(inode, NULL, &dwords, NULL); > > @@ -367,11 +369,23 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, > void *buf = fh->buf; > int err; > > - fh->type = FILEID_ROOT; > - fh->len = 0; > + BUILD_BUG_ON(FANOTIFY_NULL_FH_LEN < 4 || > + FANOTIFY_NULL_FH_LEN > FANOTIFY_INLINE_FH_LEN); > + > fh->flags = 0; > - if (!inode) > - return 0; > + > + if (!inode) { > + /* > + * Invalid FHs are used on FAN_FS_ERROR for errors not > + * linked to any inode. The f_handle won't be reported > + * back to userspace. The extra bytes are cleared prior > + * to reporting. > + */ > + type = FILEID_INVALID; > + fh_len = FANOTIFY_NULL_FH_LEN; Please memset() the NULL_FH buffer to zero. > + > + goto success; > + } > > /* > * !gpf means preallocated variable size fh, but fh_len could > @@ -400,6 +414,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, > if (!type || type == FILEID_INVALID || fh_len != dwords << 2) > goto out_err; > > +success: > fh->type = type; > fh->len = fh_len; > > @@ -529,7 +544,7 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id, > struct fanotify_info *info; > struct fanotify_fh *dfh, *ffh; > unsigned int dir_fh_len = fanotify_encode_fh_len(id); > - unsigned int child_fh_len = fanotify_encode_fh_len(child); > + unsigned int child_fh_len = child ? fanotify_encode_fh_len(child) : 0; > unsigned int size; > > size = sizeof(*fne) + FANOTIFY_FH_HDR_LEN + dir_fh_len; > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index c47a5a45c0d3..4cacea5fcaca 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -360,7 +360,10 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, > return -EFAULT; > > handle.handle_type = fh->type; > - handle.handle_bytes = fh_len; > + > + /* FILEID_INVALID handle type is reported without its f_handle. */ > + if (fh->type != FILEID_INVALID) > + handle.handle_bytes = fh_len; I know I suggested those exact lines, but looking at the patch, I think it would be better to do: + if (fh->type != FILEID_INVALID) + fh_len = 0; handle.handle_bytes = fh_len; > if (copy_to_user(buf, &handle, sizeof(handle))) > return -EFAULT; > > @@ -369,20 +372,22 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, > if (WARN_ON_ONCE(len < fh_len)) > return -EFAULT; > > - /* > - * For an inline fh and inline file name, copy through stack to exclude > - * the copy from usercopy hardening protections. > - */ > - fh_buf = fanotify_fh_buf(fh); > - if (fh_len <= FANOTIFY_INLINE_FH_LEN) { > - memcpy(bounce, fh_buf, fh_len); > - fh_buf = bounce; > + if (fh->type != FILEID_INVALID) { ... and here: if (fh_len) { > + /* > + * For an inline fh and inline file name, copy through > + * stack to exclude the copy from usercopy hardening > + * protections. > + */ > + fh_buf = fanotify_fh_buf(fh); > + if (fh_len <= FANOTIFY_INLINE_FH_LEN) { > + memcpy(bounce, fh_buf, fh_len); > + fh_buf = bounce; > + } > + if (copy_to_user(buf, fh_buf, fh_len)) > + return -EFAULT; > + buf += fh_len; > + len -= fh_len; > } > - if (copy_to_user(buf, fh_buf, fh_len)) > - return -EFAULT; > - > - buf += fh_len; > - len -= fh_len; > > if (name_len) { > /* Copy the filename with terminating null */ > @@ -398,7 +403,7 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, > } > > /* Pad with 0's */ > - WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN); > + WARN_ON_ONCE(len < 0); According to my calculations, FAN_FS_ERROR event with NULL_FH is expected to get here with len == 4, so you can change this to: WARN_ON_ONCE(len < 0 || len > FANOTIFY_EVENT_ALIGN); But first, I would like to get Jan's feedback on this concept of keeping unneeded 4 bytes zero padding in reported event in case of NULL_FH in order to keep the FID reporting code simpler. Thanks, Amir.
On Fri 13-08-21 11:27:48, Amir Goldstein wrote: > On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi > <krisman@collabora.com> wrote: > > > > Instead of failing, encode an invalid file handle in fanotify_encode_fh > > if no inode is provided. This bogus file handle will be reported by > > FAN_FS_ERROR for non-inode errors. > > > > When being reported to userspace, the length information is actually > > reset and the handle cleaned up, such that userspace don't have the > > visibility of the internal kernel representation of this null handle. > > > > Also adjust the single caller that might rely on failure after passing > > an empty inode. > > > > Suggested-by: Amir Goldstein <amir73il@gmail.com> > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > > > > --- > > Changes since v5: > > - Preserve flags initialization (jan) > > - Add BUILD_BUG_ON (amir) > > - Require minimum of FANOTIFY_NULL_FH_LEN for fh_len(amir) > > - Improve comment to explain the null FH length (jan) > > - Simplify logic > > --- > > fs/notify/fanotify/fanotify.c | 27 ++++++++++++++++++----- > > fs/notify/fanotify/fanotify_user.c | 35 +++++++++++++++++------------- > > 2 files changed, 41 insertions(+), 21 deletions(-) > > > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > > index 50fce4fec0d6..2b1ab031fbe5 100644 > > --- a/fs/notify/fanotify/fanotify.c > > +++ b/fs/notify/fanotify/fanotify.c > > @@ -334,6 +334,8 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > > return test_mask & user_mask; > > } > > > > +#define FANOTIFY_NULL_FH_LEN 4 > > + > > /* > > * Check size needed to encode fanotify_fh. > > * > > @@ -345,7 +347,7 @@ static int fanotify_encode_fh_len(struct inode *inode) > > int dwords = 0; > > > > if (!inode) > > - return 0; > > + return FANOTIFY_NULL_FH_LEN; > > > > exportfs_encode_inode_fh(inode, NULL, &dwords, NULL); > > > > @@ -367,11 +369,23 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, > > void *buf = fh->buf; > > int err; > > > > - fh->type = FILEID_ROOT; > > - fh->len = 0; > > + BUILD_BUG_ON(FANOTIFY_NULL_FH_LEN < 4 || > > + FANOTIFY_NULL_FH_LEN > FANOTIFY_INLINE_FH_LEN); > > + > > fh->flags = 0; > > - if (!inode) > > - return 0; > > + > > + if (!inode) { > > + /* > > + * Invalid FHs are used on FAN_FS_ERROR for errors not > > + * linked to any inode. The f_handle won't be reported > > + * back to userspace. The extra bytes are cleared prior > > + * to reporting. > > + */ > > + type = FILEID_INVALID; > > + fh_len = FANOTIFY_NULL_FH_LEN; > > Please memset() the NULL_FH buffer to zero. > > > + > > + goto success; > > + } > > > > /* > > * !gpf means preallocated variable size fh, but fh_len could > > @@ -400,6 +414,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, > > if (!type || type == FILEID_INVALID || fh_len != dwords << 2) > > goto out_err; > > > > +success: > > fh->type = type; > > fh->len = fh_len; > > > > @@ -529,7 +544,7 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id, > > struct fanotify_info *info; > > struct fanotify_fh *dfh, *ffh; > > unsigned int dir_fh_len = fanotify_encode_fh_len(id); > > - unsigned int child_fh_len = fanotify_encode_fh_len(child); > > + unsigned int child_fh_len = child ? fanotify_encode_fh_len(child) : 0; > > unsigned int size; > > > > size = sizeof(*fne) + FANOTIFY_FH_HDR_LEN + dir_fh_len; > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > index c47a5a45c0d3..4cacea5fcaca 100644 > > --- a/fs/notify/fanotify/fanotify_user.c > > +++ b/fs/notify/fanotify/fanotify_user.c > > @@ -360,7 +360,10 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, > > return -EFAULT; > > > > handle.handle_type = fh->type; > > - handle.handle_bytes = fh_len; > > + > > + /* FILEID_INVALID handle type is reported without its f_handle. */ > > + if (fh->type != FILEID_INVALID) > > + handle.handle_bytes = fh_len; > > I know I suggested those exact lines, but looking at the patch, > I think it would be better to do: > + if (fh->type != FILEID_INVALID) > + fh_len = 0; > handle.handle_bytes = fh_len; > > > if (copy_to_user(buf, &handle, sizeof(handle))) > > return -EFAULT; > > > > @@ -369,20 +372,22 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, > > if (WARN_ON_ONCE(len < fh_len)) > > return -EFAULT; > > > > - /* > > - * For an inline fh and inline file name, copy through stack to exclude > > - * the copy from usercopy hardening protections. > > - */ > > - fh_buf = fanotify_fh_buf(fh); > > - if (fh_len <= FANOTIFY_INLINE_FH_LEN) { > > - memcpy(bounce, fh_buf, fh_len); > > - fh_buf = bounce; > > + if (fh->type != FILEID_INVALID) { > > ... and here: if (fh_len) { > > > + /* > > + * For an inline fh and inline file name, copy through > > + * stack to exclude the copy from usercopy hardening > > + * protections. > > + */ > > + fh_buf = fanotify_fh_buf(fh); > > + if (fh_len <= FANOTIFY_INLINE_FH_LEN) { > > + memcpy(bounce, fh_buf, fh_len); > > + fh_buf = bounce; > > + } > > + if (copy_to_user(buf, fh_buf, fh_len)) > > + return -EFAULT; > > + buf += fh_len; > > + len -= fh_len; > > } > > - if (copy_to_user(buf, fh_buf, fh_len)) > > - return -EFAULT; > > - > > - buf += fh_len; > > - len -= fh_len; > > > > if (name_len) { > > /* Copy the filename with terminating null */ > > @@ -398,7 +403,7 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, > > } > > > > /* Pad with 0's */ > > - WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN); > > + WARN_ON_ONCE(len < 0); > > According to my calculations, FAN_FS_ERROR event with NULL_FH is expected > to get here with len == 4, so you can change this to: > WARN_ON_ONCE(len < 0 || len > FANOTIFY_EVENT_ALIGN); > > But first, I would like to get Jan's feedback on this concept of keeping > unneeded 4 bytes zero padding in reported event in case of NULL_FH > in order to keep the FID reporting code simpler. Dunno, it still seems like quite some complications (simple ones but non-trivial amount of them) for what is rather a corner case. What if we *internally* propagated the information that there's no inode info with FILEID_ROOT fh? That means: No changes to fanotify_encode_fh_len(), fanotify_encode_fh(), or fanotify_alloc_name_event(). In copy_info_to_user() we just mangle FILEID_ROOT to FILEID_INVALID and that's all. No useless padding, no specialcasing of copying etc. Am I missing something? Honza
On Mon, Aug 16, 2021 at 5:07 PM Jan Kara <jack@suse.cz> wrote: > > On Fri 13-08-21 11:27:48, Amir Goldstein wrote: > > On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi > > <krisman@collabora.com> wrote: > > > > > > Instead of failing, encode an invalid file handle in fanotify_encode_fh > > > if no inode is provided. This bogus file handle will be reported by > > > FAN_FS_ERROR for non-inode errors. > > > > > > When being reported to userspace, the length information is actually > > > reset and the handle cleaned up, such that userspace don't have the > > > visibility of the internal kernel representation of this null handle. > > > > > > Also adjust the single caller that might rely on failure after passing > > > an empty inode. > > > > > > Suggested-by: Amir Goldstein <amir73il@gmail.com> > > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > > > > > > --- > > > Changes since v5: > > > - Preserve flags initialization (jan) > > > - Add BUILD_BUG_ON (amir) > > > - Require minimum of FANOTIFY_NULL_FH_LEN for fh_len(amir) > > > - Improve comment to explain the null FH length (jan) > > > - Simplify logic > > > --- > > > fs/notify/fanotify/fanotify.c | 27 ++++++++++++++++++----- > > > fs/notify/fanotify/fanotify_user.c | 35 +++++++++++++++++------------- > > > 2 files changed, 41 insertions(+), 21 deletions(-) > > > > > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > > > index 50fce4fec0d6..2b1ab031fbe5 100644 > > > --- a/fs/notify/fanotify/fanotify.c > > > +++ b/fs/notify/fanotify/fanotify.c > > > @@ -334,6 +334,8 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > > > return test_mask & user_mask; > > > } > > > > > > +#define FANOTIFY_NULL_FH_LEN 4 > > > + > > > /* > > > * Check size needed to encode fanotify_fh. > > > * > > > @@ -345,7 +347,7 @@ static int fanotify_encode_fh_len(struct inode *inode) > > > int dwords = 0; > > > > > > if (!inode) > > > - return 0; > > > + return FANOTIFY_NULL_FH_LEN; > > > > > > exportfs_encode_inode_fh(inode, NULL, &dwords, NULL); > > > > > > @@ -367,11 +369,23 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, > > > void *buf = fh->buf; > > > int err; > > > > > > - fh->type = FILEID_ROOT; > > > - fh->len = 0; > > > + BUILD_BUG_ON(FANOTIFY_NULL_FH_LEN < 4 || > > > + FANOTIFY_NULL_FH_LEN > FANOTIFY_INLINE_FH_LEN); > > > + > > > fh->flags = 0; > > > - if (!inode) > > > - return 0; > > > + > > > + if (!inode) { > > > + /* > > > + * Invalid FHs are used on FAN_FS_ERROR for errors not > > > + * linked to any inode. The f_handle won't be reported > > > + * back to userspace. The extra bytes are cleared prior > > > + * to reporting. > > > + */ > > > + type = FILEID_INVALID; > > > + fh_len = FANOTIFY_NULL_FH_LEN; > > > > Please memset() the NULL_FH buffer to zero. > > > > > + > > > + goto success; > > > + } > > > > > > /* > > > * !gpf means preallocated variable size fh, but fh_len could > > > @@ -400,6 +414,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, > > > if (!type || type == FILEID_INVALID || fh_len != dwords << 2) > > > goto out_err; > > > > > > +success: > > > fh->type = type; > > > fh->len = fh_len; > > > > > > @@ -529,7 +544,7 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id, > > > struct fanotify_info *info; > > > struct fanotify_fh *dfh, *ffh; > > > unsigned int dir_fh_len = fanotify_encode_fh_len(id); > > > - unsigned int child_fh_len = fanotify_encode_fh_len(child); > > > + unsigned int child_fh_len = child ? fanotify_encode_fh_len(child) : 0; > > > unsigned int size; > > > > > > size = sizeof(*fne) + FANOTIFY_FH_HDR_LEN + dir_fh_len; > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > > index c47a5a45c0d3..4cacea5fcaca 100644 > > > --- a/fs/notify/fanotify/fanotify_user.c > > > +++ b/fs/notify/fanotify/fanotify_user.c > > > @@ -360,7 +360,10 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, > > > return -EFAULT; > > > > > > handle.handle_type = fh->type; > > > - handle.handle_bytes = fh_len; > > > + > > > + /* FILEID_INVALID handle type is reported without its f_handle. */ > > > + if (fh->type != FILEID_INVALID) > > > + handle.handle_bytes = fh_len; > > > > I know I suggested those exact lines, but looking at the patch, > > I think it would be better to do: > > + if (fh->type != FILEID_INVALID) > > + fh_len = 0; > > handle.handle_bytes = fh_len; > > > > > if (copy_to_user(buf, &handle, sizeof(handle))) > > > return -EFAULT; > > > > > > @@ -369,20 +372,22 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, > > > if (WARN_ON_ONCE(len < fh_len)) > > > return -EFAULT; > > > > > > - /* > > > - * For an inline fh and inline file name, copy through stack to exclude > > > - * the copy from usercopy hardening protections. > > > - */ > > > - fh_buf = fanotify_fh_buf(fh); > > > - if (fh_len <= FANOTIFY_INLINE_FH_LEN) { > > > - memcpy(bounce, fh_buf, fh_len); > > > - fh_buf = bounce; > > > + if (fh->type != FILEID_INVALID) { > > > > ... and here: if (fh_len) { > > > > > + /* > > > + * For an inline fh and inline file name, copy through > > > + * stack to exclude the copy from usercopy hardening > > > + * protections. > > > + */ > > > + fh_buf = fanotify_fh_buf(fh); > > > + if (fh_len <= FANOTIFY_INLINE_FH_LEN) { > > > + memcpy(bounce, fh_buf, fh_len); > > > + fh_buf = bounce; > > > + } > > > + if (copy_to_user(buf, fh_buf, fh_len)) > > > + return -EFAULT; > > > + buf += fh_len; > > > + len -= fh_len; > > > } > > > - if (copy_to_user(buf, fh_buf, fh_len)) > > > - return -EFAULT; > > > - > > > - buf += fh_len; > > > - len -= fh_len; > > > > > > if (name_len) { > > > /* Copy the filename with terminating null */ > > > @@ -398,7 +403,7 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, > > > } > > > > > > /* Pad with 0's */ > > > - WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN); > > > + WARN_ON_ONCE(len < 0); > > > > According to my calculations, FAN_FS_ERROR event with NULL_FH is expected > > to get here with len == 4, so you can change this to: > > WARN_ON_ONCE(len < 0 || len > FANOTIFY_EVENT_ALIGN); > > > > But first, I would like to get Jan's feedback on this concept of keeping > > unneeded 4 bytes zero padding in reported event in case of NULL_FH > > in order to keep the FID reporting code simpler. > > Dunno, it still seems like quite some complications (simple ones but > non-trivial amount of them) for what is rather a corner case. What if we > *internally* propagated the information that there's no inode info with > FILEID_ROOT fh? That means: No changes to fanotify_encode_fh_len(), > fanotify_encode_fh(), or fanotify_alloc_name_event(). In > copy_info_to_user() we just mangle FILEID_ROOT to FILEID_INVALID and that's > all. No useless padding, no specialcasing of copying etc. Am I missing > something? I am perfectly fine with encoding "no inode" with FILEID_ROOT internally. It's already the value used by fanotify_encode_fh() in upstream. However, if we use zero len internally, we need to pass fh_type to fanotify_fid_info_len() and special case FILEID_ROOT in order to take FANOTIFY_FID_INFO_HDR_LEN into account. And special case fanotify_event_object_fh_len() in fanotify_event_info_len() and in copy_info_records_to_user(). Or maybe I am missing something.... Thanks, Amir.
On Mon 16-08-21 18:54:58, Amir Goldstein wrote: > On Mon, Aug 16, 2021 at 5:07 PM Jan Kara <jack@suse.cz> wrote: > > Dunno, it still seems like quite some complications (simple ones but > > non-trivial amount of them) for what is rather a corner case. What if we > > *internally* propagated the information that there's no inode info with > > FILEID_ROOT fh? That means: No changes to fanotify_encode_fh_len(), > > fanotify_encode_fh(), or fanotify_alloc_name_event(). In > > copy_info_to_user() we just mangle FILEID_ROOT to FILEID_INVALID and that's > > all. No useless padding, no specialcasing of copying etc. Am I missing > > something? > > I am perfectly fine with encoding "no inode" with FILEID_ROOT internally. > It's already the value used by fanotify_encode_fh() in upstream. > > However, if we use zero len internally, we need to pass fh_type to > fanotify_fid_info_len() and special case FILEID_ROOT in order to > take FANOTIFY_FID_INFO_HDR_LEN into account. > > And special case fanotify_event_object_fh_len() in > fanotify_event_info_len() and in copy_info_records_to_user(). Right, this will need some tweaking. I would actually leave fanotify_fid_info_len() alone, just have in fanotify_event_info_len() something like: - if (fh_len) + if (fh_len || fanotify_event_needs_fsid(event)) and similarly in copy_info_records_to_user(): - if (fanotify_event_object_fh_len(event)) { + if (fanotify_event_object_fh_len(event) || + fanotify_event_needs_fsid(event)) { And that should be all that's needed as far as I'm reading the code. Honza
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 50fce4fec0d6..2b1ab031fbe5 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -334,6 +334,8 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, return test_mask & user_mask; } +#define FANOTIFY_NULL_FH_LEN 4 + /* * Check size needed to encode fanotify_fh. * @@ -345,7 +347,7 @@ static int fanotify_encode_fh_len(struct inode *inode) int dwords = 0; if (!inode) - return 0; + return FANOTIFY_NULL_FH_LEN; exportfs_encode_inode_fh(inode, NULL, &dwords, NULL); @@ -367,11 +369,23 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, void *buf = fh->buf; int err; - fh->type = FILEID_ROOT; - fh->len = 0; + BUILD_BUG_ON(FANOTIFY_NULL_FH_LEN < 4 || + FANOTIFY_NULL_FH_LEN > FANOTIFY_INLINE_FH_LEN); + fh->flags = 0; - if (!inode) - return 0; + + if (!inode) { + /* + * Invalid FHs are used on FAN_FS_ERROR for errors not + * linked to any inode. The f_handle won't be reported + * back to userspace. The extra bytes are cleared prior + * to reporting. + */ + type = FILEID_INVALID; + fh_len = FANOTIFY_NULL_FH_LEN; + + goto success; + } /* * !gpf means preallocated variable size fh, but fh_len could @@ -400,6 +414,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, if (!type || type == FILEID_INVALID || fh_len != dwords << 2) goto out_err; +success: fh->type = type; fh->len = fh_len; @@ -529,7 +544,7 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id, struct fanotify_info *info; struct fanotify_fh *dfh, *ffh; unsigned int dir_fh_len = fanotify_encode_fh_len(id); - unsigned int child_fh_len = fanotify_encode_fh_len(child); + unsigned int child_fh_len = child ? fanotify_encode_fh_len(child) : 0; unsigned int size; size = sizeof(*fne) + FANOTIFY_FH_HDR_LEN + dir_fh_len; diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index c47a5a45c0d3..4cacea5fcaca 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -360,7 +360,10 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, return -EFAULT; handle.handle_type = fh->type; - handle.handle_bytes = fh_len; + + /* FILEID_INVALID handle type is reported without its f_handle. */ + if (fh->type != FILEID_INVALID) + handle.handle_bytes = fh_len; if (copy_to_user(buf, &handle, sizeof(handle))) return -EFAULT; @@ -369,20 +372,22 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, if (WARN_ON_ONCE(len < fh_len)) return -EFAULT; - /* - * For an inline fh and inline file name, copy through stack to exclude - * the copy from usercopy hardening protections. - */ - fh_buf = fanotify_fh_buf(fh); - if (fh_len <= FANOTIFY_INLINE_FH_LEN) { - memcpy(bounce, fh_buf, fh_len); - fh_buf = bounce; + if (fh->type != FILEID_INVALID) { + /* + * For an inline fh and inline file name, copy through + * stack to exclude the copy from usercopy hardening + * protections. + */ + fh_buf = fanotify_fh_buf(fh); + if (fh_len <= FANOTIFY_INLINE_FH_LEN) { + memcpy(bounce, fh_buf, fh_len); + fh_buf = bounce; + } + if (copy_to_user(buf, fh_buf, fh_len)) + return -EFAULT; + buf += fh_len; + len -= fh_len; } - if (copy_to_user(buf, fh_buf, fh_len)) - return -EFAULT; - - buf += fh_len; - len -= fh_len; if (name_len) { /* Copy the filename with terminating null */ @@ -398,7 +403,7 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, } /* Pad with 0's */ - WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN); + WARN_ON_ONCE(len < 0); if (len > 0 && clear_user(buf, len)) return -EFAULT;
Instead of failing, encode an invalid file handle in fanotify_encode_fh if no inode is provided. This bogus file handle will be reported by FAN_FS_ERROR for non-inode errors. When being reported to userspace, the length information is actually reset and the handle cleaned up, such that userspace don't have the visibility of the internal kernel representation of this null handle. Also adjust the single caller that might rely on failure after passing an empty inode. Suggested-by: Amir Goldstein <amir73il@gmail.com> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- Changes since v5: - Preserve flags initialization (jan) - Add BUILD_BUG_ON (amir) - Require minimum of FANOTIFY_NULL_FH_LEN for fh_len(amir) - Improve comment to explain the null FH length (jan) - Simplify logic --- fs/notify/fanotify/fanotify.c | 27 ++++++++++++++++++----- fs/notify/fanotify/fanotify_user.c | 35 +++++++++++++++++------------- 2 files changed, 41 insertions(+), 21 deletions(-)