linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] fstests: fixes and new tests
@ 2019-02-12  2:17 Darrick J. Wong
  2019-02-12  2:17 ` [PATCH 1/3] common: fix kmemleak to work with sections Darrick J. Wong
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Darrick J. Wong @ 2019-02-12  2:17 UTC (permalink / raw)
  To: guaneryu, darrick.wong; +Cc: linux-xfs, fstests

Hi all,

The first patch fixes kmemleak to deal with sections correctly.

Patches 2-3 check that statx btime (aka inode creation time) report
reasonable results for filesystems that support it.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=djwong-devel

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=djwong-devel

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=djwong-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] common: fix kmemleak to work with sections
  2019-02-12  2:17 [PATCH 0/3] fstests: fixes and new tests Darrick J. Wong
@ 2019-02-12  2:17 ` Darrick J. Wong
  2019-02-12  2:17 ` [PATCH 2/3] common: fix _require_btime for lazy filesystems Darrick J. Wong
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2019-02-12  2:17 UTC (permalink / raw)
  To: guaneryu, darrick.wong; +Cc: linux-xfs, fstests

From: Darrick J. Wong <darrick.wong@oracle.com>

Refactor the kmemleak code to work correctly with sections.  This
requires changing the location of the "is kmemleak enabled?" flag to use
/tmp instead of RESULT_BASE, scanning for leaks after every test, and
clarifying which functions get used when.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 README    |    2 ++
 check     |    7 +++++--
 common/rc |   32 ++++++++++++++++++++++----------
 3 files changed, 29 insertions(+), 12 deletions(-)


diff --git a/README b/README
index 73f057f3..6e9aa34d 100644
--- a/README
+++ b/README
@@ -99,6 +99,8 @@ Preparing system for tests:
                that relevant results are compared.  For example 'spinningrust'
                for configurations that use spinning disks and 'nvme' for tests
                using nvme drives.
+             - set USE_KMEMLEAK=yes to scan for memory leaks in the kernel
+               after every test, if the kernel supports kmemleak.
 
         - or add a case to the switch in common/config assigning
           these variables based on the hostname of your test
diff --git a/check b/check
index 77a06b00..20e95302 100755
--- a/check
+++ b/check
@@ -496,7 +496,7 @@ _expunge_test()
 	return 0
 }
 
-_init_kmemleak
+_detect_kmemleak
 _prepare_test_list
 
 if $OPTIONS_HAVE_SECTIONS; then
@@ -771,9 +771,12 @@ for section in $HOST_OPTIONS_SECTIONS; do
 			# and log messages that shouldn't be there.
 			_check_filesystems
 			_check_dmesg || err=true
-			_check_kmemleak || err=true
 		fi
 
+		# Scan for memory leaks after every test so that associating
+		# a leak to a particular test will be as accurate as possible.
+		_check_kmemleak || err=true
+
 		# test ends after all checks are done.
 		$timestamp && _timestamp
 		stop=`_wallclock`
diff --git a/common/rc b/common/rc
index b8ed1776..ab468adf 100644
--- a/common/rc
+++ b/common/rc
@@ -3500,7 +3500,7 @@ _check_dmesg()
 # capture the kmemleak report
 _capture_kmemleak()
 {
-	local kern_knob="${DEBUGFS_MNT}/kmemleak"
+	local kern_knob="$DEBUGFS_MNT/kmemleak"
 	local leak_file="$1"
 
 	# Tell the kernel to scan for memory leaks.  Apparently the write
@@ -3521,17 +3521,20 @@ ENDL
 	echo "clear" > "$kern_knob"
 }
 
-# set up kmemleak
-_init_kmemleak()
+# Figure out if the running kernel supports kmemleak; if it does, clear out
+# anything that leaked before we even started testing.  The leak checker only
+# needs to be primed like this once per ./check invocation.
+_detect_kmemleak()
 {
-	local kern_knob="${DEBUGFS_MNT}/kmemleak"
+	local kern_knob="$DEBUGFS_MNT/kmemleak"
+	KMEMLEAK_CHECK_FILE="/tmp/check_kmemleak"
 
 	# Since kernel v4.19-rc3, the kmemleak knob exists even if kmemleak is
 	# disabled, but returns EBUSY on write. So instead of relying on
 	# existance of writable knob file, we use a test file to indicate that
 	# _check_kmemleak() is enabled only if we actually managed to write to
 	# the knob file.
-	rm -f ${RESULT_BASE}/check_kmemleak
+	rm -f "$KMEMLEAK_CHECK_FILE"
 
 	if [ ! -w "$kern_knob" ]; then
 		return 0
@@ -3541,17 +3544,26 @@ _init_kmemleak()
 	# then dump all the leaks recorded so far.
 	if echo "scan=off" > "$kern_knob" 2>/dev/null; then
 		_capture_kmemleak /dev/null
-		touch ${RESULT_BASE}/check_kmemleak
+		touch "$KMEMLEAK_CHECK_FILE"
 	fi
 }
 
-# check kmemleak log
+# Kick the kmemleak checker to scan for leaks.  Background leak scan mode is
+# not enabled, so we must call the kernel to ask for a scan and deal with the
+# results appropriately.  This we do after every test completes, whether or not
+# it was successful.
 _check_kmemleak()
 {
-	local kern_knob="${DEBUGFS_MNT}/kmemleak"
-	local leak_file="${seqres}.kmemleak"
+	local kern_knob="$DEBUGFS_MNT/kmemleak"
+	local leak_file="$seqres.kmemleak"
 
-	if [ ! -f ${RESULT_BASE}/check_kmemleak ]; then
+	if [ ! -f "$KMEMLEAK_CHECK_FILE" ]; then
+		return 0
+	fi
+
+	# Not enabled, so discard any report of leaks found.
+	if [ "$USE_KMEMLEAK" != "yes" ]; then
+		_capture_kmemleak /dev/null
 		return 0
 	fi
 

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/3] common: fix _require_btime for lazy filesystems
  2019-02-12  2:17 [PATCH 0/3] fstests: fixes and new tests Darrick J. Wong
  2019-02-12  2:17 ` [PATCH 1/3] common: fix kmemleak to work with sections Darrick J. Wong
