diff mbox series

[v2] fdc: check null block pointer before r/w data transfer

Message ID 20200922092722.344518-1-ppandit@redhat.com (mailing list archive)
State New, archived
Headers show
Series [v2] fdc: check null block pointer before r/w data transfer | expand

Commit Message

Prasad Pandit Sept. 22, 2020, 9:27 a.m. UTC 
From: Prasad J Pandit <pjp@fedoraproject.org>

While transferring data via fdctrl_read/write_data() routines,
check that current drive does not have a null block pointer.
Avoid null pointer dereference.

 -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Ffdc_nullptr1
    ==1658854==Hint: address points to the zero page.
    #0 blk_inc_in_flight block/block-backend.c:1327
    #1 blk_prw block/block-backend.c:1299
    #2 blk_pwrite block/block-backend.c:1464
    #3 fdctrl_write_data hw/block/fdc.c:2418
    #4 fdctrl_write hw/block/fdc.c:962
    #5 portio_write ioport.c:205
    #6 memory_region_write_accessor memory.c:483
    #7 access_with_adjusted_size memory.c:544
    #8 memory_region_dispatch_write memory.c:1476

Reported-by: Ruhr-University <bugs-syssec@rub.de>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/block/fdc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Update v2: treat NULL blk pointer as an error
  -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg06642.html

--
2.26.2

Comments

