Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/sys/dev fix use-after-free for ata xfer on bio submission fo...



details:   https://anonhg.NetBSD.org/src/rev/65dbf3b670cb
branches:  trunk
changeset: 1009121:65dbf3b670cb
user:      jdolecek <jdolecek%NetBSD.org@localhost>
date:      Mon Apr 13 10:49:34 2020 +0000

description:
fix use-after-free for ata xfer on bio submission found by KASAN

driver ata_bio hooks read parts of the xfer after ata_exec_xfer()
call in order to determine return value, change so that the hook
doesn't return any value - callers do not care already,
as all I/O requests are asynchronous

this problem was uncovered by recent change for wd(4) to not hold
wd mutex during ata_bio call, the interrupt for the xfer might
thus actually fire immediately

adjust also ata_exec_command driver hooks similarily - remove all
completion and waiting logic from drivers, upper layer ata code
using AT_WAIT/AT_POLL changed to call ata_wait_cmd() itself

PR kern/55169 by Nick Hudson

diffstat:

 sys/dev/ata/ata.c          |  20 +++-------
 sys/dev/ata/ata_recovery.c |  12 ++---
 sys/dev/ata/ata_wdc.c      |  12 ++---
 sys/dev/ata/atavar.h       |   6 +-
 sys/dev/ata/satapmp_subr.c |  26 +++++--------
 sys/dev/ata/wd.c           |  92 +++++++++++++++------------------------------
 sys/dev/ic/ahcisata_core.c |  34 +++-------------
 sys/dev/ic/mvsata.c        |  32 +++------------
 sys/dev/ic/siisata.c       |  38 ++++---------------
 sys/dev/ic/wdc.c           |  25 +-----------
 sys/dev/ic/wdcvar.h        |   4 +-
 sys/dev/scsipi/atapi_wdc.c |  14 +++----
 12 files changed, 93 insertions(+), 222 deletions(-)

diffs (truncated from 804 to 300 lines):

diff -r 3f756fb56bb8 -r 65dbf3b670cb sys/dev/ata/ata.c
--- a/sys/dev/ata/ata.c Mon Apr 13 09:34:02 2020 +0000
+++ b/sys/dev/ata/ata.c Mon Apr 13 10:49:34 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ata.c,v 1.154 2020/04/04 21:36:15 jdolecek Exp $       */
+/*     $NetBSD: ata.c,v 1.155 2020/04/13 10:49:34 jdolecek Exp $       */
 
 /*
  * Copyright (c) 1998, 2001 Manuel Bouyer.  All rights reserved.
@@ -25,7 +25,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ata.c,v 1.154 2020/04/04 21:36:15 jdolecek Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ata.c,v 1.155 2020/04/13 10:49:34 jdolecek Exp $");
 
 #include "opt_ata.h"
 
@@ -847,13 +847,8 @@
        xfer->c_ata_c.flags = AT_READ | flags;
        xfer->c_ata_c.data = tb;
        xfer->c_ata_c.bcount = ATA_BSIZE;
-       if ((*atac->atac_bustype_ata->ata_exec_command)(drvp,
-                                               xfer) != ATACMD_COMPLETE) {
-               ATADEBUG_PRINT(("ata_get_parms: wdc_exec_command failed\n"),
-                   DEBUG_FUNCS|DEBUG_PROBE);
-               rv = CMD_AGAIN;
-               goto out;
-       }
+       (*atac->atac_bustype_ata->ata_exec_command)(drvp, xfer);
+       ata_wait_cmd(chp, xfer);
        if (xfer->c_ata_c.flags & (AT_ERROR | AT_TIMEOU | AT_DF)) {
                ATADEBUG_PRINT(("ata_get_parms: ata_c.flags=0x%x\n",
                    xfer->c_ata_c.flags), DEBUG_FUNCS|DEBUG_PROBE);
@@ -937,11 +932,8 @@
        xfer->c_ata_c.r_count = mode;
        xfer->c_ata_c.flags = flags;
        xfer->c_ata_c.timeout = 1000; /* 1s */