@ 2019-02-12  2:17 ` Darrick J. Wong
  2019-02-12  2:17 ` [PATCH 3/3] generic: check for reasonable inode creation time Darrick J. Wong
  2019-02-13 20:48 ` [PATCH 4/3] generic: posix acl extended attribute memory corruption test Darrick J. Wong
  3 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2019-02-12  2:17 UTC (permalink / raw)
  To: guaneryu, darrick.wong; +Cc: linux-xfs, fstests

From: Darrick J. Wong <darrick.wong@oracle.com>

Filesystems are not required to try to fill the statx btime field unless
the caller actually sets STATX_BTIME.  They're allowed to volunteer that
information "if it's cheap", but XFS doesn't volunteer and there may be
filesystems that support btime but not cheaply.

Either way, we want to test btime on any filesystem that supports it,
cheaply or otherwise, so set STATX_BTIME when we're trying to detect
support for it.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 common/rc |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)


diff --git a/common/rc b/common/rc
index ab468adf..6fb71e60 100644
--- a/common/rc
+++ b/common/rc
@@ -3849,7 +3849,11 @@ _dmsetup_create()
 
 _require_btime()
 {
-	$XFS_IO_PROG -f $TEST_DIR/test_creation_time -c "statx -v" \
+	# Note: filesystems are not required to report btime (creation time)
+	# if the caller doesn't ask for it, so we define STATX_BTIME here and
+	# pass it in to the statx command.
+	export STATX_BTIME=0x800
+	$XFS_IO_PROG -f $TEST_DIR/test_creation_time -c "statx -m $STATX_BTIME -v" \
 		| grep btime >>$seqres.full 2>&1 || \
 		_notrun "inode creation time not supported by this filesystem"
 	rm -f $TEST_DIR/test_creation_time

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/3] generic: check for reasonable inode creation time
  2019-02-12  2:17 [PATCH 0/3] fstests: fixes and new tests Darrick J. Wong
  2019-02-12  2:17 ` [PATCH 1/3] common: fix kmemleak to work with sections Darrick J. Wong
  2019-02-12  2:17 ` [PATCH 2/3] common: fix _require_btime for lazy filesystems Darrick J. Wong
@ 2019-02-12  2:17 ` Darrick J. Wong
  2019-02-13 20:48 ` [PATCH 4/3] generic: posix acl extended attribute memory corruption test Darrick J. Wong
  3 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2019-02-12  2:17 UTC (permalink / raw)
  To: guaneryu, darrick.wong; +Cc: linux-xfs, fstests

From: Darrick J. Wong <darrick.wong@oracle.com>

If statx returns inode creation time (aka btime), check it to make sure
that the filesystem is setting a creation time that's reasonably close
to when it creates a file.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tests/generic/709     |   53 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/709.out |    2 ++
 tests/generic/group   |    1 +
 3 files changed, 56 insertions(+)
 create mode 100755 tests/generic/709
 create mode 100644 tests/generic/709.out


diff --git a/tests/generic/709 b/tests/generic/709
new file mode 100755
index 00000000..ebec10de
--- /dev/null
+++ b/tests/generic/709
@@ -0,0 +1,53 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2019 Oracle, Inc.  All Rights Reserved.
+#
+# FS QA Test No. 709
+#
+# Check that statx btime (aka creation time) is plausibly close to when
+# we created a file.  A bug caught during code review of xfs patches revealed
+# that there weren't any sanity checks of the btime values.
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+tmp=/tmp/$$
+status=1	# failure is the default!
+testfile=$TEST_DIR/$seq.txt
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.* $testfile
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/attr
+. ./common/filter
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_test
+_require_xfs_io_command "statx" "-r"
+_require_btime
+
+rm -f $seqres.full
+rm -f $testfile
+
+# Create a file and the time we created it
+now=$(date +%s)
+touch $testfile
+
+# Make sure the reported btime is within 5 seconds of the time we recorded
+# just prior to creating the file.
+btime=$(date +%s -d "$($XFS_IO_PROG -c "statx -v -m $STATX_BTIME" $testfile | \
+	grep 'stat.btime =' | cut -d '=' -f 2)")
+test -n "$btime" || echo "error: did not see btime in output??"
+
+_within_tolerance "btime" "$btime" "$now" 0 5 -v
+
+status=0
+exit
diff --git a/tests/generic/709.out b/tests/generic/709.out
new file mode 100644
index 00000000..d8495ace
--- /dev/null
+++ b/tests/generic/709.out
@@ -0,0 +1,2 @@
+QA output created by 709
+btime is in range
diff --git a/tests/generic/group b/tests/generic/group
index cfd003d3..f56eb475 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -528,3 +528,4 @@
 523 auto quick attr
 524 auto quick
 525 auto quick rw
+709 auto quick

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/3] generic: posix acl extended attribute memory corruption test
  2019-02-12  2:17 [PATCH 0/3] fstests: fixes and new tests Darrick J. Wong
                   ` (2 preceding siblings ...)
  2019-02-12  2:17 ` [PATCH 3/3] generic: check for reasonable inode creation time Darrick J. Wong
@ 2019-02-13 20:48 ` Darrick J. Wong
  2019-02-16 12:05   ` Eryu Guan
                     ` (2 more replies)
  3 siblings, 3 replies; 12+ messages in thread
From: Darrick J. Wong @ 2019-02-13 20:48 UTC (permalink / raw)
  To: guaneryu; +Cc: linux-xfs, fstests

From: Darrick J. Wong <darrick.wong@oracle.com>

XFS had a use-after-free bug when xfs_xattr_put_listent runs out of
listxattr buffer space while trying to store the name
"system.posix_acl_access" and then corrupts memory by not checking the
seen_enough state and then trying to shove "trusted.SGI_ACL_FILE" into
the buffer as well.

In order to tickle the bug in a user visible way we must have already
put a name in the buffer, so we take advantage of the fact that
"security.evm" sorts before "system.posix_acl_access" to make sure this
happens.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 .gitignore              |    1 
 src/Makefile            |    2 -
 src/t_attr_corruption.c |  122 +++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/712       |   41 ++++++++++++++++
 tests/generic/712.out   |    2 +
 tests/generic/group     |    1 
 6 files changed, 168 insertions(+), 1 deletion(-)
 create mode 100644 src/t_attr_corruption.c
 create mode 100755 tests/generic/712
 create mode 100644 tests/generic/712.out

diff --git a/.gitignore b/.gitignore
index ea1aac8a..0933dc7d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -114,6 +114,7 @@
 /src/stat_test
 /src/swapon
 /src/t_access_root
+/src/t_attr_corruption
 /src/t_dir_offset
 /src/t_dir_offset2
 /src/t_dir_type
diff --git a/src/Makefile b/src/Makefile
index 41826585..ae09eb0a 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -27,7 +27,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
 	renameat2 t_getcwd e4compact test-nextquota punch-alternating \
 	attr-list-by-handle-cursor-test listxattr dio-interleaved t_dir_type \
 	dio-invalidate-cache stat_test t_encrypted_d_revalidate \