Li Qiang Sept. 22, 2020, 10:42 a.m. UTC | #1 
P J P <ppandit@redhat.com> 于2020年9月22日周二 下午5:29写道：
>
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While transferring data via fdctrl_read/write_data() routines,
> check that current drive does not have a null block pointer.
> Avoid null pointer dereference.
>
>  -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Ffdc_nullptr1
>     ==1658854==Hint: address points to the zero page.
>     #0 blk_inc_in_flight block/block-backend.c:1327
>     #1 blk_prw block/block-backend.c:1299
>     #2 blk_pwrite block/block-backend.c:1464
>     #3 fdctrl_write_data hw/block/fdc.c:2418
>     #4 fdctrl_write hw/block/fdc.c:962
>     #5 portio_write ioport.c:205
>     #6 memory_region_write_accessor memory.c:483
>     #7 access_with_adjusted_size memory.c:544
>     #8 memory_region_dispatch_write memory.c:1476
>
> Reported-by: Ruhr-University <bugs-syssec@rub.de>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>  hw/block/fdc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> Update v2: treat NULL blk pointer as an error
>   -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg06642.html
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 224bac504f..bf968dc66f 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1923,7 +1923,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>                                     fd_sector(cur_drv));
>                      return 0;
>                  }
> -            if (blk_pread(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> +            if (!cur_drv->blk
> +                || blk_pread(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
>                            BDRV_SECTOR_SIZE)
>                  < 0) {
>                  FLOPPY_DPRINTF("error getting sector %d\n",
> @@ -2427,7 +2428,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
> +                || 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.26.2
>
John Snow Oct. 1, 2020, 3:38 p.m. UTC | #2 
On 9/22/20 5:27 AM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> While transferring data via fdctrl_read/write_data() routines,
> check that current drive does not have a null block pointer.
> Avoid null pointer dereference.
> 

Will get to these and other IDE issues ASAP.

--js
John Snow May 18, 2021, 5:32 p.m. UTC | #3 
On 9/22/20 5:27 AM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> While transferring data via fdctrl_read/write_data() routines,
> check that current drive does not have a null block pointer.
> Avoid null pointer dereference.
> 
>   -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Ffdc_nullptr1
>      ==1658854==Hint: address points to the zero page.
>      #0 blk_inc_in_flight block/block-backend.c:1327
>      #1 blk_prw block/block-backend.c:1299
>      #2 blk_pwrite block/block-backend.c:1464
>      #3 fdctrl_write_data hw/block/fdc.c:2418
>      #4 fdctrl_write hw/block/fdc.c:962
>      #5 portio_write ioport.c:205
>      #6 memory_region_write_accessor memory.c:483
>      #7 access_with_adjusted_size memory.c:544
>      #8 memory_region_dispatch_write memory.c:1476
> 
> Reported-by: Ruhr-University <bugs-syssec@rub.de>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>   hw/block/fdc.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Update v2: treat NULL blk pointer as an error
>    -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg06642.html
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 224bac504f..bf968dc66f 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1923,7 +1923,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>                                      fd_sector(cur_drv));
>                       return 0;
>                   }
> -            if (blk_pread(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> +            if (!cur_drv->blk
> +                || blk_pread(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
>                             BDRV_SECTOR_SIZE)
>                   < 0) {
>                   FLOPPY_DPRINTF("error getting sector %d\n",
> @@ -2427,7 +2428,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
> +                || 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.26.2
> 

This patch actually looks good to me. Though again, I assume it can be 
rolled into the most recent issue that actually grabbed my attention:

https://gitlab.com/qemu-project/qemu/-/issues/338

And we can credit both reporters (and Alexander) and solve all of these 
issues all at once.

I am therefore dropping this patch from my queue. Please let me know if 
I am mistaken.

Thanks,
--js
Prasad Pandit May 19, 2021, 7:27 a.m. UTC | #4 
+-- On Tue, 18 May 2021, John Snow wrote --+
| I assume it can be rolled into the most recent issue that actually grabbed 
| my attention:
|
|  -> https://gitlab.com/qemu-project/qemu/-/issues/338
| 
| And we can credit both reporters (and Alexander) and solve all of these issues
| all at once.
| 
| I am therefore dropping this patch from my queue. Please let me know if I am
| mistaken.

* It should be okay to collate patches together under above gitlab issue as a 
  series.
 
  Considering they've individual CVEs assigned, we'll still need to tag them 
  with CVEs, so that it's easier for downstream users to pick them.


Thank you.
--
 - P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Salvatore Bonaccorso Sept. 3, 2021, 12:56 p.m. UTC | #5 
On Tue, Sep 22, 2020 at 06:42:22PM +0800, Li Qiang wrote:
> P J P <ppandit@redhat.com> 于2020年9月22日周二 下午5:29写道：
> >
> > From: Prasad J Pandit <pjp@fedoraproject.org>
> >
> > While transferring data via fdctrl_read/write_data() routines,
> > check that current drive does not have a null block pointer.
> > Avoid null pointer dereference.
> >
> >  -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Ffdc_nullptr1
> >     ==1658854==Hint: address points to the zero page.
> >     #0 blk_inc_in_flight block/block-backend.c:1327
> >     #1 blk_prw block/block-backend.c:1299
> >     #2 blk_pwrite block/block-backend.c:1464
> >     #3 fdctrl_write_data hw/block/fdc.c:2418
> >     #4 fdctrl_write hw/block/fdc.c:962
> >     #5 portio_write ioport.c:205
> >     #6 memory_region_write_accessor memory.c:483
> >     #7 access_with_adjusted_size memory.c:544
> >     #8 memory_region_dispatch_write memory.c:1476
> >
> > Reported-by: Ruhr-University <bugs-syssec@rub.de>
> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> 
> Reviewed-by: Li Qiang <liq3ea@gmail.com>

Did this one just felt through the cracks, or was it not further
considered?

Regards,
Salvatore
diff mbox series

Patch

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 224bac504f..bf968dc66f 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1923,7 +1923,8 @@  static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
                                    fd_sector(cur_drv));
                     return 0;
                 }
-            if (blk_pread(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
+            if (!cur_drv->blk
+                || blk_pread(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
                           BDRV_SECTOR_SIZE)
                 < 0) {
                 FLOPPY_DPRINTF("error getting sector %d\n",
@@ -2427,7 +2428,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
+                || 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));