-       if ((*atac->atac_bustype_ata->ata_exec_command)(drvp,
-                                               xfer) != ATACMD_COMPLETE) {
-               rv = CMD_AGAIN;
-               goto out;
-       }
+       (*atac->atac_bustype_ata->ata_exec_command)(drvp, xfer);
+       ata_wait_cmd(chp, xfer);
        if (xfer->c_ata_c.flags & (AT_ERROR | AT_TIMEOU | AT_DF)) {
                rv = CMD_ERR;
                goto out;
diff -r 3f756fb56bb8 -r 65dbf3b670cb sys/dev/ata/ata_recovery.c
--- a/sys/dev/ata/ata_recovery.c        Mon Apr 13 09:34:02 2020 +0000
+++ b/sys/dev/ata/ata_recovery.c        Mon Apr 13 10:49:34 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ata_recovery.c,v 1.3 2020/04/04 22:30:02 jdolecek Exp $        */
+/*     $NetBSD: ata_recovery.c,v 1.4 2020/04/13 10:49:34 jdolecek Exp $        */
 
 /*-
  * Copyright (c) 2018 The NetBSD Foundation, Inc.
@@ -27,7 +27,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ata_recovery.c,v 1.3 2020/04/04 22:30:02 jdolecek Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ata_recovery.c,v 1.4 2020/04/13 10:49:34 jdolecek Exp $");
 
 #include "opt_ata.h"
 
@@ -103,11 +103,9 @@
        xfer->c_ata_c.data = tb;
        xfer->c_ata_c.bcount = sizeof(chp->recovery_blk);
 
-       if ((*atac->atac_bustype_ata->ata_exec_command)(drvp,
-                                               xfer) != ATACMD_COMPLETE) {
-               rv = EAGAIN;
-               goto out;
-       }
+       (*atac->atac_bustype_ata->ata_exec_command)(drvp, xfer);
+       ata_wait_cmd(chp, xfer);
+
        if (xfer->c_ata_c.flags & (AT_ERROR | AT_TIMEOU | AT_DF)) {
                rv = EINVAL;
                goto out;
diff -r 3f756fb56bb8 -r 65dbf3b670cb sys/dev/ata/ata_wdc.c
--- a/sys/dev/ata/ata_wdc.c     Mon Apr 13 09:34:02 2020 +0000
+++ b/sys/dev/ata/ata_wdc.c     Mon Apr 13 10:49:34 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ata_wdc.c,v 1.114 2020/04/04 21:36:15 jdolecek Exp $   */
+/*     $NetBSD: ata_wdc.c,v 1.115 2020/04/13 10:49:34 jdolecek Exp $   */
 
 /*
  * Copyright (c) 1998, 2001, 2003 Manuel Bouyer.
@@ -54,7 +54,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ata_wdc.c,v 1.114 2020/04/04 21:36:15 jdolecek Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ata_wdc.c,v 1.115 2020/04/13 10:49:34 jdolecek Exp $");
 
 #include "opt_ata.h"
 #include "opt_wdc.h"
@@ -102,7 +102,7 @@
 
 #define ATA_DELAY 10000 /* 10s for a drive I/O */
 
-static int     wdc_ata_bio(struct ata_drive_datas*, struct ata_xfer *);
+static void    wdc_ata_bio(struct ata_drive_datas*, struct ata_xfer *);
 static int     wdc_ata_bio_start(struct ata_channel *,struct ata_xfer *);
 static int     _wdc_ata_bio_start(struct ata_channel *,struct ata_xfer *);
 static void    wdc_ata_bio_poll(struct ata_channel *,struct ata_xfer *);
@@ -140,10 +140,9 @@
 };
 
 /*
- * Handle block I/O operation. Return ATACMD_COMPLETE, ATACMD_QUEUED, or
- * ATACMD_TRY_AGAIN. Must be called at splbio().
+ * Handle block I/O operation.
  */
-static int
+static void
 wdc_ata_bio(struct ata_drive_datas *drvp, struct ata_xfer *xfer)
 {
        struct ata_channel *chp = drvp->chnl_softc;
@@ -171,7 +170,6 @@
        xfer->c_bcount = ata_bio->bcount;
        xfer->ops = &wdc_bio_xfer_ops;
        ata_exec_xfer(chp, xfer);
-       return (ata_bio->flags & ATA_ITSDONE) ? ATACMD_COMPLETE : ATACMD_QUEUED;
 }
 
 static int