-	attr_replace_test swapon mkswap
+	attr_replace_test swapon mkswap t_attr_corruption
 
 SUBDIRS = log-writes perf
 
diff --git a/src/t_attr_corruption.c b/src/t_attr_corruption.c
new file mode 100644
index 00000000..1fa5e41f
--- /dev/null
+++ b/src/t_attr_corruption.c
@@ -0,0 +1,122 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Oracle.  All Rights Reserved.
+ * Author: Darrick J. Wong <darrick.wong@oracle.com>
+ *
+ * Test program to tickle a use-after-free bug in xfs.
+ *
+ * XFS had a use-after-free bug when xfs_xattr_put_listent runs out of
+ * listxattr buffer space while trying to store the name
+ * "system.posix_acl_access" and then corrupts memory by not checking the
+ * seen_enough state and then trying to shove "trusted.SGI_ACL_FILE" into the
+ * buffer as well.
+ *
+ * In order to tickle the bug in a user visible way we must have already put a
+ * name in the buffer, so we take advantage of the fact that "security.evm"
+ * sorts before "system.posix_acl_access" to make sure this happens.
+ *
+ * If we trigger the bug, the program will print the garbled string
+ * "rusted.SGI_ACL_FILE".  If the bug is fixed, the flistxattr call returns
+ * ERANGE.
+ */
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+#include <unistd.h>
+#include <attr/xattr.h>
+
+void die(const char *msg)
+{
+	perror(msg);
+	exit(1);
+}
+
+struct entry {
+	uint16_t	a;
+	uint16_t	b;
+	uint32_t	c;
+};
+
+struct myacl {
+	uint32_t	d;
+	struct entry	e[4];
+};
+
+int main(int argc, char *argv[])
+{
+	struct myacl acl = {
+		.d = 2,
+		.e = {
+			{1, 0, 0},
+			{4, 0, 0},
+			{0x10, 0, 0},
+			{0x20, 0, 0},
+		},
+	};
+	char buf[64];
+	ssize_t sz;
+	int fd;
+	int ret;
+
+	if (argc > 1) {
+		ret = chdir(argv[1]);
+		if (ret)
+			die(argv[1]);
+	}
+
+	fd = creat("file0", 0644);
+	if (fd < 0)
+		die("create");
+
+	ret = fsetxattr(fd, "system.posix_acl_access", &acl, sizeof(acl), 0);
+	if (ret)
+		die("set posix acl");
+
+	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
+	if (ret)
+		die("set evm");
+
+	sz = flistxattr(fd, buf, 30);
+	if (sz < 0)
+		die("list attr");
+
+	printf("%s\n", buf);
+
+	return 0;
+
+#if 0
+	/* original syzkaller reproducer */
+
+	syscall(__NR_mmap, 0x20000000, 0x1000000, 3, 0x32, -1, 0);
+
+	memcpy((void*)0x20000180, "./file0", 8);
+	syscall(__NR_creat, 0x20000180, 0);
+	memcpy((void*)0x20000000, "./file0", 8);
+	memcpy((void*)0x20000040, "system.posix_acl_access", 24);
+	*(uint32_t*)0x20000680 = 2;
+	*(uint16_t*)0x20000684 = 1;
+	*(uint16_t*)0x20000686 = 0;
+	*(uint32_t*)0x20000688 = 0;
+	*(uint16_t*)0x2000068c = 4;
+	*(uint16_t*)0x2000068e = 0;
+	*(uint32_t*)0x20000690 = 0;
+	*(uint16_t*)0x20000694 = 0x10;
+	*(uint16_t*)0x20000696 = 0;
+	*(uint32_t*)0x20000698 = 0;
+	*(uint16_t*)0x2000069c = 0x20;
+	*(uint16_t*)0x2000069e = 0;
+	*(uint32_t*)0x200006a0 = 0;
+	syscall(__NR_setxattr, 0x20000000, 0x20000040, 0x20000680, 0x24, 0);
+	memcpy((void*)0x20000080, "./file0", 8);
+	memcpy((void*)0x200000c0, "security.evm", 13);
+	memcpy((void*)0x20000100, "\x03\x00\x00\x00\x57", 5);
+	syscall(__NR_lsetxattr, 0x20000080, 0x200000c0, 0x20000100, 1, 1);
+	memcpy((void*)0x20000300, "./file0", 8);
+	syscall(__NR_listxattr, 0x20000300, 0x200002c0, 0x1e);
+	return 0;
+#endif
+}
diff --git a/tests/generic/712 b/tests/generic/712
new file mode 100755
index 00000000..6348a797
--- /dev/null
+++ b/tests/generic/712
@@ -0,0 +1,41 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2019 Oracle, Inc.  All Rights Reserved.
+#
+# FS QA Test No. 712
+#
+# Regression test for a bug where XFS corrupts memory if the listxattr buffer
+# is a particularly well crafted size on a filesystem that supports posix acls.
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+tmp=/tmp/$$
+status=1	# failure is the default!
+testfile=$TEST_DIR/$seq.txt
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/attr
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_acls
+_require_scratch
+
+rm -f $seqres.full
+_scratch_mkfs >> $seqres.full 2>&1
+_scratch_mount
+
+src/t_attr_corruption $SCRATCH_MNT
+
+status=0
+exit
diff --git a/tests/generic/712.out b/tests/generic/712.out
new file mode 100644
index 00000000..a2ba09f3
--- /dev/null
+++ b/tests/generic/712.out
@@ -0,0 +1,2 @@
+QA output created by 712
+list attr: Numerical result out of range
diff --git a/tests/generic/group b/tests/generic/group
index f56eb475..b3086154 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -529,3 +529,4 @@
 524 auto quick
 525 auto quick rw
 709 auto quick
