* [PATCH] xfs: fix overflow in xfs_attr3_leaf_verify
@ 2018-11-02 4:31 Dave Chinner
2018-11-02 15:10 ` Darrick J. Wong
0 siblings, 1 reply; 2+ messages in thread
From: Dave Chinner @ 2018-11-02 4:31 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
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 <dchinner@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] xfs: fix overflow in xfs_attr3_leaf_verify
2018-11-02 4:31 [PATCH] xfs: fix overflow in xfs_attr3_leaf_verify Dave Chinner
@ 2018-11-02 15:10 ` Darrick J. Wong
0 siblings, 0 replies; 2+ messages in thread
From: Darrick J. Wong @ 2018-11-02 15:10 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Fri, Nov 02, 2018 at 03:31:16PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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 <dchinner@redhat.com>
DOH. :(
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-11-03 0:17 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-02 4:31 [PATCH] xfs: fix overflow in xfs_attr3_leaf_verify Dave Chinner
2018-11-02 15:10 ` 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