linux-ext4 - [PATCH v6 12/21] fanotify: Encode invalid file handle when no inode is provided

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 12 Aug 2021 17:40:01 -0400
From:   Gabriel Krisman Bertazi <krisman@...labora.com>
To:     amir73il@...il.com, jack@...e.com
Cc:     linux-api@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, khazhy@...gle.com,
        dhowells@...hat.com, david@...morbit.com, tytso@....edu,
        djwong@...nel.org, repnop@...gle.com,
        Gabriel Krisman Bertazi <krisman@...labora.com>,
        kernel@...labora.com
Subject: [PATCH v6 12/21] fanotify: Encode invalid file handle when no inode is provided

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;
+
+		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;
 
-- 
2.32.0