+712 auto quick attr

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/3] generic: posix acl extended attribute memory corruption test
  2019-02-13 20:48 ` [PATCH 4/3] generic: posix acl extended attribute memory corruption test Darrick J. Wong
@ 2019-02-16 12:05   ` Eryu Guan
  2019-02-16 17:24     ` Darrick J. Wong
  2019-02-20 13:29   ` David Sterba
  2019-02-25 18:57   ` Jeff Mahoney
  2 siblings, 1 reply; 12+ messages in thread
From: Eryu Guan @ 2019-02-16 12:05 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, fstests

On Wed, Feb 13, 2019 at 12:48:14PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> XFS had a use-after-free bug when xfs_xattr_put_listent runs out of
> listxattr buffer space while trying to store the name
> "system.posix_acl_access" and then corrupts memory by not checking the
> seen_enough state and then trying to shove "trusted.SGI_ACL_FILE" into
> the buffer as well.
> 
> In order to tickle the bug in a user visible way we must have already
> put a name in the buffer, so we take advantage of the fact that
> "security.evm" sorts before "system.posix_acl_access" to make sure this
> happens.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  .gitignore              |    1 
>  src/Makefile            |    2 -
>  src/t_attr_corruption.c |  122 +++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/712       |   41 ++++++++++++++++
>  tests/generic/712.out   |    2 +
>  tests/generic/group     |    1 
>  6 files changed, 168 insertions(+), 1 deletion(-)
>  create mode 100644 src/t_attr_corruption.c
>  create mode 100755 tests/generic/712
>  create mode 100644 tests/generic/712.out
> 
> diff --git a/.gitignore b/.gitignore
> index ea1aac8a..0933dc7d 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -114,6 +114,7 @@
>  /src/stat_test
>  /src/swapon
>  /src/t_access_root
> +/src/t_attr_corruption
>  /src/t_dir_offset
>  /src/t_dir_offset2
>  /src/t_dir_type
> diff --git a/src/Makefile b/src/Makefile
> index 41826585..ae09eb0a 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -27,7 +27,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
>  	renameat2 t_getcwd e4compact test-nextquota punch-alternating \
>  	attr-list-by-handle-cursor-test listxattr dio-interleaved t_dir_type \
>  	dio-invalidate-cache stat_test t_encrypted_d_revalidate \
> -	attr_replace_test swapon mkswap
> +	attr_replace_test swapon mkswap t_attr_corruption
>  
>  SUBDIRS = log-writes perf
>  
> diff --git a/src/t_attr_corruption.c b/src/t_attr_corruption.c
> new file mode 100644
> index 00000000..1fa5e41f
> --- /dev/null
> +++ b/src/t_attr_corruption.c
> @@ -0,0 +1,122 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Oracle.  All Rights Reserved.
> + * Author: Darrick J. Wong <darrick.wong@oracle.com>
> + *
> + * Test program to tickle a use-after-free bug in xfs.
> + *
> + * XFS had a use-after-free bug when xfs_xattr_put_listent runs out of
> + * listxattr buffer space while trying to store the name
> + * "system.posix_acl_access" and then corrupts memory by not checking the
> + * seen_enough state and then trying to shove "trusted.SGI_ACL_FILE" into the
> + * buffer as well.
> + *
> + * In order to tickle the bug in a user visible way we must have already put a
> + * name in the buffer, so we take advantage of the fact that "security.evm"
> + * sorts before "system.posix_acl_access" to make sure this happens.
> + *
> + * If we trigger the bug, the program will print the garbled string
> + * "rusted.SGI_ACL_FILE".  If the bug is fixed, the flistxattr call returns
> + * ERANGE.
> + */
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdint.h>
> +#include <unistd.h>
> +#include <attr/xattr.h>
> +
> +void die(const char *msg)
> +{
> +	perror(msg);
> +	exit(1);
> +}
> +
> +struct entry {
> +	uint16_t	a;
> +	uint16_t	b;
> +	uint32_t	c;
> +};
> +
> +struct myacl {
> +	uint32_t	d;
> +	struct entry	e[4];
> +};
> +
> +int main(int argc, char *argv[])
> +{
> +	struct myacl acl = {
> +		.d = 2,
> +		.e = {
> +			{1, 0, 0},
> +			{4, 0, 0},
> +			{0x10, 0, 0},
> +			{0x20, 0, 0},
> +		},
> +	};
> +	char buf[64];
> +	ssize_t sz;
> +	int fd;
> +	int ret;
> +
> +	if (argc > 1) {
> +		ret = chdir(argv[1]);
> +		if (ret)
> +			die(argv[1]);
> +	}
> +
> +	fd = creat("file0", 0644);
> +	if (fd < 0)
> +		die("create");
> +
> +	ret = fsetxattr(fd, "system.posix_acl_access", &acl, sizeof(acl), 0);
> +	if (ret)
> +		die("set posix acl");
> +
> +	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
> +	if (ret)
> +		die("set evm");
> +
> +	sz = flistxattr(fd, buf, 30);
> +	if (sz < 0)
> +		die("list attr");
> +
> +	printf("%s\n", buf);
> +
> +	return 0;
> +
> +#if 0
> +	/* original syzkaller reproducer */
> +
> +	syscall(__NR_mmap, 0x20000000, 0x1000000, 3, 0x32, -1, 0);
> +
> +	memcpy((void*)0x20000180, "./file0", 8);
> +	syscall(__NR_creat, 0x20000180, 0);
> +	memcpy((void*)0x20000000, "./file0", 8);
> +	memcpy((void*)0x20000040, "system.posix_acl_access", 24);
> +	*(uint32_t*)0x20000680 = 2;
> +	*(uint16_t*)0x20000684 = 1;
> +	*(uint16_t*)0x20000686 = 0;
> +	*(uint32_t*)0x20000688 = 0;
> +	*(uint16_t*)0x2000068c = 4;
> +	*(uint16_t*)0x2000068e = 0;
> +	*(uint32_t*)0x20000690 = 0;
> +	*(uint16_t*)0x20000694 = 0x10;
> +	*(uint16_t*)0x20000696 = 0;
> +	*(uint32_t*)0x20000698 = 0;
> +	*(uint16_t*)0x2000069c = 0x20;
> +	*(uint16_t*)0x2000069e = 0;
> +	*(uint32_t*)0x200006a0 = 0;
> +	syscall(__NR_setxattr, 0x20000000, 0x20000040, 0x20000680, 0x24, 0);
> +	memcpy((void*)0x20000080, "./file0", 8);
> +	memcpy((void*)0x200000c0, "security.evm", 13);
> +	memcpy((void*)0x20000100, "\x03\x00\x00\x00\x57", 5);
> +	syscall(__NR_lsetxattr, 0x20000080, 0x200000c0, 0x20000100, 1, 1);
> +	memcpy((void*)0x20000300, "./file0", 8);
> +	syscall(__NR_listxattr, 0x20000300, 0x200002c0, 0x1e);
> +	return 0;
> +#endif
> +}
> diff --git a/tests/generic/712 b/tests/generic/712
> new file mode 100755
> index 00000000..6348a797
> --- /dev/null
> +++ b/tests/generic/712
> @@ -0,0 +1,41 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (c) 2019 Oracle, Inc.  All Rights Reserved.
> +#
> +# FS QA Test No. 712
> +#
> +# Regression test for a bug where XFS corrupts memory if the listxattr buffer
> +# is a particularly well crafted size on a filesystem that supports posix acls.
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +testfile=$TEST_DIR/$seq.txt

I removed this definition, which is not used in the test.

> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/attr
> +
> +# real QA test starts here
> +_supported_fs generic
> +_supported_os Linux
> +_require_acls
> +_require_scratch

I also added

_require_test_program "t_attr_corruption"

Thanks,
Eryu

> +
> +rm -f $seqres.full
> +_scratch_mkfs >> $seqres.full 2>&1
> +_scratch_mount
> +
> +src/t_attr_corruption $SCRATCH_MNT
> +
> +status=0
> +exit
> diff --git a/tests/generic/712.out b/tests/generic/712.out
> new file mode 100644
> index 00000000..a2ba09f3
> --- /dev/null
> +++ b/tests/generic/712.out
> @@ -0,0 +1,2 @@
> +QA output created by 712
> +list attr: Numerical result out of range
> diff --git a/tests/generic/group b/tests/generic/group
> index f56eb475..b3086154 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -529,3 +529,4 @@
>  524 auto quick
>  525 auto quick rw
>  709 auto quick
> +712 auto quick attr

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/3] generic: posix acl extended attribute memory corruption test
  2019-02-16 12:05   ` Eryu Guan
