On Wed, Aug 26, 2015 at 10:45:02AM +1000, Dave Chinner wrote:

On Tue, Aug 25, 2015 at 05:32:59PM -0700, Darrick J. Wong wrote:

quoted

Check the v5 fields (uuid, blocknr, owner) of attribute blocks for
obvious errors while scanning xattr blocks.  If the ownership info
is incorrect, kill the block.

Signed-off-by: Darrick J. Wong <redacted>

Why hasn't the buffer verifier done this validation?

Maybe I'm confused here, so here's what I think is going on:

AFAICT most of the verifiers do things like this:

if (crcs_enabled && cksum_verification fails) {
	xfs_buf_ioerror(bp, -EFSBADCRC);
} else if (header_is_insane) {
	xfs_buf_ioerror(bp, -EFSCORRUPTED);
}

The fuzzer corrupts the UUID without updating the CRC.  The verifier first
checks the CRC and it doesn't match, so it sets b_error to -EFSBADCRC and
doesn't get to the header check.  Then the verifier returns, so we end up back
in process_longform_attr.  Where do we set -EFSCORRUPTED when the CRC also
doesn't match?

--D

quoted

@@ -1564,6 +1602,13 @@ process_longform_attr(
 	if (bp->b_error == -EFSBADCRC)
 		(*repair)++;
 
+	/* is this block sane? */
+	if (__check_attr_header(mp, bp, ino)) {
+		*repair = 0;
+		libxfs_putbuf(bp);
+		return 1;
+	}

As you can see the above hunk has a bad CRC check from the verifier,
and if the attr header is wrong then the verifier should be setting
bp->b_error == -EFSCORRUPTED.

So shouldn't this simply be:

+	if (bp->b_error == -EFSCORRUPTED) {
+		*repair = 0;
+		libxfs_putbuf(bp);
+		return 1;
+	}
+

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs