diff mbox series

fdc: check drive block device before usage (CVE-2021-20196)

Message ID 20210123100345.642933-1-ppandit@redhat.com (mailing list archive)
State New, archived
Headers show
Series fdc: check drive block device before usage (CVE-2021-20196) | expand

Commit Message

P J P Jan. 23, 2021, 10:03 a.m. UTC
From: Prasad J Pandit <pjp@fedoraproject.org>

While processing ioport command in 'fdctrl_write_dor', device
controller may select a drive which is not initialised with a
block device. This may result in a NULL pointer dereference.
Add checks to avoid it.

Fixes: CVE-2021-20196
Reported-by: Gaoning Pan <pgn@zju.edu.cn>
Buglink: https://bugs.launchpad.net/qemu/+bug/1912780
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/block/fdc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Alexander Bulekov Jan. 23, 2021, 5:47 p.m. UTC | #1
Hello,
Here is a reproducer for this bug found by the OSS-Fuzz fuzzer:

cat << EOF | qemu-system-i386 -display none -machine accel=qtest \
-m 512M -machine pc -device floppy,unit=1,id=floppy0,drive=disk0 \
-drive id=disk0,file=null-co://,file.read-zeroes=on,if=none,format=raw \
-qtest stdio
outw 0x3f4 0x2500
outb 0x3f5 0x81
outb 0x3f5 0x0
outb 0x3f5 0x0
outb 0x3f5 0x0
outw 0x3f4 0x0
outw 0x3f4 0x4000
outw 0x3f4 0x13
outb 0x3f5 0x1
outw 0x3f2 0x1405
outw 0x3f4 0x0
EOF

-Alex