@ 2019-02-16 17:24     ` Darrick J. Wong
  0 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2019-02-16 17:24 UTC (permalink / raw)
  To: Eryu Guan; +Cc: linux-xfs, fstests

On Sat, Feb 16, 2019 at 08:05:30PM +0800, Eryu Guan wrote:
> On Wed, Feb 13, 2019 at 12:48:14PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > XFS had a use-after-free bug when xfs_xattr_put_listent runs out of
> > listxattr buffer space while trying to store the name
> > "system.posix_acl_access" and then corrupts memory by not checking the
> > seen_enough state and then trying to shove "trusted.SGI_ACL_FILE" into
> > the buffer as well.
> > 
> > In order to tickle the bug in a user visible way we must have already
> > put a name in the buffer, so we take advantage of the fact that
> > "security.evm" sorts before "system.posix_acl_access" to make sure this
> > happens.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  .gitignore              |    1 
> >  src/Makefile            |    2 -
> >  src/t_attr_corruption.c |  122 +++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/712       |   41 ++++++++++++++++
> >  tests/generic/712.out   |    2 +
> >  tests/generic/group     |    1 
> >  6 files changed, 168 insertions(+), 1 deletion(-)
> >  create mode 100644 src/t_attr_corruption.c
> >  create mode 100755 tests/generic/712
> >  create mode 100644 tests/generic/712.out
> > 
> > diff --git a/.gitignore b/.gitignore
> > index ea1aac8a..0933dc7d 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -114,6 +114,7 @@
> >  /src/stat_test
> >  /src/swapon
> >  /src/t_access_root
> > +/src/t_attr_corruption
> >  /src/t_dir_offset
> >  /src/t_dir_offset2
> >  /src/t_dir_type
> > diff --git a/src/Makefile b/src/Makefile
> > index 41826585..ae09eb0a 100644
> > --- a/src/Makefile
> > +++ b/src/Makefile
> > @@ -27,7 +27,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
> >  	renameat2 t_getcwd e4compact test-nextquota punch-alternating \
> >  	attr-list-by-handle-cursor-test listxattr dio-interleaved t_dir_type \
> >  	dio-invalidate-cache stat_test t_encrypted_d_revalidate \
> > -	attr_replace_test swapon mkswap
> > +	attr_replace_test swapon mkswap t_attr_corruption
> >  
> >  SUBDIRS = log-writes perf
> >  
> > diff --git a/src/t_attr_corruption.c b/src/t_attr_corruption.c
> > new file mode 100644
> > index 00000000..1fa5e41f
> > --- /dev/null
> > +++ b/src/t_attr_corruption.c
> > @@ -0,0 +1,122 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2019 Oracle.  All Rights Reserved.
> > + * Author: Darrick J. Wong <darrick.wong@oracle.com>
> > + *
> > + * Test program to tickle a use-after-free bug in xfs.
> > + *
> > + * XFS had a use-after-free bug when xfs_xattr_put_listent runs out of
> > + * listxattr buffer space while trying to store the name
> > + * "system.posix_acl_access" and then corrupts memory by not checking the
> > + * seen_enough state and then trying to shove "trusted.SGI_ACL_FILE" into the
> > + * buffer as well.
> > + *
> > + * In order to tickle the bug in a user visible way we must have already put a
> > + * name in the buffer, so we take advantage of the fact that "security.evm"
> > + * sorts before "system.posix_acl_access" to make sure this happens.
> > + *
> > + * If we trigger the bug, the program will print the garbled string
> > + * "rusted.SGI_ACL_FILE".  If the bug is fixed, the flistxattr call returns
> > + * ERANGE.
> > + */
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <fcntl.h>
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <stdint.h>
> > +#include <unistd.h>
> > +#include <attr/xattr.h>
> > +
> > +void die(const char *msg)
> > +{
> > +	perror(msg);
> > +	exit(1);
> > +}
> > +
> > +struct entry {
> > +	uint16_t	a;
> > +	uint16_t	b;
> > +	uint32_t	c;
> > +};
> > +
> > +struct myacl {
> > +	uint32_t	d;
> > +	struct entry	e[4];
> > +};
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +	struct myacl acl = {
> > +		.d = 2,
> > +		.e = {
> > +			{1, 0, 0},
> > +			{4, 0, 0},
> > +			{0x10, 0, 0},
> > +			{0x20, 0, 0},
> > +		},
> > +	};
> > +	char buf[64];
> > +	ssize_t sz;
> > +	int fd;
> > +	int ret;
> > +
> > +	if (argc > 1) {
> > +		ret = chdir(argv[1]);
> > +		if (ret)
> > +			die(argv[1]);
> > +	}
> > +
> > +	fd = creat("file0", 0644);
> > +	if (fd < 0)
> > +		die("create");
> > +
> > +	ret = fsetxattr(fd, "system.posix_acl_access", &acl, sizeof(acl), 0);
> > +	if (ret)
> > +		die("set posix acl");
> > +
> > +	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
> > +	if (ret)
> > +		die("set evm");
> > +
> > +	sz = flistxattr(fd, buf, 30);
> > +	if (sz < 0)
> > +		die("list attr");
> > +
> > +	printf("%s\n", buf);
> > +
> > +	return 0;
> > +
> > +#if 0
> > +	/* original syzkaller reproducer */
> > +
> > +	syscall(__NR_mmap, 0x20000000, 0x1000000, 3, 0x32, -1, 0);
> > +
> > +	memcpy((void*)0x20000180, "./file0", 8);
> > +	syscall(__NR_creat, 0x20000180, 0);
> > +	memcpy((void*)0x20000000, "./file0", 8);
> > +	memcpy((void*)0x20000040, "system.posix_acl_access", 24);
> > +	*(uint32_t*)0x20000680 = 2;
> > +	*(uint16_t*)0x20000684 = 1;
> > +	*(uint16_t*)0x20000686 = 0;
> > +	*(uint32_t*)0x20000688 = 0;
> > +	*(uint16_t*)0x2000068c = 4;
> > +	*(uint16_t*)0x2000068e = 0;
> > +	*(uint32_t*)0x20000690 = 0;
> > +	*(uint16_t*)0x20000694 = 0x10;
> > +	*(uint16_t*)0x20000696 = 0;
> > +	*(uint32_t*)0x20000698 = 0;
> > +	*(uint16_t*)0x2000069c = 0x20;
> > +	*(uint16_t*)0x2000069e = 0;
> > +	*(uint32_t*)0x200006a0 = 0;
> > +	syscall(__NR_setxattr, 0x20000000, 0x20000040, 0x20000680, 0x24, 0);
> > +	memcpy((void*)0x20000080, "./file0", 8);
> > +	memcpy((void*)0x200000c0, "security.evm", 13);
> > +	memcpy((void*)0x20000100, "\x03\x00\x00\x00\x57", 5);
> > +	syscall(__NR_lsetxattr, 0x20000080, 0x200000c0, 0x20000100, 1, 1);
> > +	memcpy((void*)0x20000300, "./file0", 8);
> > +	syscall(__NR_listxattr, 0x20000300, 0x200002c0, 0x1e);
> > +	return 0;
> > +#endif
> > +}
> > diff --git a/tests/generic/712 b/tests/generic/712
> > new file mode 100755
> > index 00000000..6348a797
> > --- /dev/null
> > +++ b/tests/generic/712
> > @@ -0,0 +1,41 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0+
> > +# Copyright (c) 2019 Oracle, Inc.  All Rights Reserved.
> > +#
> > +# FS QA Test No. 712
> > +#
> > +# Regression test for a bug where XFS corrupts memory if the listxattr buffer
> > +# is a particularly well crafted size on a filesystem that supports posix acls.
> > +#
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +tmp=/tmp/$$
> > +status=1	# failure is the default!
> > +testfile=$TEST_DIR/$seq.txt
> 
> I removed this definition, which is not used in the test.
> 
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -f $tmp.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/attr
> > +
> > +# real QA test starts here
> > +_supported_fs generic
> > +_supported_os Linux
> > +_require_acls
> > +_require_scratch
> 
> I also added
> 
> _require_test_program "t_attr_corruption"

Sounds good, thank you!

> Thanks,
> Eryu
> 
> > +
> > +rm -f $seqres.full
> > +_scratch_mkfs >> $seqres.full 2>&1
> > +_scratch_mount
> > +
> > +src/t_attr_corruption $SCRATCH_MNT
> > +
> > +status=0
> > +exit
> > diff --git a/tests/generic/712.out b/tests/generic/712.out
> > new file mode 100644
> > index 00000000..a2ba09f3
> > --- /dev/null
> > +++ b/tests/generic/712.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 712
> > +list attr: Numerical result out of range
> > diff --git a/tests/generic/group b/tests/generic/group
> > index f56eb475..b3086154 100644
> > --- a/tests/generic/group
> > +++ b/tests/generic/group
> > @@ -529,3 +529,4 @@
> >  524 auto quick
> >  525 auto quick rw
> >  709 auto quick
> > +712 auto quick attr

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/3] generic: posix acl extended attribute memory corruption test
  2019-02-13 20:48 ` [PATCH 4/3] generic: posix acl extended attribute memory corruption test Darrick J. Wong
  2019-02-16 12:05   ` Eryu Guan
@ 2019-02-20 13:29   ` David Sterba
  2019-02-20 14:09     ` Holger Hoffstätte
  2019-02-25 18:57   ` Jeff Mahoney
  2 siblings, 1 reply; 12+ messages in thread
