diff mbox series

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

Message ID 20200922092722.344518-1-ppandit@redhat.com
State New
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));