[xfstests PATCH 0/3] Fix test bugs related to fs.verity.require_signatures

[xfstests PATCH 0/3] Fix test bugs related to fs.verity.require_signatures

[Eric Biggers]

This is a replacement for Andrey's patchset
"generic/577: fix hash check and add metadata cleaning".
It handles some other things that were overlooked.

Eric Biggers (3):
  common/verity: fix _fsv_have_hash_algorithm() with required signatures
  generic/577: add missing file removal before empty file test
  tests: fix some tests for systems with fs.verity.require_signatures=1

 common/verity     | 69 +++++++++++++++++++++++++++++++----------------
 tests/btrfs/290   |  9 +++++++
 tests/btrfs/291   |  2 ++
 tests/generic/577 |  1 +
 tests/generic/624 |  8 ++++++
 tests/generic/692 |  8 ++++++
 6 files changed, 74 insertions(+), 23 deletions(-)


base-commit: a75c5f50584e03ca7862ad51f48efd2d524d1dc5
-- 
2.38.1

[xfstests PATCH 1/3] common/verity: fix _fsv_have_hash_algorithm() with required signatures

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

_fsv_have_hash_algorithm() uses _fsv_enable() without a signature, so it
always fails when called while fs.verity.require_signatures=1.  This
happens in generic/577, which tests file signing.  This wasn't noticed
because it just made part of generic/577 always be skipped.

Fix this by making _fsv_have_hash_algorithm() temporarily set
fs.verity.require_signatures to 0.

Since the previous value needs to be restored afterwards, whether it is
0 or 1, also make some changes to the fs.verity.require_signatures
helper functions to allow the restoration of the previous value, rather
than the value that existed at the beginning of the test.

Finally, make a couple related cleanups: make _fsv_have_hash_algorithm()
always delete the file it works with, and also update the similar code
in _require_scratch_verity().

Reported-by: Andrey Albershteyn <aalbersh@redhat.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 common/verity | 69 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 23 deletions(-)

diff --git a/common/verity b/common/verity
index 65a39d3e..4a9c9872 100644
--- a/common/verity
+++ b/common/verity
@@ -44,12 +44,13 @@ _require_scratch_verity()
 	# doesn't work on ext3-style filesystems.  So, try actually using it.
 	echo foo > $SCRATCH_MNT/tmpfile
 	_disable_fsverity_signatures
-	if ! _fsv_enable $SCRATCH_MNT/tmpfile; then
-		_restore_fsverity_signatures
+	_fsv_enable $SCRATCH_MNT/tmpfile
+	local status=$?
+	_restore_prev_fsverity_signatures
+	rm -f $SCRATCH_MNT/tmpfile
+	if (( $status != 0 )); then
 		_notrun "$FSTYP verity isn't usable by default with these mkfs options"
 	fi
-	_restore_fsverity_signatures
-	rm -f $SCRATCH_MNT/tmpfile
 
 	_scratch_unmount
 
@@ -104,30 +105,52 @@ _fsv_load_cert()
 # Disable mandatory signatures for fs-verity files, if they are supported.
 _disable_fsverity_signatures()
 {
-	if [ -e /proc/sys/fs/verity/require_signatures ]; then
-		if [ -z "$FSVERITY_SIG_CTL_ORIG" ]; then
-			FSVERITY_SIG_CTL_ORIG=$(</proc/sys/fs/verity/require_signatures)
-		fi
-		echo 0 > /proc/sys/fs/verity/require_signatures
-	fi
+	_set_fsverity_require_signatures 0
 }
 
 # Enable mandatory signatures for fs-verity files.
 # This assumes that _require_fsverity_builtin_signatures() was called.
 _enable_fsverity_signatures()
 {
-	if [ -z "$FSVERITY_SIG_CTL_ORIG" ]; then
-		FSVERITY_SIG_CTL_ORIG=$(</proc/sys/fs/verity/require_signatures)
-	fi
-	echo 1 > /proc/sys/fs/verity/require_signatures
+	_set_fsverity_require_signatures 1
 }
 
