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

[Salvatore Bonaccorso]

On Tue, Sep 22, 2020 at 06:42:22PM +0800, Li Qiang wrote:
> P J P  于2020年9月22日周二 下午5:29写道：
> >
> > From: Prasad J Pandit 
> >
> > 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 
> > Signed-off-by: Prasad J Pandit 
> 
> Reviewed-by: Li Qiang 

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

Regards,
Salvatore

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

[P J P]

+-- 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

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

[John Snow]

On 9/22/20 5:27 AM, P J P wrote:

From: Prasad J Pandit 

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 
Signed-off-by: Prasad J Pandit 
---
  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

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

[John Snow]

On 9/22/20 5:27 AM, P J P wrote:

From: Prasad J Pandit 

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

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

[Li Qiang]

P J P  于2020年9月22日周二 下午5:29写道：
>
> From: Prasad J Pandit 
>
> 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 
> Signed-off-by: Prasad J Pandit 

Reviewed-by: Li Qiang 

> ---
>  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
>

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

[P J P]

From: Prasad J Pandit 

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 
Signed-off-by: Prasad J Pandit 
---
 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