From: David Sterba @ 2019-02-20 13:29 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: guaneryu, linux-xfs, fstests

On Wed, Feb 13, 2019 at 12:48:14PM -0800, Darrick J. Wong wrote:
> --- /dev/null
> +++ b/src/t_attr_corruption.c
> @@ -0,0 +1,122 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Oracle.  All Rights Reserved.
> + * Author: Darrick J. Wong <darrick.wong@oracle.com>
> + *
> + * Test program to tickle a use-after-free bug in xfs.
> + *
> + * XFS had a use-after-free bug when xfs_xattr_put_listent runs out of
> + * listxattr buffer space while trying to store the name
> + * "system.posix_acl_access" and then corrupts memory by not checking the
> + * seen_enough state and then trying to shove "trusted.SGI_ACL_FILE" into the
> + * buffer as well.
> + *
> + * In order to tickle the bug in a user visible way we must have already put a
> + * name in the buffer, so we take advantage of the fact that "security.evm"
> + * sorts before "system.posix_acl_access" to make sure this happens.
> + *
> + * If we trigger the bug, the program will print the garbled string
> + * "rusted.SGI_ACL_FILE".  If the bug is fixed, the flistxattr call returns
> + * ERANGE.
> + */
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdint.h>
> +#include <unistd.h>
> +#include <attr/xattr.h>

This does not compile on some systems, sys/xattr.h works (it's provided
by glibc) and is also used by other fstests' sources. I'm not sure where
does attr/xattr.h come from, my devel package for libattr provides only
attr/libattr.h.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/3] generic: posix acl extended attribute memory corruption test
  2019-02-20 13:29   ` David Sterba
@ 2019-02-20 14:09     ` Holger Hoffstätte
  2019-02-20 18:58       ` Darrick J. Wong
  0 siblings, 1 reply; 12+ messages in thread
From: Holger Hoffstätte @ 2019-02-20 14:09 UTC (permalink / raw)
  To: David Sterba; +Cc: Darrick J. Wong, guaneryu, linux-xfs, fstests


On Wed, 20 Feb 2019, David Sterba wrote:

> On Wed, Feb 13, 2019 at 12:48:14PM -0800, Darrick J. Wong wrote:
>> +#include <attr/xattr.h>
>
> This does not compile on some systems, sys/xattr.h works (it's provided
> by glibc) and is also used by other fstests' sources. I'm not sure where
> does attr/xattr.h come from, my devel package for libattr provides only
> attr/libattr.h.

This was removed from attr back in 2015 [1] but apparently caused
many aplications to no longer compile, so some packagers added 
attr/xattr.h back, completing the cycle of confusion.
I just randomly remembered because Gentoo fell into the same sinkhole
and is still climbing out [2].

AFAIK sys/xattr.h is the preferred thing now unless you are building on 
an ancient distro.

cheers
Holger

[1] http://git.savannah.nongnu.org/cgit/attr.git/commit/?id=7921157890d07858d092f4003ca4c6bae9fd2c38
[2] https://bugs.gentoo.org/648864

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/3] generic: posix acl extended attribute memory corruption test
  2019-02-20 14:09     ` Holger Hoffstätte
