[PATCH] ubifs: xattr: fix out-of-bounds write in ubifs_listxattr()

[shao.mingyin]

From: Shao Mingyin <shao.mingyin@xxxxxxxxxx>

ubifs_listxattr() derives the size of the name list from
ui->xattr_names + ui->xattr_cnt and uses it to reject a call whose
buffer is too small.  It then copies every visible xattr name returned
by ubifs_tnc_next_ent() without checking @size again.

Both counters are read from the on-flash inode node and are only
accounting values.  A modified image can make them under-report the
number of xattr entries while the TNC still contains more entries: the
node CRC only protects against accidental corruption, it is recomputed
for a crafted image.

The usual caller first invokes listxattr(path, NULL, 0), which returns
the too small size computed from those counters, then allocates a
buffer of that size and invokes listxattr() again.  In the second
system call fs/xattr.c:listxattr() allocates the kernel buffer with
kvmalloc(size) and passes it to ubifs_listxattr(), which copies the
names past its end.  The copied bytes are the xattr names from the
image, so part of the overflow content is attacker controlled.

Bound each copy by @size and return -ERANGE when the list does not fit.
With a corrupt image the reported size may now be too small, but the
following call fails cleanly instead of overflowing.  Note that nlen
itself is already bounded by ubifs_validate_entry(), which every node
returned by ubifs_tnc_next_ent() has passed; the missing check is only
against the caller's buffer.

While here, validate the two counters individually.  Their sum is
computed in 32 bits, so the existing check is bypassed when it wraps
around; rejecting the oversized values at mount time keeps the O(1)
size query in listxattr() from returning a wrapped (possibly negative)
length.

Mounting UBIFS requires CAP_SYS_ADMIN in the initial user namespace, so
this is only reachable with a crafted image.  Compile tested with W=1
and checkpatch --strict.  Runtime reproduction needs a modified UBI
image: change xattr_cnt/xattr_names in an inode node and recompute its
CRC; a KASAN kernel then reports the overflow on the second
listxattr() call.

Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Shao Mingyin <shao.mingyin@xxxxxxxxxx>
---
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 38972786817e..fe24e1949bb3 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -82,7 +82,15 @@ static int validate_inode(struct ubifs_info *c, const struct inode *inode)
 		return 2;
 	}

-	if (ui->xattr_names + ui->xattr_cnt > XATTR_LIST_MAX)
+	/*
+	 * Both counters are read from the on-flash inode node.  Check them
+	 * individually first: the addition is performed in 32 bits and can
+	 * wrap around, which would make the check below pass for absurd
+	 * values.
+	 */
+	if (ui->xattr_cnt > XATTR_LIST_MAX ||
+	    ui->xattr_names > XATTR_LIST_MAX ||
+	    ui->xattr_names + ui->xattr_cnt > XATTR_LIST_MAX)
 		return 3;

 	if (ui->data_len < 0 || ui->data_len > UBIFS_MAX_INO_DATA)
diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 3b0e8a270f59..8efe2c545bf4 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -421,8 +421,25 @@ ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size)
 		fname_len(&nm) = le16_to_cpu(xent->nlen);

 		if (xattr_visible(xent->name)) {
-			memcpy(buffer + written, fname_name(&nm), fname_len(&nm) + 1);
-			written += fname_len(&nm) + 1;
+			int nlen = fname_len(&nm);
+
+			/*
+			 * nlen is bounded by ubifs_validate_entry(): the
+			 * node length is nlen + UBIFS_DENT_NODE_SZ + 1 and
+			 * is itself bounded by UBIFS_MAX_DENT_NODE_SZ, so
+			 * the name cannot run past the node.
+			 *
+			 * The missing bound is the caller's buffer: the
+			 * on-flash xattr counters are not authoritative and
+			 * a crafted image can under-report them.
+			 */
+			if (written + nlen + 1 > size) {
+				kfree(xent);
+				err = -ERANGE;
+				break;
+			}
+			memcpy(buffer + written, fname_name(&nm), nlen + 1);
+			written += nlen + 1;
 		}

 		kfree(pxent);
@@ -432,6 +449,9 @@ ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size)
 	kfree(pxent);
 	up_read(&host_ui->xattr_sem);

+	if (err == -ERANGE)
+		return err;
+
 	if (err != -ENOENT) {
 		ubifs_err(c, "cannot find next direntry, error %d", err);
 		return err;