diff -r 3f756fb56bb8 -r 65dbf3b670cb sys/dev/ata/atavar.h
--- a/sys/dev/ata/atavar.h      Mon Apr 13 09:34:02 2020 +0000
+++ b/sys/dev/ata/atavar.h      Mon Apr 13 10:49:34 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: atavar.h,v 1.104 2020/04/04 21:36:15 jdolecek Exp $    */
+/*     $NetBSD: atavar.h,v 1.105 2020/04/13 10:49:34 jdolecek Exp $    */
 
 /*
  * Copyright (c) 1998, 2001 Manuel Bouyer.
@@ -358,10 +358,10 @@
  */
 struct ata_bustype {
        int     bustype_type;   /* symbolic name of type */
-       int     (*ata_bio)(struct ata_drive_datas *, struct ata_xfer *);
+       void    (*ata_bio)(struct ata_drive_datas *, struct ata_xfer *);
        void    (*ata_reset_drive)(struct ata_drive_datas *, int, uint32_t *);
        void    (*ata_reset_channel)(struct ata_channel *, int);
-       int     (*ata_exec_command)(struct ata_drive_datas *,
+       void    (*ata_exec_command)(struct ata_drive_datas *,
                                    struct ata_xfer *);
 
 #define        ATACMD_COMPLETE         0x01
diff -r 3f756fb56bb8 -r 65dbf3b670cb sys/dev/ata/satapmp_subr.c
--- a/sys/dev/ata/satapmp_subr.c        Mon Apr 13 09:34:02 2020 +0000
+++ b/sys/dev/ata/satapmp_subr.c        Mon Apr 13 10:49:34 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: satapmp_subr.c,v 1.15 2018/10/22 20:13:47 jdolecek Exp $       */
+/*     $NetBSD: satapmp_subr.c,v 1.16 2020/04/13 10:49:34 jdolecek Exp $       */
 
 /*
  * Copyright (c) 2012 Manuel Bouyer.  All rights reserved.
@@ -25,7 +25,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: satapmp_subr.c,v 1.15 2018/10/22 20:13:47 jdolecek Exp $");
+__KERNEL_RCSID(0, "$NetBSD: satapmp_subr.c,v 1.16 2020/04/13 10:49:34 jdolecek Exp $");
 
 #include <sys/param.h>
 #include <sys/systm.h>
@@ -72,13 +72,10 @@
        xfer->c_ata_c.flags = AT_LBA48 | AT_READREG | AT_WAIT;
 
        ata_channel_unlock(chp);
-       if ((*atac->atac_bustype_ata->ata_exec_command)(drvp,
-           xfer) != ATACMD_COMPLETE) {
-               aprint_error_dev(chp->atabus,
-                   "PMP port %d register %d read failed\n", port, reg);
-               error = EIO;
-               goto out;
-       }
+
+       (*atac->atac_bustype_ata->ata_exec_command)(drvp, xfer);
+       ata_wait_cmd(chp, xfer);
+
        if (xfer->c_ata_c.flags & (AT_TIMEOU | AT_DF)) {
                aprint_error_dev(chp->atabus,
                    "PMP port %d register %d read failed, flags 0x%x\n",
@@ -148,13 +145,10 @@
        xfer->c_ata_c.flags = AT_LBA48 | AT_WAIT;
 
        ata_channel_unlock(chp);
-       if ((*atac->atac_bustype_ata->ata_exec_command)(drvp,
-           xfer) != ATACMD_COMPLETE) {
-               aprint_error_dev(chp->atabus,
-                   "PMP port %d register %d write failed\n", port, reg);
-               error = EIO;
-               goto out;
-       }
+
+       (*atac->atac_bustype_ata->ata_exec_command)(drvp, xfer);
+       ata_wait_cmd(chp, xfer);
+
        if (xfer->c_ata_c.flags & (AT_TIMEOU | AT_DF)) {
                aprint_error_dev(chp->atabus,
                    "PMP port %d register %d write failed, flags 0x%x\n",
diff -r 3f756fb56bb8 -r 65dbf3b670cb sys/dev/ata/wd.c
--- a/sys/dev/ata/wd.c  Mon Apr 13 09:34:02 2020 +0000
+++ b/sys/dev/ata/wd.c  Mon Apr 13 10:49:34 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: wd.c,v 1.461 2020/04/13 08:05:02 maxv Exp $ */
+/*     $NetBSD: wd.c,v 1.462 2020/04/13 10:49:34 jdolecek Exp $ */
 
 /*
  * Copyright (c) 1998, 2001 Manuel Bouyer.  All rights reserved.
@@ -54,7 +54,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: wd.c,v 1.461 2020/04/13 08:05:02 maxv Exp $");
+__KERNEL_RCSID(0, "$NetBSD: wd.c,v 1.462 2020/04/13 10:49:34 jdolecek Exp $");
 
 #include "opt_ata.h"
 #include "opt_wd.h"
@@ -201,7 +201,7 @@
 static void    wdbioretry(void *);
 static void    wdbiorequeue(void *);
 static void    wddone(device_t, struct ata_xfer *);
-static int     wd_get_params(struct wd_softc *, uint8_t, struct ataparams *);
+static int     wd_get_params(struct wd_softc *, struct ataparams *);
 static void    wd_set_geometry(struct wd_softc *);
 static int     wd_flushcache(struct wd_softc *, int, bool);
 static int     wd_trim(struct wd_softc *, daddr_t, long);
@@ -336,7 +336,7 @@
        aprint_normal("\n");
 
        /* read our drive info */
-       if (wd_get_params(wd, AT_WAIT, &wd->sc_params) != 0) {
+       if (wd_get_params(wd, &wd->sc_params) != 0) {
                aprint_error_dev(self, "IDENTIFY failed\n");
                goto out;
        }
@@ -760,16 +760,8 @@
                wd->inflight++;
        mutex_exit(&wd->sc_lock);
 
-       switch (wd->atabus->ata_bio(wd->drvp, xfer)) {
-       case ATACMD_TRY_AGAIN:
-               panic("wdstart1: try again");
-               break;
-       case ATACMD_QUEUED:
-       case ATACMD_COMPLETE:
-               break;
-       default:
-               panic("wdstart1: bad return code from ata_bio()");
-       }
+       /* Queue the xfer */
+       wd->atabus->ata_bio(wd->drvp, xfer);
 
        mutex_enter(&wd->sc_lock);
 }
@@ -1165,7 +1157,7 @@
                int param_error;
 
                /* Load the physical device parameters. */
-               param_error = wd_get_params(wd, AT_WAIT, &wd->sc_params);
+               param_error = wd_get_params(wd, &wd->sc_params);
                if (param_error != 0) {
                        aprint_error_dev(dksc->sc_dev, "IDENTIFY failed\n");
                        error = EIO;
@@ -1609,18 +1601,9 @@
        xfer->c_bio.bcount = nblk * dg->dg_secsize;
        xfer->c_bio.databuf = va;
 #ifndef WD_DUMP_NOT_TRUSTED
-       switch (err = wd->atabus->ata_bio(wd->drvp, xfer)) {
-       case ATACMD_TRY_AGAIN:
-               panic("wddump: try again");
-               break;
-       case ATACMD_QUEUED:
-               panic("wddump: polled command has been queued");
-               break;
-       case ATACMD_COMPLETE:
-               break;
-       default:
-               panic("wddump: unknown atacmd code %d", err);
-       }
+       /* This will poll until the bio is complete */
+       wd->atabus->ata_bio(wd->drvp, xfer);
+
        switch(err = xfer->c_bio.error) {
        case TIMEOUT:
                printf("wddump: device timed out");
@@ -1707,10 +1690,11 @@
 }
 
 int
-wd_get_params(struct wd_softc *wd, uint8_t flags, struct ataparams *params)
+wd_get_params(struct wd_softc *wd, struct ataparams *params)
 {
        int retry = 0;
        struct ata_channel *chp = wd->drvp->chnl_softc;



Home | Main Index | Thread Index | Old Index