-# Restore the original signature verification setting.
+# Restore the original value of fs.verity.require_signatures, i.e. the value it
+# had at the beginning of the test.
 _restore_fsverity_signatures()
 {
-        if [ -n "$FSVERITY_SIG_CTL_ORIG" ]; then
-                echo "$FSVERITY_SIG_CTL_ORIG" > /proc/sys/fs/verity/require_signatures
-        fi
+	if [ -n "$FSVERITY_SIG_CTL_ORIG" ]; then
+		_set_fsverity_require_signatures "$FSVERITY_SIG_CTL_ORIG"
+	fi
+}
+
+# Restore the previous value of fs.verity.require_signatures, i.e. the value it
+# had just before it was last written to.
+_restore_prev_fsverity_signatures()
+{
+	if [ -n "$FSVERITY_SIG_CTL_PREV" ]; then
+		_set_fsverity_require_signatures "$FSVERITY_SIG_CTL_PREV"
+	fi
+}
+
+_set_fsverity_require_signatures()
+{
+	local newval=$1
+	if [ ! -e /proc/sys/fs/verity/require_signatures ]; then
+		# If the kernel doesn't support fs.verity.require_signatures,
+		# then trying to disable it is fine, but enabling it is not.
+		if [ "$newval" != 0 ]; then
+			# Forgot to call _require_fsverity_builtin_signatures().
+			_fail "fs.verity.require_signatures is missing"
+		fi
+		return
+	fi
+	local oldval=$(</proc/sys/fs/verity/require_signatures)
+	FSVERITY_SIG_CTL_PREV=$oldval
+	if [ -z "$FSVERITY_SIG_CTL_ORIG" ]; then
+		FSVERITY_SIG_CTL_ORIG=$oldval
+	fi
+	echo "$newval" > /proc/sys/fs/verity/require_signatures
 }
 
 # Require userspace and kernel support for 'fsverity dump_metadata'.
@@ -245,14 +268,14 @@ _fsv_have_hash_algorithm()
 	local hash_alg=$1
 	local test_file=$2
 
+	_disable_fsverity_signatures
 	rm -f $test_file
 	head -c 4096 /dev/zero > $test_file
-	if ! _fsv_enable --hash-alg=$hash_alg $test_file &>> $seqres.full; then
-		# no kernel support
-		return 1
-	fi
+	_fsv_enable --hash-alg=$hash_alg $test_file &>> $seqres.full
+	local status=$?
+	_restore_prev_fsverity_signatures
 	rm -f $test_file
-	return 0
+	return $status
 }
 
 #
-- 
2.38.1

Re: [xfstests PATCH 1/3] common/verity: fix _fsv_have_hash_algorithm() with required signatures

[Andrey Albershteyn]

On Thu, Nov 03, 2022 at 11:47:40PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> _fsv_have_hash_algorithm() uses _fsv_enable() without a signature, so it
> always fails when called while fs.verity.require_signatures=1.  This
> happens in generic/577, which tests file signing.  This wasn't noticed
> because it just made part of generic/577 always be skipped.
> 
> Fix this by making _fsv_have_hash_algorithm() temporarily set
> fs.verity.require_signatures to 0.
> 
> Since the previous value needs to be restored afterwards, whether it is
> 0 or 1, also make some changes to the fs.verity.require_signatures
> helper functions to allow the restoration of the previous value, rather
> than the value that existed at the beginning of the test.
> 
> Finally, make a couple related cleanups: make _fsv_have_hash_algorithm()
> always delete the file it works with, and also update the similar code
> in _require_scratch_verity().
> 
> Reported-by: Andrey Albershteyn <aalbersh@redhat.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  common/verity | 69 ++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 46 insertions(+), 23 deletions(-)
> 

Looks good.
Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>

-- 
- Andrey

[xfstests PATCH 2/3] generic/577: add missing file removal before empty file test

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The fix for _fsv_have_hash_algorithm() exposed a bug where one of the
test cases in generic/577 isn't deleting the file from the previous test
case before it tries to write to it.  That causes a failure, since due
to the fix for _fsv_have_hash_algorithm(), the file from the previous
test case now ends up with verity enabled and therefore cannot be
written to.  Fix this by deleting the file.

Reported-by: Andrey Albershteyn <aalbersh@redhat.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 tests/generic/577 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/generic/577 b/tests/generic/577
index 98c3888f..5f7e0573 100755
--- a/tests/generic/577
+++ b/tests/generic/577
@@ -121,6 +121,7 @@ if _fsv_have_hash_algorithm sha512 $fsv_file; then
 fi
 
 echo -e "\n# Testing empty file"
+rm -f $fsv_file
 echo -n > $fsv_file
 _fsv_sign $fsv_file $sigfile.emptyfile --key=$keyfile --cert=$certfile | \
 		_filter_scratch
-- 
2.38.1

Re: [xfstests PATCH 2/3] generic/577: add missing file removal before empty file test

[Andrey Albershteyn]

