| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAOQ4uxgwuJ4hv8Dp1v40K5qdnnwa7n9MYyvuh2tkb4gkpZv2Yw@mail.gmail.com>
Date: Fri, 13 Aug 2021 11:27:48 +0300
From: Amir Goldstein <amir73il@...il.com>
To: Gabriel Krisman Bertazi <krisman@...labora.com>
Cc: Jan Kara <jack@...e.com>, Linux API <linux-api@...r.kernel.org>,
Ext4 <linux-ext4@...r.kernel.org>,
linux-fsdevel <linux-fsdevel@...r.kernel.org>,
Khazhismel Kumykov <khazhy@...gle.com>,
David Howells <dhowells@...hat.com>,
Dave Chinner <david@...morbit.com>,
Theodore Tso <tytso@....edu>,
"Darrick J. Wong" <djwong@...nel.org>,
Matthew Bobrowski <repnop@...gle.com>, kernel@...labora.com
Subject: Re: [PATCH v6 12/21] fanotify: Encode invalid file handle when no
inode is provided
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi
<krisman@...labora.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@...il.com>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@...labora.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.
Powered by blists - more mailing lists