@ 2019-02-20 18:58       ` Darrick J. Wong
  0 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2019-02-20 18:58 UTC (permalink / raw)
  To: Holger Hoffstätte; +Cc: David Sterba, guaneryu, linux-xfs, fstests

On Wed, Feb 20, 2019 at 03:09:32PM +0100, Holger Hoffstätte wrote:
> 
> On Wed, 20 Feb 2019, David Sterba wrote:
> 
> > On Wed, Feb 13, 2019 at 12:48:14PM -0800, Darrick J. Wong wrote:
> > > +#include <attr/xattr.h>
> > 
> > This does not compile on some systems, sys/xattr.h works (it's provided
> > by glibc) and is also used by other fstests' sources. I'm not sure where
> > does attr/xattr.h come from, my devel package for libattr provides only
> > attr/libattr.h.
> 
> This was removed from attr back in 2015 [1] but apparently caused
> many aplications to no longer compile, so some packagers added attr/xattr.h
> back, completing the cycle of confusion.
> I just randomly remembered because Gentoo fell into the same sinkhole
> and is still climbing out [2].
> 
> AFAIK sys/xattr.h is the preferred thing now unless you are building on an
> ancient distro.

...an ancient distro like Debian Testing or Ubuntu 18.04, which still
ships libattr 2.4.47, which has manpages telling you to use
attr/xattr.h.

OTOH xfsprogs has been using sys/xattr.h since 2009, so patch coming
soon.

--D

> cheers
> Holger
> 
> [1] http://git.savannah.nongnu.org/cgit/attr.git/commit/?id=7921157890d07858d092f4003ca4c6bae9fd2c38
> [2] https://bugs.gentoo.org/648864

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/3] generic: posix acl extended attribute memory corruption test
  2019-02-13 20:48 ` [PATCH 4/3] generic: posix acl extended attribute memory corruption test Darrick J. Wong
  2019-02-16 12:05   ` Eryu Guan
  2019-02-20 13:29   ` David Sterba
@ 2019-02-25 18:57   ` Jeff Mahoney
  2019-02-25 21:00     ` Darrick J. Wong
  2 siblings, 1 reply; 12+ messages in thread
From: Jeff Mahoney @ 2019-02-25 18:57 UTC (permalink / raw)
  To: Darrick J. Wong, guaneryu; +Cc: linux-xfs, fstests

On 2/13/19 3:48 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> XFS had a use-after-free bug when xfs_xattr_put_listent runs out of
> listxattr buffer space while trying to store the name
> "system.posix_acl_access" and then corrupts memory by not checking the
> seen_enough state and then trying to shove "trusted.SGI_ACL_FILE" into
> the buffer as well.
> 
> In order to tickle the bug in a user visible way we must have already
> put a name in the buffer, so we take advantage of the fact that
> "security.evm" sorts before "system.posix_acl_access" to make sure this
> happens.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

[...]

> +
> +int main(int argc, char *argv[])
> +{
> +	struct myacl acl = {
> +		.d = 2,
> +		.e = {
> +			{1, 0, 0},
> +			{4, 0, 0},
> +			{0x10, 0, 0},
> +			{0x20, 0, 0},
> +		},
> +	};
> +	char buf[64];
> +	ssize_t sz;
> +	int fd;
> +	int ret;
> +
> +	if (argc > 1) {
> +		ret = chdir(argv[1]);
> +		if (ret)
> +			die(argv[1]);
> +	}
> +
> +	fd = creat("file0", 0644);
> +	if (fd < 0)
> +		die("create");
> +
> +	ret = fsetxattr(fd, "system.posix_acl_access", &acl, sizeof(acl), 0);
> +	if (ret)
> +		die("set posix acl");
> +
> +	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
> +	if (ret)
> +		die("set evm");

How is this working on your test system?  The EVM xattr is a formatted
structure and this is passing it an uninitialized buffer.  It *should*
return EPERM and on our test systems it is.

Using security.capability will sort before system.posix_acl_access and
accepts unformatted contents.

-Jeff

> +	sz = flistxattr(fd, buf, 30);
> +	if (sz < 0)
> +		die("list attr");
> +
> +	printf("%s\n", buf);
> +
> +	return 0;
> +
> +#if 0
> +	/* original syzkaller reproducer */
> +
> +	syscall(__NR_mmap, 0x20000000, 0x1000000, 3, 0x32, -1, 0);
> +
> +	memcpy((void*)0x20000180, "./file0", 8);
> +	syscall(__NR_creat, 0x20000180, 0);
> +	memcpy((void*)0x20000000, "./file0", 8);
> +	memcpy((void*)0x20000040, "system.posix_acl_access", 24);
> +	*(uint32_t*)0x20000680 = 2;
> +	*(uint16_t*)0x20000684 = 1;
> +	*(uint16_t*)0x20000686 = 0;
> +	*(uint32_t*)0x20000688 = 0;
> +	*(uint16_t*)0x2000068c = 4;
> +	*(uint16_t*)0x2000068e = 0;
> +	*(uint32_t*)0x20000690 = 0;
> +	*(uint16_t*)0x20000694 = 0x10;
> +	*(uint16_t*)0x20000696 = 0;
> +	*(uint32_t*)0x20000698 = 0;
> +	*(uint16_t*)0x2000069c = 0x20;
> +	*(uint16_t*)0x2000069e = 0;
> +	*(uint32_t*)0x200006a0 = 0;
> +	syscall(__NR_setxattr, 0x20000000, 0x20000040, 0x20000680, 0x24, 0);
> +	memcpy((void*)0x20000080, "./file0", 8);
> +	memcpy((void*)0x200000c0, "security.evm", 13);
> +	memcpy((void*)0x20000100, "\x03\x00\x00\x00\x57", 5);
> +	syscall(__NR_lsetxattr, 0x20000080, 0x200000c0, 0x20000100, 1, 1);
> +	memcpy((void*)0x20000300, "./file0", 8);
> +	syscall(__NR_listxattr, 0x20000300, 0x200002c0, 0x1e);
> +	return 0;
> +#endif
> +}