On 210123 1533, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> While processing ioport command in 'fdctrl_write_dor', device
> controller may select a drive which is not initialised with a
> block device. This may result in a NULL pointer dereference.
> Add checks to avoid it.
> 
> Fixes: CVE-2021-20196
> Reported-by: Gaoning Pan <pgn@zju.edu.cn>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1912780
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/block/fdc.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 3636874432..13a9470d19 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1429,7 +1429,9 @@ static void fdctrl_write_dor(FDCtrl *fdctrl, uint32_t value)
>          }
>      }
>      /* Selected drive */
> -    fdctrl->cur_drv = value & FD_DOR_SELMASK;
> +    if (fdctrl->drives[value & FD_DOR_SELMASK].blk) {
> +        fdctrl->cur_drv = value & FD_DOR_SELMASK;
> +    }
>  
>      fdctrl->dor = value;
>  }
> @@ -1894,6 +1896,10 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>      uint32_t pos;
>  
>      cur_drv = get_cur_drv(fdctrl);
> +    if (!cur_drv->blk) {
> +        FLOPPY_DPRINTF("No drive connected\n");
> +        return 0;
> +    }
>      fdctrl->dsr &= ~FD_DSR_PWRDOWN;
>      if (!(fdctrl->msr & FD_MSR_RQM) || !(fdctrl->msr & FD_MSR_DIO)) {
>          FLOPPY_DPRINTF("error: controller not ready for reading\n");
> @@ -2420,7 +2426,8 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>          if (pos == FD_SECTOR_LEN - 1 ||
>              fdctrl->data_pos == fdctrl->data_len) {
>              cur_drv = get_cur_drv(fdctrl);
> -            if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> +            if (cur_drv->blk == NULL
> +                || blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
>                             BDRV_SECTOR_SIZE, 0) < 0) {
>                  FLOPPY_DPRINTF("error writing sector %d\n",
>                                 fd_sector(cur_drv));
> -- 
> 2.29.2
> 
>
Alexander Bulekov Jan. 23, 2021, 5:52 p.m. UTC | #2
On 210123 1247, Alexander Bulekov wrote:
> Hello,
> Here is a reproducer for this bug found by the OSS-Fuzz fuzzer:
> 
> cat << EOF | qemu-system-i386 -display none -machine accel=qtest \
> -m 512M -machine pc -device floppy,unit=1,id=floppy0,drive=disk0 \
> -drive id=disk0,file=null-co://,file.read-zeroes=on,if=none,format=raw \
> -qtest stdio
> outw 0x3f4 0x2500
> outb 0x3f5 0x81
> outb 0x3f5 0x0
> outb 0x3f5 0x0
> outb 0x3f5 0x0
> outw 0x3f4 0x0
> outw 0x3f4 0x4000
> outw 0x3f4 0x13
> outb 0x3f5 0x1
> outw 0x3f2 0x1405
> outw 0x3f4 0x0
> EOF
> 
Oops there's a mistake there. Fixed:

cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest \
-m 512M -machine pc -device floppy,unit=1,id=floppy0,drive=disk0 \
-drive id=disk0,file=null-co://,file.read-zeroes=on,if=none,format=raw \
-qtest stdio
outw 0x3f4 0x2500
outb 0x3f5 0x81
outb 0x3f5 0x0
outb 0x3f5 0x0
outb 0x3f5 0x0
outw 0x3f2 0x14
outw 0x3f4 0x0
outw 0x3f4 0x4000
outw 0x3f4 0x13
outb 0x3f5 0x1
outw 0x3f2 0x1405
outw 0x3f4 0x0
EOF

> -Alex
> 
> On 210123 1533, P J P wrote:
> > From: Prasad J Pandit <pjp@fedoraproject.org>
> > 
> > While processing ioport command in 'fdctrl_write_dor', device
> > controller may select a drive which is not initialised with a
> > block device. This may result in a NULL pointer dereference.
> > Add checks to avoid it.
> > 
> > Fixes: CVE-2021-20196
> > Reported-by: Gaoning Pan <pgn@zju.edu.cn>
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1912780
> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> > ---
> >  hw/block/fdc.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> > index 3636874432..13a9470d19 100644
> > --- a/hw/block/fdc.c
> > +++ b/hw/block/fdc.c
> > @@ -1429,7 +1429,9 @@ static void fdctrl_write_dor(FDCtrl *fdctrl, uint32_t value)
> >          }
> >      }
> >      /* Selected drive */
> > -    fdctrl->cur_drv = value & FD_DOR_SELMASK;
> > +    if (fdctrl->drives[value & FD_DOR_SELMASK].blk) {
> > +        fdctrl->cur_drv = value & FD_DOR_SELMASK;
> > +    }
> >  
> >      fdctrl->dor = value;
> >  }
> > @@ -1894,6 +1896,10 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
> >      uint32_t pos;
> >  
> >      cur_drv = get_cur_drv(fdctrl);
> > +    if (!cur_drv->blk) {
> > +        FLOPPY_DPRINTF("No drive connected\n");
> > +        return 0;
> > +    }
> >      fdctrl->dsr &= ~FD_DSR_PWRDOWN;
> >      if (!(fdctrl->msr & FD_MSR_RQM) || !(fdctrl->msr & FD_MSR_DIO)) {
> >          FLOPPY_DPRINTF("error: controller not ready for reading\n");
> > @@ -2420,7 +2426,8 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
> >          if (pos == FD_SECTOR_LEN - 1 ||
> >              fdctrl->data_pos == fdctrl->data_len) {
> >              cur_drv = get_cur_drv(fdctrl);
> > -            if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> > +            if (cur_drv->blk == NULL
> > +                || blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> >                             BDRV_SECTOR_SIZE, 0) < 0) {
> >                  FLOPPY_DPRINTF("error writing sector %d\n",
> >                                 fd_sector(cur_drv));
> > -- 
> > 2.29.2
> > 
> >
P J P Jan. 30, 2021, 1:35 p.m. UTC | #3
+-- On Sat, 23 Jan 2021, P J P wrote --+
| From: Prasad J Pandit <pjp@fedoraproject.org>
| 
| While processing ioport command in 'fdctrl_write_dor', device
| controller may select a drive which is not initialised with a
| block device. This may result in a NULL pointer dereference.
| Add checks to avoid it.
| 
| Fixes: CVE-2021-20196
| Reported-by: Gaoning Pan <pgn@zju.edu.cn>
| Buglink: https://bugs.launchpad.net/qemu/+bug/1912780
| Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
| ---
|  hw/block/fdc.c | 11 +++++++++--
|  1 file changed, 9 insertions(+), 2 deletions(-)
| 
| diff --git a/hw/block/fdc.c b/hw/block/fdc.c
| index 3636874432..13a9470d19 100644
| --- a/hw/block/fdc.c
| +++ b/hw/block/fdc.c
| @@ -1429,7 +1429,9 @@ static void fdctrl_write_dor(FDCtrl *fdctrl, uint32_t value)
|          }
|      }
|      /* Selected drive */
| -    fdctrl->cur_drv = value & FD_DOR_SELMASK;
| +    if (fdctrl->drives[value & FD_DOR_SELMASK].blk) {
| +        fdctrl->cur_drv = value & FD_DOR_SELMASK;
| +    }
|  
|      fdctrl->dor = value;
|  }
| @@ -1894,6 +1896,10 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
|      uint32_t pos;
|  
|      cur_drv = get_cur_drv(fdctrl);
| +    if (!cur_drv->blk) {
| +        FLOPPY_DPRINTF("No drive connected\n");
| +        return 0;
| +    }
|      fdctrl->dsr &= ~FD_DSR_PWRDOWN;
|      if (!(fdctrl->msr & FD_MSR_RQM) || !(fdctrl->msr & FD_MSR_DIO)) {
|          FLOPPY_DPRINTF("error: controller not ready for reading\n");
| @@ -2420,7 +2426,8 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
|          if (pos == FD_SECTOR_LEN - 1 ||
|              fdctrl->data_pos == fdctrl->data_len) {
|              cur_drv = get_cur_drv(fdctrl);
| -            if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
| +            if (cur_drv->blk == NULL
| +                || blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
|                             BDRV_SECTOR_SIZE, 0) < 0) {
|                  FLOPPY_DPRINTF("error writing sector %d\n",
|                                 fd_sector(cur_drv));
| 


Ping..!
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
diff mbox series

Patch

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 3636874432..13a9470d19 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1429,7 +1429,9 @@  static void fdctrl_write_dor(FDCtrl *fdctrl, uint32_t value)
         }
     }
     /* Selected drive */
