Skip site navigation (1)Skip section navigation (2) 
Date:      Wed, 23 Dec 2020 16:17:44 GMT
From:      Mark Johnston <markj@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org
Subject:   git: a7a7c306bfb0 - md: Fix a read-after-free in BIO_GETATTR handling
Message-ID:  <202012231617.0BNGHisT055146@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
The branch main has been updated by markj:

URL: https://cgit.FreeBSD.org/src/commit/?id=a7a7c306bfb0d8d1a83569a098cf6cde492f8bf7

commit a7a7c306bfb0d8d1a83569a098cf6cde492f8bf7
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2020-12-23 16:13:31 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2020-12-23 16:16:40 +0000

    md: Fix a read-after-free in BIO_GETATTR handling
    
    g_handleattr_int() consumes the bio if the attribute matches, so when we
    check bp->bio_cmd bp may have been freed.
    
    Move GETATTR handling to a separate function to avoid the problem.  We
    do not need to set bio_completed for such bios, g_handleattr_int() will
    handle it.  Also remove the setting of bio_resid before the
    devstat_end_transaction_bio() call.  All of the md(4) bio handlers set
    bio_resid already.
    
    Reported by:    KASAN
    Reviewed by:    kib
    MFC after:      2 weeks
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D27724
---
 sys/dev/md/md.c | 65 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 32 insertions(+), 33 deletions(-)

diff --git a/sys/dev/md/md.c b/sys/dev/md/md.c
index 1d17603ffdfe..1778eda48f35 100644
--- a/sys/dev/md/md.c
+++ b/sys/dev/md/md.c
@@ -1012,11 +1012,11 @@ unmapped_step:
 				goto unmapped_step;
 		}
 		uma_zfree(md_pbuf_zone, pb);
+	} else {
+		bp->bio_resid = auio.uio_resid;
 	}
 
 	free(piov, M_MD);
-	if (pb == NULL)
-		bp->bio_resid = auio.uio_resid;
 	return (error);
 }
 
@@ -1184,6 +1184,23 @@ mdstart_null(struct md_s *sc, struct bio *bp)
 	return (0);
 }
 
+static void
+md_handleattr(struct md_s *sc, struct bio *bp)
+{
+	if (sc->fwsectors && sc->fwheads &&
+	    (g_handleattr_int(bp, "GEOM::fwsectors", sc->fwsectors) != 0 ||
+	    g_handleattr_int(bp, "GEOM::fwheads", sc->fwheads) != 0))
+		return;
+	if (g_handleattr_int(bp, "GEOM::candelete", 1) != 0)
+		return;
+	if (sc->ident[0] != '\0' &&
+	    g_handleattr_str(bp, "GEOM::ident", sc->ident) != 0)
+		return;
+	if (g_handleattr_int(bp, "MNT::verified", (sc->flags & MD_VERIFY) != 0))
+		return;
+	g_io_deliver(bp, EOPNOTSUPP);
+}
+
 static void
 md_kthread(void *arg)
 {
@@ -1212,39 +1229,21 @@ md_kthread(void *arg)
 		}
 		mtx_unlock(&sc->queue_mtx);
 		if (bp->bio_cmd == BIO_GETATTR) {
-			int isv = ((sc->flags & MD_VERIFY) != 0);
-
-			if ((sc->fwsectors && sc->fwheads &&
-			    (g_handleattr_int(bp, "GEOM::fwsectors",
-			    sc->fwsectors) ||
-			    g_handleattr_int(bp, "GEOM::fwheads",
-			    sc->fwheads))) ||
-			    g_handleattr_int(bp, "GEOM::candelete", 1))
-				error = -1;
-			else if (sc->ident[0] != '\0' &&
-			    g_handleattr_str(bp, "GEOM::ident", sc->ident))
-				error = -1;
-			else if (g_handleattr_int(bp, "MNT::verified", isv))
-				error = -1;
-			else
-				error = EOPNOTSUPP;
+			md_handleattr(sc, bp);
 		} else {
 			error = sc->start(sc, bp);
-		}
-
-		if (bp->bio_cmd == BIO_READ || bp->bio_cmd == BIO_WRITE) {
-			/*
-			 * Devstat uses (bio_bcount, bio_resid) for
-			 * determining the length of the completed part of
-			 * the i/o.  g_io_deliver() will translate from
-			 * bio_completed to that, but it also destroys the
-			 * bio so we must do our own translation.
-			 */
-			bp->bio_bcount = bp->bio_length;
-			bp->bio_resid = (error == -1 ? bp->bio_bcount : 0);
-			devstat_end_transaction_bio(sc->devstat, bp);
-		}
-		if (error != -1) {
+			if (bp->bio_cmd == BIO_READ || bp->bio_cmd == BIO_WRITE) {
+				/*
+				 * Devstat uses (bio_bcount, bio_resid) for
+				 * determining the length of the completed part
+				 * of the i/o.  g_io_deliver() will translate
+				 * from bio_completed to that, but it also
+				 * destroys the bio so we must do our own
+				 * translation.
+				 */
+				bp->bio_bcount = bp->bio_length;
+				devstat_end_transaction_bio(sc->devstat, bp);
+			}
 			bp->bio_completed = bp->bio_length;
 			g_io_deliver(bp, error);
 		}

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202012231617.0BNGHisT055146>