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:
> > 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 <darrick.wong@xxxxxxxxxx>
>
> 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
>
> > @@ -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@xxxxxxxxxxxxx
|