From a48b803ccd4e1bb5f84b6502d5ca8fe4697bfaa1 Mon Sep 17 00:00:00 2001
From: Ned Bass <bass6@llnl.gov>
Date: Tue, 29 Dec 2015 18:41:22 -0800
Subject: [PATCH] Prevent SA length overflow

The function sa_update() accepts a 32-bit length parameter and
assigns it to a 16-bit field in sa_bulk_attr_t, potentially
truncating the passed-in value. This could lead to corrupt system
attribute (SA) records getting written to the pool. Add a VERIFY to
sa_update() to detect cases where overflow would occur. The SA length
is limited to 16-bit values by the on-disk format defined by
sa_hdr_phys_t.

The function zfs_sa_set_xattr() is vulnerable to this bug if the
unpacked nvlist of xattrs is less than 64k in size but the packed
size is greater than 64k. Fix this by appropriately checking the
size of the packed nvlist before calling sa_update(). Add error
handling to zpl_xattr_set_sa() to keep the cached list of SA-based
xattrs consistent with the data on disk.

Lastly, zfs_sa_set_xattr() calls dmu_tx_abort() on an assigned
transaction if sa_update() returns an error, but the DMU only allows
unassigned transactions to be aborted. Wrap the sa_update() call in a
VERIFY0, remove the transaction abort, and call dmu_tx_commit()
unconditionally. This is consistent practice with other callers
of sa_update().

Fixes #4150

Signed-off-by: Ned Bass <bass6@llnl.gov>
---
 include/sys/sa.h       |  5 +++++
 module/zfs/sa.c        |  2 ++
 module/zfs/zfs_sa.c    | 11 +++++------
 module/zfs/zpl_xattr.c |  5 ++++-
 4 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/include/sys/sa.h b/include/sys/sa.h
index 48e3bcd7cdf3..25bd31bd6ec5 100644
--- a/include/sys/sa.h
+++ b/include/sys/sa.h
@@ -101,6 +101,11 @@ typedef struct sa_bulk_attr {
 	b[idx++].sa_length = len; \
 }
 
+/*
+ * The on-disk format of sa_hdr_phys_t limits SA lengths to 16-bit values.
+ */
+#define	SA_ATTR_MAX_LEN ((1 << (sizeof (uint16_t) * 8)) - 1)
+
 typedef struct sa_os sa_os_t;
 
 typedef enum sa_handle_type {
diff --git a/module/zfs/sa.c b/module/zfs/sa.c
index 2383252e2447..bb8ad4a927f8 100644
--- a/module/zfs/sa.c
+++ b/module/zfs/sa.c
@@ -1836,6 +1836,8 @@ sa_update(sa_handle_t *hdl, sa_attr_type_t type,
 	int error;
 	sa_bulk_attr_t bulk;
 
+	VERIFY3U(buflen, <=, SA_ATTR_MAX_LEN);
+
 	bulk.sa_attr = type;
 	bulk.sa_data_func = NULL;
 	bulk.sa_length = buflen;
diff --git a/module/zfs/zfs_sa.c b/module/zfs/zfs_sa.c
index c9a9da7528d7..86c565c47407 100644
--- a/module/zfs/zfs_sa.c
+++ b/module/zfs/zfs_sa.c
@@ -231,6 +231,8 @@ zfs_sa_set_xattr(znode_t *zp)
 	error = nvlist_size(zp->z_xattr_cached, &size, NV_ENCODE_XDR);
 	if (error)
 		goto out;
+	if (size > SA_ATTR_MAX_LEN)
+		return (EFBIG);
 
 	obj = zio_buf_alloc(size);
 
@@ -247,12 +249,9 @@ zfs_sa_set_xattr(znode_t *zp)
 	if (error) {
 		dmu_tx_abort(tx);
 	} else {
-		error = sa_update(zp->z_sa_hdl, SA_ZPL_DXATTR(zsb),
-		    obj, size, tx);
-		if (error)
-			dmu_tx_abort(tx);
-		else
-			dmu_tx_commit(tx);
+		VERIFY0(sa_update(zp->z_sa_hdl, SA_ZPL_DXATTR(zsb),
+		    obj, size, tx));
+		dmu_tx_commit(tx);
 	}
 out_free:
 	zio_buf_free(obj, size);
diff --git a/module/zfs/zpl_xattr.c b/module/zfs/zpl_xattr.c
index d9d0673051e4..8859fd02006d 100644
--- a/module/zfs/zpl_xattr.c
+++ b/module/zfs/zpl_xattr.c
@@ -478,8 +478,11 @@ zpl_xattr_set_sa(struct inode *ip, const char *name, const void *value,
 	}
 
 	/* Update the SA for additions, modifications, and removals. */
-	if (!error)
+	if (!error) {
 		error = -zfs_sa_set_xattr(zp);
+		if (error && (value != NULL))
+			(void) nvlist_remove(nvl, name, DATA_TYPE_BYTE_ARRAY);
+	}
 
 	ASSERT3S(error, <=, 0);