From 43b4935e5358806de18461f3ee92e07c67071eb5 Mon Sep 17 00:00:00 2001 From: Ned Bass 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(). Signed-off-by: Ned Bass Signed-off-by: Brian Behlendorf Signed-off-by: Richard Yao Closes #4150 --- include/sys/sa.h | 5 +++++ module/zfs/sa.c | 6 ++++++ module/zfs/zfs_sa.c | 11 +++++------ module/zfs/zpl_xattr.c | 15 +++++++++++---- 4 files changed, 27 insertions(+), 10 deletions(-) diff --git a/include/sys/sa.h b/include/sys/sa.h index 48e3bcd7cdf3..01d24662a0e0 100644 --- a/include/sys/sa.h +++ b/include/sys/sa.h @@ -82,6 +82,10 @@ typedef struct sa_bulk_attr { uint16_t sa_size; } sa_bulk_attr_t; +/* + * The on-disk format of sa_hdr_phys_t limits SA lengths to 16-bit values. + */ +#define SA_ATTR_MAX_LEN UINT16_MAX /* * special macro for adding entries for bulk attr support @@ -95,6 +99,7 @@ typedef struct sa_bulk_attr { #define SA_ADD_BULK_ATTR(b, idx, attr, func, data, len) \ { \ + ASSERT3U(len, <=, SA_ATTR_MAX_LEN); \ b[idx].sa_attr = attr;\ b[idx].sa_data_func = func; \ b[idx].sa_data = data; \ diff --git a/module/zfs/sa.c b/module/zfs/sa.c index 2383252e2447..d6ac5fcc709a 100644 --- a/module/zfs/sa.c +++ b/module/zfs/sa.c @@ -1464,6 +1464,8 @@ sa_lookup(sa_handle_t *hdl, sa_attr_type_t attr, void *buf, uint32_t buflen) int error; sa_bulk_attr_t bulk; + VERIFY3U(buflen, <=, SA_ATTR_MAX_LEN); + bulk.sa_attr = attr; bulk.sa_data = buf; bulk.sa_length = buflen; @@ -1836,6 +1838,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; @@ -1854,6 +1858,8 @@ sa_update_from_cb(sa_handle_t *hdl, sa_attr_type_t attr, int error; sa_bulk_attr_t bulk; + VERIFY3U(buflen, <=, SA_ATTR_MAX_LEN); + bulk.sa_attr = attr; bulk.sa_data = userdata; bulk.sa_data_func = locator; diff --git a/module/zfs/zfs_sa.c b/module/zfs/zfs_sa.c index c9a9da7528d7..fa1a679e177a 100644 --- a/module/zfs/zfs_sa.c +++ b/module/zfs/zfs_sa.c @@ -229,6 +229,8 @@ zfs_sa_set_xattr(znode_t *zp) ASSERT(zp->z_is_sa); error = nvlist_size(zp->z_xattr_cached, &size, NV_ENCODE_XDR); + if ((error == 0) && (size > SA_ATTR_MAX_LEN)) + error = EFBIG; if (error) goto out; @@ -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..253ea966179a 100644 --- a/module/zfs/zpl_xattr.c +++ b/module/zfs/zpl_xattr.c @@ -473,14 +473,21 @@ zpl_xattr_set_sa(struct inode *ip, const char *name, const void *value, error = -nvlist_add_byte_array(nvl, name, (uchar_t *)value, size); - if (error) - return (error); } - /* Update the SA for additions, modifications, and removals. */ - if (!error) + /* + * Update the SA for additions, modifications, and removals. On + * error drop the inconsistent cached version of the nvlist, it + * will be reconstructed from the ARC when next accessed. + */ + if (error == 0) error = -zfs_sa_set_xattr(zp); + if (error) { + nvlist_free(nvl); + zp->z_xattr_cached = NULL; + } + ASSERT3S(error, <=, 0); return (error);