On Thu, Nov 03, 2022 at 11:47:41PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> The fix for _fsv_have_hash_algorithm() exposed a bug where one of the
> test cases in generic/577 isn't deleting the file from the previous test
> case before it tries to write to it.  That causes a failure, since due
> to the fix for _fsv_have_hash_algorithm(), the file from the previous
> test case now ends up with verity enabled and therefore cannot be
> written to.  Fix this by deleting the file.
> 
> Reported-by: Andrey Albershteyn <aalbersh@redhat.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  tests/generic/577 | 1 +
>  1 file changed, 1 insertion(+)
> 

Looks good.
Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>

-- 
- Andrey

[xfstests PATCH 3/3] tests: fix some tests for systems with fs.verity.require_signatures=1

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Some of the newer verity tests don't work properly on systems where
fs.verity.require_signatures is enabled, either because they forget to
disable it at the beginning of the test, or they forget to re-enable it
afterwards, or both.  Fix this.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 tests/btrfs/290   | 9 +++++++++
 tests/btrfs/291   | 2 ++
 tests/generic/624 | 8 ++++++++
 tests/generic/692 | 8 ++++++++
 4 files changed, 27 insertions(+)

diff --git a/tests/btrfs/290 b/tests/btrfs/290
index b7254c5e..06a58f47 100755
--- a/tests/btrfs/290
+++ b/tests/btrfs/290
@@ -15,6 +15,14 @@ _begin_fstest auto quick verity
 . ./common/filter
 . ./common/verity
 
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	_restore_fsverity_signatures
+	rm -f $tmp.*
+}
+
 # real QA test starts here
 _supported_fs btrfs
 _require_scratch_verity
@@ -24,6 +32,7 @@ _require_xfs_io_command "falloc"
 _require_xfs_io_command "pread"
 _require_xfs_io_command "pwrite"
 _require_btrfs_corrupt_block
+_disable_fsverity_signatures
 
 get_ino() {
 	local file=$1
diff --git a/tests/btrfs/291 b/tests/btrfs/291
index bbdd183d..c5947133 100755
--- a/tests/btrfs/291
+++ b/tests/btrfs/291
@@ -23,6 +23,7 @@ _cleanup()
 	rm -f $img
 	$LVM_PROG vgremove -f -y $vgname >>$seqres.full 2>&1
 	losetup -d $loop_dev >>$seqres.full 2>&1
+	_restore_fsverity_signatures
 }
 
 # Import common functions.
@@ -43,6 +44,7 @@ _require_command $LVM_PROG lvm
 _require_scratch_verity
 _require_btrfs_command inspect-internal dump-tree
 _require_test_program "log-writes/replay-log"
+_disable_fsverity_signatures
 
 sync_loop() {
 	i=$1
diff --git a/tests/generic/624 b/tests/generic/624
index 89fbf256..7c447289 100755
--- a/tests/generic/624
+++ b/tests/generic/624
@@ -10,6 +10,14 @@
 . ./common/preamble
 _begin_fstest auto quick verity
 
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	_restore_fsverity_signatures
+	rm -f $tmp.*
+}
+
 . ./common/filter
 . ./common/verity
 
diff --git a/tests/generic/692 b/tests/generic/692
index 0bb1fd33..d6da734b 100644
--- a/tests/generic/692
+++ b/tests/generic/692
@@ -15,6 +15,13 @@
 . ./common/preamble
 _begin_fstest auto quick verity
 
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	_restore_fsverity_signatures
+	rm -f $tmp.*
+}
 
 # Import common functions.
 . ./common/filter
@@ -26,6 +33,7 @@ _require_test
 _require_math
 _require_scratch_verity
 _require_fsverity_max_file_size_limit
+_disable_fsverity_signatures
 
 _scratch_mkfs_verity &>> $seqres.full
 _scratch_mount
-- 
2.38.1

Re: [xfstests PATCH 3/3] tests: fix some tests for systems with fs.verity.require_signatures=1

[Andrey Albershteyn]

On Thu, Nov 03, 2022 at 11:47:42PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Some of the newer verity tests don't work properly on systems where
> fs.verity.require_signatures is enabled, either because they forget to
> disable it at the beginning of the test, or they forget to re-enable it
> afterwards, or both.  Fix this.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  tests/btrfs/290   | 9 +++++++++
>  tests/btrfs/291   | 2 ++
>  tests/generic/624 | 8 ++++++++
>  tests/generic/692 | 8 ++++++++
>  4 files changed, 27 insertions(+)
> 

Looks good. I haven't run btrfs tests but change looks ok.
Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>

-- 
- Andrey