-    fdctrl->cur_drv = value & FD_DOR_SELMASK;
+    if (fdctrl->drives[value & FD_DOR_SELMASK].blk) {
+        fdctrl->cur_drv = value & FD_DOR_SELMASK;
+    }
 
     fdctrl->dor = value;
 }
@@ -1894,6 +1896,10 @@  static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
     uint32_t pos;
 
     cur_drv = get_cur_drv(fdctrl);
+    if (!cur_drv->blk) {
+        FLOPPY_DPRINTF("No drive connected\n");
+        return 0;
+    }
     fdctrl->dsr &= ~FD_DSR_PWRDOWN;
     if (!(fdctrl->msr & FD_MSR_RQM) || !(fdctrl->msr & FD_MSR_DIO)) {
         FLOPPY_DPRINTF("error: controller not ready for reading\n");
@@ -2420,7 +2426,8 @@  static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
         if (pos == FD_SECTOR_LEN - 1 ||
             fdctrl->data_pos == fdctrl->data_len) {
             cur_drv = get_cur_drv(fdctrl);
-            if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
+            if (cur_drv->blk == NULL
+                || blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
                            BDRV_SECTOR_SIZE, 0) < 0) {
                 FLOPPY_DPRINTF("error writing sector %d\n",
                                fd_sector(cur_drv));