From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp2120.oracle.com ([156.151.31.85]:57500 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726318AbeKCARn (ORCPT ); Fri, 2 Nov 2018 20:17:43 -0400 Date: Fri, 2 Nov 2018 08:10:09 -0700 From: "Darrick J. Wong" Subject: Re: [PATCH] xfs: fix overflow in xfs_attr3_leaf_verify Message-ID: <20181102151009.GS4135@magnolia> References: <20181102043116.24695-1-david@fromorbit.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181102043116.24695-1-david@fromorbit.com> Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: Dave Chinner Cc: linux-xfs@vger.kernel.org On Fri, Nov 02, 2018 at 03:31:16PM +1100, Dave Chinner wrote: > From: Dave Chinner > > generic/070 on 64k block size filesystems is failing with a verifier > corruption on writeback or an attribute leaf block: > > [ 94.973083] XFS (pmem0): Metadata corruption detected at xfs_attr3_leaf_verify+0x246/0x260, xfs_attr3_leaf block 0x811480 > [ 94.975623] XFS (pmem0): Unmount and run xfs_repair > [ 94.976720] XFS (pmem0): First 128 bytes of corrupted metadata buffer: > [ 94.978270] 000000004b2e7b45: 00 00 00 00 00 00 00 00 3b ee 00 00 00 00 00 00 ........;....... > [ 94.980268] 000000006b1db90b: 00 00 00 00 00 81 14 80 00 00 00 00 00 00 00 00 ................ > [ 94.982251] 00000000433f2407: 22 7b 5c 82 2d 5c 47 4c bb 31 1c 37 fa a9 ce d6 "{\.-\GL.1.7.... > [ 94.984157] 0000000010dc7dfb: 00 00 00 00 00 81 04 8a 00 0a 18 e8 dd 94 01 00 ................ > [ 94.986215] 00000000d5a19229: 00 a0 dc f4 fe 98 01 68 f0 d8 07 e0 00 00 00 00 .......h........ > [ 94.988171] 00000000521df36c: 0c 2d 32 e2 fe 20 01 00 0c 2d 58 65 fe 0c 01 00 .-2.. ...-Xe.... > [ 94.990162] 000000008477ae06: 0c 2d 5b 66 fe 8c 01 00 0c 2d 71 35 fe 7c 01 00 .-[f.....-q5.|.. > [ 94.992139] 00000000a4a6bca6: 0c 2d 72 37 fc d4 01 00 0c 2d d8 b8 f0 90 01 00 .-r7.....-...... > [ 94.994789] XFS (pmem0): xfs_do_force_shutdown(0x8) called from line 1453 of file fs/xfs/xfs_buf.c. Return address = ffffffff815365f3 > > This is failing this check: > > end = ichdr.freemap[i].base + ichdr.freemap[i].size; > if (end < ichdr.freemap[i].base) > >>>>> return __this_address; > if (end > mp->m_attr_geo->blksize) > return __this_address; > > And from the buffer output above, the freemap array is: > > freemap[0].base = 0x00a0 > freemap[0].size = 0xdcf4 end = 0xdd94 > freemap[1].base = 0xfe98 > freemap[1].size = 0x0168 end = 0x10000 > freemap[2].base = 0xf0d8 > freemap[2].size = 0x07e0 end = 0xf8b8 > > These all look valid - the block size is 0x10000 and so from the > last check in the above verifier fragment we know that the end > of freemap[1] is valid. The problem is that end is declared as: > > uint16_t end; > > And (uint16_t)0x10000 = 0. So we have a verifier bug here, not a > corruption. Fix the verifier to use uint32_t types for the check and > hence avoid the overflow. > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=201577 > Signed-off-by: Dave Chinner DOH. :( Reviewed-by: Darrick J. Wong --D > --- > fs/xfs/libxfs/xfs_attr_leaf.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > index 6fc5425b1474..2652d00842d6 100644 > --- a/fs/xfs/libxfs/xfs_attr_leaf.c > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > @@ -243,7 +243,7 @@ xfs_attr3_leaf_verify( > struct xfs_mount *mp = bp->b_target->bt_mount; > struct xfs_attr_leafblock *leaf = bp->b_addr; > struct xfs_attr_leaf_entry *entries; > - uint16_t end; > + uint32_t end; /* must be 32bit - see below */ > int i; > > xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf); > @@ -293,6 +293,11 @@ xfs_attr3_leaf_verify( > /* > * Quickly check the freemap information. Attribute data has to be > * aligned to 4-byte boundaries, and likewise for the free space. > + * > + * Note that for 64k block size filesystems, the freemap entries cannot > + * overflow as they are only be16 fields. However, when checking end > + * pointer of the freemap, we have to be careful to detect overflows and > + * so use uint32_t for those checks. > */ > for (i = 0; i < XFS_ATTR_LEAF_MAPSIZE; i++) { > if (ichdr.freemap[i].base > mp->m_attr_geo->blksize) > @@ -303,7 +308,9 @@ xfs_attr3_leaf_verify( > return __this_address; > if (ichdr.freemap[i].size & 0x3) > return __this_address; > - end = ichdr.freemap[i].base + ichdr.freemap[i].size; > + > + /* be care of 16 bit overflows here */ > + end = (uint32_t)ichdr.freemap[i].base + ichdr.freemap[i].size; > if (end < ichdr.freemap[i].base) > return __this_address; > if (end > mp->m_attr_geo->blksize) > -- > 2.19.1 >