-- 
Jeff Mahoney
SUSE Labs

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/3] generic: posix acl extended attribute memory corruption test
  2019-02-25 18:57   ` Jeff Mahoney
@ 2019-02-25 21:00     ` Darrick J. Wong
  0 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2019-02-25 21:00 UTC (permalink / raw)
  To: Jeff Mahoney; +Cc: guaneryu, linux-xfs, fstests

On Mon, Feb 25, 2019 at 01:57:51PM -0500, Jeff Mahoney wrote:
> On 2/13/19 3:48 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > XFS had a use-after-free bug when xfs_xattr_put_listent runs out of
> > listxattr buffer space while trying to store the name
> > "system.posix_acl_access" and then corrupts memory by not checking the
> > seen_enough state and then trying to shove "trusted.SGI_ACL_FILE" into
> > the buffer as well.
> > 
> > In order to tickle the bug in a user visible way we must have already
> > put a name in the buffer, so we take advantage of the fact that
> > "security.evm" sorts before "system.posix_acl_access" to make sure this
> > happens.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> [...]
> 
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +	struct myacl acl = {
> > +		.d = 2,
> > +		.e = {
> > +			{1, 0, 0},
> > +			{4, 0, 0},
> > +			{0x10, 0, 0},
> > +			{0x20, 0, 0},
> > +		},
> > +	};
> > +	char buf[64];
> > +	ssize_t sz;
> > +	int fd;
> > +	int ret;
> > +
> > +	if (argc > 1) {
> > +		ret = chdir(argv[1]);
> > +		if (ret)
> > +			die(argv[1]);
> > +	}
> > +
> > +	fd = creat("file0", 0644);
> > +	if (fd < 0)
> > +		die("create");
> > +
> > +	ret = fsetxattr(fd, "system.posix_acl_access", &acl, sizeof(acl), 0);
> > +	if (ret)
> > +		die("set posix acl");
> > +
> > +	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
> > +	if (ret)
> > +		die("set evm");
> 
> How is this working on your test system?

CONFIG_EVM=n, that's how. :(

> The EVM xattr is a formatted structure and this is passing it an
> uninitialized buffer.  It *should* return EPERM and on our test
> systems it is.

Er... what is the structure of the evm attr, anyway?  Does passing in a
single byte 0x03 actually work?

Oh, it's in security/integrity/integrity.h, that's why I couldn't find
it....

enum evm_ima_xattr_type {
	IMA_XATTR_DIGEST = 0x01,
	EVM_XATTR_HMAC,
	EVM_IMA_XATTR_DIGSIG,
	IMA_XATTR_DIGEST_NG,
	EVM_XATTR_PORTABLE_DIGSIG,
	IMA_XATTR_LAST
};

struct evm_ima_xattr_data {
	u8 type;
	u8 digest[SHA1_DIGEST_SIZE];
} __packed;


So I guess we're passing in a xattr_data of type EVM_IMA_XATTR_DIGSIG?
With no actual digest information, which seems suspect to me.

Now I wonder if the VM they used to generate the syzkaller report has
EVM enabled....

(And this is why I hate syzkaller reports, all of the mechanisation I
can't (under)stand and none of the context to help me write a decent
regression test case that actually just friggin works.)

> Using security.capability will sort before system.posix_acl_access and
> accepts unformatted contents.

I'll try that and report back, thank you.  Sorry for the mess.

--D

> -Jeff
> 
> > +	sz = flistxattr(fd, buf, 30);
> > +	if (sz < 0)
> > +		die("list attr");
> > +
> > +	printf("%s\n", buf);
> > +
> > +	return 0;
> > +
> > +#if 0
> > +	/* original syzkaller reproducer */
> > +
> > +	syscall(__NR_mmap, 0x20000000, 0x1000000, 3, 0x32, -1, 0);
> > +
> > +	memcpy((void*)0x20000180, "./file0", 8);
> > +	syscall(__NR_creat, 0x20000180, 0);
> > +	memcpy((void*)0x20000000, "./file0", 8);
> > +	memcpy((void*)0x20000040, "system.posix_acl_access", 24);
> > +	*(uint32_t*)0x20000680 = 2;
> > +	*(uint16_t*)0x20000684 = 1;
> > +	*(uint16_t*)0x20000686 = 0;
> > +	*(uint32_t*)0x20000688 = 0;
> > +	*(uint16_t*)0x2000068c = 4;
> > +	*(uint16_t*)0x2000068e = 0;
> > +	*(uint32_t*)0x20000690 = 0;
> > +	*(uint16_t*)0x20000694 = 0x10;
> > +	*(uint16_t*)0x20000696 = 0;
> > +	*(uint32_t*)0x20000698 = 0;
> > +	*(uint16_t*)0x2000069c = 0x20;
> > +	*(uint16_t*)0x2000069e = 0;
> > +	*(uint32_t*)0x200006a0 = 0;
> > +	syscall(__NR_setxattr, 0x20000000, 0x20000040, 0x20000680, 0x24, 0);
> > +	memcpy((void*)0x20000080, "./file0", 8);
> > +	memcpy((void*)0x200000c0, "security.evm", 13);
> > +	memcpy((void*)0x20000100, "\x03\x00\x00\x00\x57", 5);
> > +	syscall(__NR_lsetxattr, 0x20000080, 0x200000c0, 0x20000100, 1, 1);
> > +	memcpy((void*)0x20000300, "./file0", 8);
> > +	syscall(__NR_listxattr, 0x20000300, 0x200002c0, 0x1e);
> > +	return 0;
> > +#endif
> > +}
> 
> 
> 
> -- 
> Jeff Mahoney
> SUSE Labs

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-02-25 21:01 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-12  2:17 [PATCH 0/3] fstests: fixes and new tests Darrick J. Wong
2019-02-12  2:17 ` [PATCH 1/3] common: fix kmemleak to work with sections Darrick J. Wong
2019-02-12  2:17 ` [PATCH 2/3] common: fix _require_btime for lazy filesystems Darrick J. Wong
2019-02-12  2:17 ` [PATCH 3/3] generic: check for reasonable inode creation time Darrick J. Wong
2019-02-13 20:48 ` [PATCH 4/3] generic: posix acl extended attribute memory corruption test Darrick J. Wong
2019-02-16 12:05   ` Eryu Guan
2019-02-16 17:24     ` Darrick J. Wong
2019-02-20 13:29   ` David Sterba
2019-02-20 14:09     ` Holger Hoffstätte
2019-02-20 18:58       ` Darrick J. Wong
2019-02-25 18:57   ` Jeff Mahoney
2019-02-25 21:00     ` Darrick J. Wong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).