From owner-dev-commits-src-all@freebsd.org Wed Dec 23 16:17:44 2020 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 462084C3AB1; Wed, 23 Dec 2020 16:17:44 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4D1JJm1T1qz3MgQ; Wed, 23 Dec 2020 16:17:44 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 2168121122; Wed, 23 Dec 2020 16:17:44 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 0BNGHi6K055147; Wed, 23 Dec 2020 16:17:44 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 0BNGHisT055146; Wed, 23 Dec 2020 16:17:44 GMT (envelope-from git) Date: Wed, 23 Dec 2020 16:17:44 GMT Message-Id: <202012231617.0BNGHisT055146@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Mark Johnston Subject: git: a7a7c306bfb0 - md: Fix a read-after-free in BIO_GETATTR handling MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: markj X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: a7a7c306bfb0d8d1a83569a098cf6cde492f8bf7 Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: "Commit messages for all branches of the src repository." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Dec 2020 16:17:44 -0000 The branch main has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=a7a7c306bfb0d8d1a83569a098cf6cde492f8bf7 commit a7a7c306bfb0d8d1a83569a098cf6cde492f8bf7 Author: Mark Johnston AuthorDate: 2020-12-23 16:13:31 +0000 Commit: Mark Johnston 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); }