Fix xattr update out of bounds access

As we update xattrs we need to update any existing old items with the
contents of the new xattr that uses those items.   The loop that updated
existing items only took the old xattr size into account and assumed
that the new xattr would use those items.   If the new xattr size used
fewer parts then the attempt to update all the old parts that weren't
covered by the new size would go very wrong.   The length of the region
in the new xattr would be negative so it'd try to use the max part
length.  Worse, it'd copy these max part length regions outside the
input new xattr buffer.  Typically this would land in addressible memory
and copy garbage into the unused old items before they were later
deleted.

However, it could access so far outside the input buffer that it could
cross a page boudary into inaccessible memory and fault.  We saw this in
the field while trying to repeatedly incrementally shrink a large xattr.

This fixes the loop that updates overlapping items between the new and
old xattr to start with the smaller of their two item counts.  Now it
will only update items that are actually used by both xattrs and will
only safely access the new xattr input buffer.

Signed-off-by: Zach Brown <zab@versity.com>

kmod/src/xattr.c

@@ -368,7 +368,7 @@ static int change_xattr_items(struct inode *inode, u64 id,
 	}
 
 	/* update dirtied overlapping existing items, last partial first */
-	for (i = old_parts - 1; i >= 0; i--) {
+	for (i = min(old_parts, new_parts) - 1; i >= 0; i--) {
 		off = i * SCOUTFS_XATTR_MAX_PART_SIZE;
 		bytes = min_t(unsigned int, new_bytes - off,
 			      SCOUTFS_XATTR_MAX_PART_SIZE);

tests/golden/simple-xattr-unit

@@ -16,3 +16,4 @@ setfattr: /mnt/test/test/simple-xattr-unit/file: Numerical result out of range
 setfattr: /mnt/test/test/simple-xattr-unit/file: Argument list too long
 === good length boundaries
 === 500 random lengths
+=== alternate val size between interesting sizes

tests/tests/simple-xattr-unit.sh

@@ -46,6 +46,35 @@ print_and_run() {
 	"$@" || echo "returned nonzero status: $?"
 }
 
+# fill a buffer with strings that identify their byte offset
+offs=""
+for o in $(seq 0 7 $((65535 - 7))); do
+	offs+="$(printf "[%5u]" $o)"
+done
+
+change_val_sizes() {
+	local name="$1"
+	local file="$2"
+	local from="$3"
+	local to="$4"
+
+	while : ; do
+		setfattr -x "$name" "$file" > /dev/null 2>&1
+		setfattr -n "$name" -v "${offs:0:$from}" "$file"
+		setfattr -n "$name" -v "${offs:0:$to}" "$file"
+		if ! diff -u <(echo -n "${offs:0:$to}") <(getfattr --absolute-names --only-values -n "$name" $file) ; then
+			echo "setting $name from $from to $to failed"
+		fi
+
+		if [ $from == $3 ]; then
+			from=$4
+			to=$3
+		else
+			break
+		fi
+	done
+}
+
 echo "=== XATTR_ flag combinations"
 touch "$FILE"
 print_and_run dumb_setxattr -p "$FILE" -n user.test -v val -c -r
@@ -80,4 +109,17 @@ for i in $(seq 1 $NR); do
 	test_xattr_lengths $name_len $val_len
 done
 
+echo "=== alternate val size between interesting sizes"
+name="user.test"
+ITEM=896
+HDR=$((8 + 9))
+# one full item apart
+change_val_sizes $name "$FILE" $(((ITEM * 2) - HDR)) $(((ITEM * 3) - HDR))
+# multiple full items apart
+change_val_sizes $name "$FILE" $(((ITEM * 6) - HDR)) $(((ITEM * 9) - HDR))
+# item boundary fence posts
+change_val_sizes $name "$FILE" $(((ITEM * 5) - HDR - 1)) $(((ITEM * 13) - HDR + 1))
+# min and max
+change_val_sizes $name "$FILE" 1 65535
+
 t_pass