Skip to contentSkip to conversation

xfs: catch invalid negative blknos in _xfs_buf_find()

PATCH NAK Eric Sandeen 3 participants 4 messages Nov 19, 2014 – Nov 26, 2014 XFS filesystem

lore
Share & tools
lore

Eric Sandeen

author
#0 lore raw

PATCH 1/1 xfs: catch invalid negative blknos in _xfs_buf_find()

Here blkno is a daddr_t, which is a __s64; it's possible to hold a value which is negative, and thus pass the (blkno >= eofs) test. Then we try to do a xfs_perag_get() for a ridiculous agno via xfs_daddr_to_agno(), and bad things happen when that fails, and returns a null pag which is dereferenced shortly thereafter.

Found via a user-supplied fuzzed image...

Signed-off-by Eric Sandeen <[email protected]>

---

fs/xfs/xfs_buf.c +1 -1
index 24b4ebe..f54a497 100644
@@ -463,7 +463,7 @@ _xfs_buf_find(
 	 * have to check that the buffer falls within the filesystem bounds.
 	 */
 	eofs = XFS_FSB_TO_BB(btp->bt_mount, btp->bt_mount->m_sb.sb_dblocks);
-	if (blkno >= eofs) {
+	if (blkno < 0 || blkno >= eofs) {
 		/*
 		 * XXX (dgc): we should really be returning -EFSCORRUPTED here,
 		 * but none of the higher level infrastructure supports

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

NAK
#1 lore raw
Quoted 8 lines · Eric Sandeen

Here blkno is a daddr_t, which is a __s64; it's possible to hold a value which is negative, and thus pass the (blkno >= eofs) test. Then we try to do a xfs_perag_get() for a ridiculous agno via xfs_daddr_to_agno(), and bad things happen when that fails, and returns a null pag which is dereferenced shortly thereafter.

Found via a user-supplied fuzzed image...

NAK - this needs a bit more love; if we catch this and fail, the caller may still do something crazy with this data.

V2 coming in a bit.

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#2 lore raw
In reply to Eric Sandeen
Quoted 10 lines · Eric Sandeen

Here blkno is a daddr_t, which is a __s64; it's possible to hold a value which is negative, and thus pass the (blkno >= eofs) test. Then we try to do a xfs_perag_get() for a ridiculous agno via xfs_daddr_to_agno(), and bad things happen when that fails, and returns a null pag which is dereferenced shortly thereafter.

Found via a user-supplied fuzzed image...

Signed-off-by Eric Sandeen <[email protected]>

Ok, so after further digging, I think this patch is fine on its own, and could use review. How and if and when this function returns errors, and whether errors should get this far, etc, are all subjects for other patches. IF we're range-checking here, we should do both halves of the range checking properly so ... review appreciated.

Thanks,
-Eric

Quoted 16 lines

---

fs/xfs/xfs_buf.c +1 -1
index 24b4ebe..f54a497 100644
@@ -463,7 +463,7 @@ _xfs_buf_find(
 	 * have to check that the buffer falls within the filesystem bounds.
 	 */
 	eofs = XFS_FSB_TO_BB(btp->bt_mount, btp->bt_mount->m_sb.sb_dblocks);
-	if (blkno >= eofs) {
+	if (blkno < 0 || blkno >= eofs) {
 		/*
 		 * XXX (dgc): we should really be returning -EFSCORRUPTED here,
 		 * but none of the higher level infrastructure supports
Quoted 5 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Reviewed-by on v1 1/1
#3 lore raw
In reply to Eric Sandeen
Quoted 11 lines · Eric Sandeen

Here blkno is a daddr_t, which is a __s64; it's possible to hold a value which is negative, and thus pass the (blkno>= eofs) test. Then we try to do a xfs_perag_get() for a ridiculous agno via xfs_daddr_to_agno(), and bad things happen when that fails, and returns a null pag which is dereferenced shortly thereafter.

Found via a user-supplied fuzzed image...

Signed-off-by Eric Sandeen<[email protected]>

---

Looks good.

I did a little playing with sending the try lock failure (EAGAIN?) and EFSCORRUPT error status up the stack. It looked straight forward and could save a xfs_buf allocation when we know it is not necessary.

Reviewed-by Mark Tinguely <[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs