Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[P J P]

On Tuesday, 14 September, 2021, 07:00:27 pm IST, P J P  
wrote:
>* Thanks so much for restarting this thread. I've been at it intermittently 
>last few
> months, thinking about how could we annotate the source/module objects.
>
> -> [*] https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04642.html
>
>* Last time we discussed about having both compile and run time options for 
>users
> to be able to select the qualified objects/backends/devices as desired.
>
>* To confirm: How/Where is the security policy defined? Is it device/module 
>specific OR QEMU project wide?
>
>>> it feels like we need
>> 'secure': 'bool'
>
>* Though we started the (above [*]) discussion with '--security' option in 
>mind,
>  I wonder if 'secure' annotation is much specific. And if we could widen its 
> scope.
>
>
>Source annotations: I've been thinking over following approaches
>===
>
>1) Segregate the QEMU sources under
>
>  ../staging/ <= devel/experimental, not for production usage
>  ../src/ <= good for production usage, hence security relevant
>  ../deprecated/ <= Bad for production usage, not security relevant
>
>  - This is similar to Linux staging drivers' tree.
>  - Staging drivers are not considered for production usage and hence CVE 
> allocation.
>  - At build time by default we only build sources under ../src/ tree.
>  - But we can still have options to build /staging/ and /deprecated/ trees.
>  - It's readily understandable to end users.
>
>2) pkgconfig(1) way:
>  - If we could define per device/backend a configuration (.pc) file which is 
> then used
>  at build/run time to decide which sources are suitable for the build or 
> usage.
>
>  - I'm trying to experiment with this.
>
>3) We annotate QEMU devices/backends/modules with macros which define its 
>status.
>  It can then be used at build/run times to decide if it's suitable for usage.
>  For ex:
>
>  $ cat annotsrc.h
>
>  #include 
>
>  enum SRCSTATUS {
>  DEVEL = 0,
>  STAGING,
>  PRODUCTION,
>  DEPRECATED
>  };
>
...
>
>
>* Approach 3) above is similar to the _security_policy_taint() API,
>  but works at the source/object file level, rather than specific 'struct 
> type' field.
> 
>* Does adding a field to struct type (ex. DeviceClass) scale to all 
>objects/modules/backends etc?
>  Does it have any limitations to include/cover other sources/objects?
>
>* I'd really appreciate your feedback/inputs/suggestions.


Ping...!?
---
  -P J P
http://feedmug.com

Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[P J P]

Hello Philippe, all

>On Thursday, 9 September, 2021, 03:58:40 pm IST, Daniel P. Berrangé 
> wrote:
>On Thu, Sep 09, 2021 at 01:20:14AM +0200, Philippe Mathieu-Daudé wrote:
>> This series is experimental! The goal is to better limit the
>> boundary of what code is considerated security critical, and
>> what is less critical (but still important!).
>>
>> This approach was quickly discussed few months ago with Markus
>> then Daniel. Instead of classifying the code on a file path
>> basis (see [1]), we insert (runtime) hints into the code
>> (which survive code movement). Offending unsafe code can
>> taint the global security policy. By default this policy
>> is 'none': the current behavior. It can be changed on the
>> command line to 'warn' to display warnings, and to 'strict'
>> to prohibit QEMU running with a tainted policy.
>

* Thanks so much for restarting this thread. I've been at it intermittently 
last few
  months, thinking about how could we annotate the source/module objects.

   -> [*] https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04642.html

* Last time we discussed about having both compile and run time options for 
users
  to be able to select the qualified objects/backends/devices as desired.

* To confirm: How/Where is the security policy defined? Is it device/module 
specific OR QEMU project wide?

> IOW, the reporting via QAPI introspetion is much more important
> for libvirt and mgmt apps, than any custom cli arg / printfs
> at the QEMU level.
>

* True, while it makes sense to have a solution that is conversant with
  the management/libvirt layers, It'll be great if we could address qemu/cli
  other use cases too.

>it feels like we need
>  'secure': 'bool'

* Though we started the (above [*]) discussion with '--security' option in mind,
  I wonder if 'secure' annotation is much specific. And if we could widen its 
scope.
--- x ---


Source annotations: I've been thinking over following approaches
===

1) Segregate the QEMU sources under

      ../staging/      <= devel/experimental, not for production usage
      ../src/          <= good for production usage, hence security relevant
      ../deprecated/   <= Bad for production usage, not security relevant 

   - This is similar to Linux staging drivers' tree.
   - Staging drivers are not considered for production usage and hence CVE 
allocation.
   - At build time by default we only build sources under ../src/ tree.
   - But we can still have options to build /staging/ and /deprecated/ trees.  
   - It's readily understandable to end users.

2) pkgconfig(1) way:
   - If we could define per device/backend a configuration (.pc) file which is 
then used
     at build/run time to decide which sources are suitable for the build or 
usage.

   - I'm trying to experiment with this.

3) We annotate QEMU devices/backends/modules with macros which define its 
status.
   It can then be used at build/run times to decide if it's suitable for usage.
   For ex:

    $ cat annotsrc.h

    #include 

    enum SRCSTATUS {
        DEVEL = 0,
        STAGING,
        PRODUCTION,
        DEPRECATED
    };

    uint8_t get_src_status(void);
    ===

    $ cat libx.c

    #include 

    #define SRC_STATUS DEPRECATED

    uint8_t
    get_src_status(void)
    {
        return SRC_STATUS;
    }
    ===

    $ cat testlibx.c

    #include 
    #include 

    int
    main (int argc, char *argv[])
    {
        printf("LibX status: %d\n", get_src_status());
        return 0;
    }
--- x ---



* Approach 3) above is similar to the _security_policy_taint() API,
  but works at the source/object file level, rather than specific 'struct type' 
field.
  

* Does adding a field to struct type (ex. DeviceClass) scale to all 
objects/modules/backends etc?
  Does it have any limitations to include/cover other sources/objects?


* I'd really appreciate any feedback/inputs/suggestions you may have.


Thank you.
---
  -P J P
http://feedmug.com

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] fdc: check drive block device before usage (CVE-2021-20196)

[P J P]

+-- On Sat, 15 May 2021, Philippe Mathieu-Daudé wrote --+
| This patch misses the qtest companion with the reproducer
| provided by Alexander.

Do we need a revised patch[-series] including a qtest? OR it can be done at 
merge time?

Thank you.
--
 - P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync

[P J P]

+-- On Tue, 29 Sep 2020, Li Qiang wrote --+
| P J P  于2020年9月29日周二 下午2:22写道：
| > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+
| > | P J P  于2020年9月18日周五 下午6:26写道：
| > | > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+
| > | > | Update v2: use an assert() call
| > | > |   
->https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html
| > |
| > | In 'ide_ioport_write' the guest can set 'bus->unit' to 0 or 1 by issue
| > | 'ATA_IOPORT_WR_DEVICE_HEAD'. So this case the guest can set the active 
ifs.
| > | If the guest set this to 1.
| > |
| > | Then in 'idebus_active_if' will return 'IDEBus.ifs[1]' and thus the 
's->blk'
| > | will be NULL.
| >
| > Right, guest does select the drive via
| >
| >   portio_write
| >->ide_ioport_write
| >   case ATA_IOPORT_WR_DEVICE_HEAD:
| >   /* FIXME: HOB readback uses bit 7 */
| >   bus->ifs[0].select = (val & ~0x10) | 0xa0;
| >   bus->ifs[1].select = (val | 0x10) | 0xa0;
| >   /* select drive */
| >   bus->unit = (val >> 4) & 1; <== set bus->unit=0x1
| >   break;
| >
| >
| > | So from your (Peter's) saying, we need to check the value in
| > | 'ATA_IOPORT_WR_DEVICE_HEAD' handler. To say if the guest
| > | set a valid 'bus->unit'. This can also work I think.
| >
| > Yes, with the following fix, an assert(3) in ide_cancel_dma_sync fails.
| >
| > ===
| > diff --git a/hw/ide/core.c b/hw/ide/core.c
| > index f76f7e5234..cb55cc8b0f 100644
| > --- a/hw/ide/core.c
| > +++ b/hw/ide/core.c
| > @@ -1300,7 +1300,11 @@ void ide_ioport_write(void *opaque, uint32_t addr,
| > uint_)
| >  bus->ifs[0].select = (val & ~0x10) | 0xa0;
| >  bus->ifs[1].select = (val | 0x10) | 0xa0;
| >  /* select drive */
| > +uint8_t bu = bus->unit;
| >  bus->unit = (val >> 4) & 1;
| > +if (!bus->ifs[bus->unit].blk) {
| > +bus->unit = bu;
| > +}
| >  break;
| >  default:
| >
| > qemu-system-x86_64: ../hw/ide/core.c:724: ide_cancel_dma_sync: Assertion 
`s->bus->dma->aiocb == NULL' failed.
| > Aborted (core dumped)
| 
| This is what I am worried, in the 'ide_ioport_write' set the 'bus->unit'. It 
| also change the 'buf->ifs[0].select'. Also there maybe some other corner 
| case that causes some inconsistent. And if we choice this method we need to 
| deep into the more ahci-spec to know how things really going.
| 
| > ===
| >
| > | As we the 'ide_exec_cmd' and other functions in 'hw/ide/core.c' check the
| > | 's->blk' directly. I think we just check it in 'ide_cancel_dma_sync' is
| > | enough and also this is more consistent with the other functions.
| > | 'ide_cancel_dma_sync' is also called by 'cmd_device_reset' which is one of
| > | the 'ide_cmd_table' handler.
| >
| >   Yes, I'm okay with either approach. Earlier patch v1 checks 's->blk' in
| > ide_cancel_dma_sync().
| 
| I prefer the 'check the s->blk in the beginning of ide_cancel_dma_sync' 
method.
| Some little different with your earlier patch.
| 
| Anyway, let the maintainer do the choices.
| 

@John ...wdyt?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync

[P J P]

  Hello Li,

+-- On Fri, 18 Sep 2020, Li Qiang wrote --+
| P J P  
篋\x8E2020綛\xB49\xE6\x9C\x8818\xE6\x97ュ\x91\xA8篋\x94 
筝\x8B\xE5\x8D\x886:26\xE5\x86\x99\xE9\x81\x93鐚\x9A
| > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+
| > | Update v2: use an assert() call
| > |   ->https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html
| 
| In 'ide_ioport_write' the guest can set 'bus->unit' to 0 or 1 by issue 
| 'ATA_IOPORT_WR_DEVICE_HEAD'. So this case the guest can set the active ifs. 
| If the guest set this to 1.
| 
| Then in 'idebus_active_if' will return 'IDEBus.ifs[1]' and thus the 's->blk' 
| will be NULL.

Right, guest does select the drive via

  portio_write
   ->ide_ioport_write
  case ATA_IOPORT_WR_DEVICE_HEAD:
  /* FIXME: HOB readback uses bit 7 */
  bus->ifs[0].select = (val & ~0x10) | 0xa0;
  bus->ifs[1].select = (val | 0x10) | 0xa0;
  /* select drive */
  bus->unit = (val >> 4) & 1; <== set bus->unit=0x1
  break;


| So from your (Peter's) saying, we need to check the value in
| 'ATA_IOPORT_WR_DEVICE_HEAD' handler. To say if the guest
| set a valid 'bus->unit'. This can also work I think.

Yes, with the following fix, an assert(3) in ide_cancel_dma_sync fails.

===
diff --git a/hw/ide/core.c b/hw/ide/core.c
index f76f7e5234..cb55cc8b0f 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1300,7 +1300,11 @@ void ide_ioport_write(void *opaque, uint32_t addr, 
uint_)
 bus->ifs[0].select = (val & ~0x10) | 0xa0;
 bus->ifs[1].select = (val | 0x10) | 0xa0;
 /* select drive */
+uint8_t bu = bus->unit;
 bus->unit = (val >> 4) & 1;
+if (!bus->ifs[bus->unit].blk) {
+bus->unit = bu;
+}
 break;
 default:

qemu-system-x86_64: ../hw/ide/core.c:724: ide_cancel_dma_sync: Assertion 
`s->bus->dma->aiocb == NULL' failed.
Aborted (core dumped)
===
 
| As we the 'ide_exec_cmd' and other functions in 'hw/ide/core.c' check the 
| 's->blk' directly. I think we just check it in 'ide_cancel_dma_sync' is 
| enough and also this is more consistent with the other functions. 
| 'ide_cancel_dma_sync' is also called by 'cmd_device_reset' which is one of 
| the 'ide_cmd_table' handler.

  Yes, I'm okay with either approach. Earlier patch v1 checks 's->blk' in 
ide_cancel_dma_sync().
 
| BTW, where is the Peter's email saying this, just want to learn something, 
| :).

  -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg05820.html

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

[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

Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync

[P J P]

+-- On Fri, 18 Sep 2020, Li Qiang wrote --+
| Update v2: use an assert() call
|   ->https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html
...
| I think it is better to defer this check to 'ide_cancel_dma_sync'. 
| 'ide_cancel_dma_sync' is also called by 'cmd_device_reset' and all of the 
| handlers of 'ide_cmd_table' will check whether the 's->blk' is NULL in the 
| beginning of 'ide_exec_cmd'.
| 
| So I think it is reasonable to check 's->blk' at the begining of 
| 'ide_cancel_dma_sync'.

* Yes, earlier patch v1 above does the same.

* From Peter's reply in another thread of similar issue I gather, issue is 
  setting 'blk' to NULL, even erroneously. So (blk == NULL) check should be 
  done where 'blk' is set to null, rather than where it is dereferenced.

* At the dereference point, assert(3) is good.


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync

[P J P]

+-- On Fri, 4 Sep 2020, P J P wrote --+
| From: Prasad J Pandit 
| 
| When cancelling an i/o operation via ide_cancel_dma_sync(),
| a block pointer may be null. Add check to avoid null pointer
| dereference.
| 
|  -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fide_nullptr1
|  ==1803100==Hint: address points to the zero page.
|  #0 blk_bs ../block/block-backend.c:714
|  #1 blk_drain ../block/block-backend.c:1715
|  #2 ide_cancel_dma_sync ../hw/ide/core.c:723
|  #3 bmdma_cmd_writeb ../hw/ide/pci.c:298
|  #4 bmdma_write ../hw/ide/piix.c:75
|  #5 memory_region_write_accessor ../softmmu/memory.c:483
|  #6 access_with_adjusted_size ../softmmu/memory.c:544
|  #7 memory_region_dispatch_write ../softmmu/memory.c:1465
|  #8 flatview_write_continue ../exec.c:3176
|  ...
| 
| Reported-by: Ruhr-University 
| Signed-off-by: Prasad J Pandit 
| ---
|  hw/ide/core.c | 1 +
|  hw/ide/pci.c  | 5 -
|  2 files changed, 5 insertions(+), 1 deletion(-)
| 
| Update v2: use an assert() call
|  -> https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html
| 
| diff --git a/hw/ide/core.c b/hw/ide/core.c
| index f76f7e5234..8105187f49 100644
| --- a/hw/ide/core.c
| +++ b/hw/ide/core.c
| @@ -718,6 +718,7 @@ void ide_cancel_dma_sync(IDEState *s)
|   * whole DMA operation will be submitted to disk with a single
|   * aio operation with preadv/pwritev.
|   */
| +assert(s->blk);
|  if (s->bus->dma->aiocb) {
|  trace_ide_cancel_dma_sync_remaining();
|  blk_drain(s->blk);
| diff --git a/hw/ide/pci.c b/hw/ide/pci.c
| index b50091b615..b47e675456 100644
| --- a/hw/ide/pci.c
| +++ b/hw/ide/pci.c
| @@ -295,7 +295,10 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
|  /* Ignore writes to SSBM if it keeps the old value */
|  if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
|  if (!(val & BM_CMD_START)) {
| -ide_cancel_dma_sync(idebus_active_if(bm->bus));
| +IDEState *s = idebus_active_if(bm->bus);
| +if (s->blk) {
| +ide_cancel_dma_sync(s);
| +}
|  bm->status &= ~BM_STATUS_DMAING;
|  } else {
|  bm->cur_addr = bm->addr;


Ping...! (needs review)
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH] fdc: check null block pointer before blk_pwrite

[P J P]

+-- On Thu, 27 Aug 2020, P J P wrote --+
| While transferring data via fdctrl_write_data(), 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 | 3 ++-
|  1 file changed, 2 insertions(+), 1 deletion(-)
| 
| diff --git a/hw/block/fdc.c b/hw/block/fdc.c
| index e9ed3eef45..dedadac68a 100644
| --- a/hw/block/fdc.c
| +++ b/hw/block/fdc.c
| @@ -2419,7 +2419,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));
| 

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

[PATCH v2] hw/ide: check null block before _cancel_dma_sync

[P J P]

From: Prasad J Pandit 

When cancelling an i/o operation via ide_cancel_dma_sync(),
a block pointer may be null. Add check to avoid null pointer
dereference.

 -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fide_nullptr1
 ==1803100==Hint: address points to the zero page.
 #0 blk_bs ../block/block-backend.c:714
 #1 blk_drain ../block/block-backend.c:1715
 #2 ide_cancel_dma_sync ../hw/ide/core.c:723
 #3 bmdma_cmd_writeb ../hw/ide/pci.c:298
 #4 bmdma_write ../hw/ide/piix.c:75
 #5 memory_region_write_accessor ../softmmu/memory.c:483
 #6 access_with_adjusted_size ../softmmu/memory.c:544
 #7 memory_region_dispatch_write ../softmmu/memory.c:1465
 #8 flatview_write_continue ../exec.c:3176
 ...

Reported-by: Ruhr-University 
Signed-off-by: Prasad J Pandit 
---
 hw/ide/core.c | 1 +
 hw/ide/pci.c  | 5 -
 2 files changed, 5 insertions(+), 1 deletion(-)

Update v2: use an assert() call
 -> https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html

diff --git a/hw/ide/core.c b/hw/ide/core.c
index f76f7e5234..8105187f49 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -718,6 +718,7 @@ void ide_cancel_dma_sync(IDEState *s)
  * whole DMA operation will be submitted to disk with a single
  * aio operation with preadv/pwritev.
  */
+assert(s->blk);
 if (s->bus->dma->aiocb) {
 trace_ide_cancel_dma_sync_remaining();
 blk_drain(s->blk);
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index b50091b615..b47e675456 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -295,7 +295,10 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
 /* Ignore writes to SSBM if it keeps the old value */
 if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
 if (!(val & BM_CMD_START)) {
-ide_cancel_dma_sync(idebus_active_if(bm->bus));
+IDEState *s = idebus_active_if(bm->bus);
+if (s->blk) {
+ide_cancel_dma_sync(s);
+}
 bm->status &= ~BM_STATUS_DMAING;
 } else {
 bm->cur_addr = bm->addr;
-- 
2.26.2

Re: [PATCH v1] sd: sdhci: assert data_count is within fifo_buffer

[P J P]

+-- On Thu, 3 Sep 2020, Philippe Mathieu-Daudé wrote --+
| If you don't mind I might split the assert in 2 when applying:
| 
| -assert(s->data_count <= s->buf_maxsz && s->data_count > begin);
| +assert(s->data_count <= s->buf_maxsz);
| +assert(s->data_count > begin);

Sure, np. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH v1] sd: sdhci: assert data_count is within fifo_buffer

[P J P]

+-- On Thu, 3 Sep 2020, Philippe Mathieu-Daudé wrote --+
| > -assert(s->data_count <= s->buf_maxsz && s->data_count > begin);
| > +assert(s->data_count <= s->buf_maxsz);
| > +assert(s->data_count > begin);
| 
| Doesn't seem enough, guest crash here, having:
| 
| (gdb) p begin
| $1 = 0
| (gdb) p s->data_count
| $2 = 0

I was actually thinking of a case if 's->data_count' and 'begin' are same? It 
may lead to an infinite loop condition.

| (gdb) p s->blksize
| $3 = 0

This is strange. 

| Beh, something is wrong in this model, because when using ADMA2
| length 0 means 65536 bytes (see '1.13.4. Descriptor Table' in
| "SD Host Controller Simplified Specification Version 2.00").

* DMA length 's->data_count - begin'?

* if s->blksize is 65536, it'd set 'block_size = 0' in transfer_multi_blocks()

   #define BLOCK_SIZE_MASK (4 * KiB - 1)  <== 0xFFF

   static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)  
   
   {
 ...
 const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;  <== 0


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH] hw/ide: check null block pointer before blk_drain

[P J P]

+-- On Thu, 3 Sep 2020, Philippe Mathieu-Daudé wrote --+
| > +if (s->blk) {
| > +ide_cancel_dma_sync(s);
| > +bm->status &= ~BM_STATUS_DMAING;
| 
| If you don't clear this bit the guest might keep retrying (looping).

Oh, okay will keep it out of the if(s->blk) block.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH] hw/ide: check null block pointer before blk_drain

[P J P]

+-- On Mon, 31 Aug 2020, Philippe Mathieu-Daudé wrote --+
| > +++ b/hw/ide/core.c
| > @@ -718,7 +718,7 @@ void ide_cancel_dma_sync(IDEState *s)
| > -if (s->bus->dma->aiocb) {
| > +if (s->blk && s->bus->dma->aiocb) {
| 
| But s->blk mustn't be null here... IMHO we should assert() here and add a
| check earlier.

===
diff --git a/hw/ide/core.c b/hw/ide/core.c
index f76f7e5234..8105187f49 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -718,6 +718,7 @@ void ide_cancel_dma_sync(IDEState *s)
  * whole DMA operation will be submitted to disk with a single
  * aio operation with preadv/pwritev.
  */
+assert(s->blk);
 if (s->bus->dma->aiocb) {
 trace_ide_cancel_dma_sync_remaining();
 blk_drain(s->blk);
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index b50091b615..51bb9c9abc 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -295,8 +295,11 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
 /* Ignore writes to SSBM if it keeps the old value */
 if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
 if (!(val & BM_CMD_START)) {
-ide_cancel_dma_sync(idebus_active_if(bm->bus));
-bm->status &= ~BM_STATUS_DMAING;
+IDEState *s = idebus_active_if(bm->bus);
+if (s->blk) {
+ide_cancel_dma_sync(s);
+bm->status &= ~BM_STATUS_DMAING;
+}
 } else {
 bm->cur_addr = bm->addr;
 if (!(bm->status & BM_STATUS_DMAING)) {
===


Does it look okay?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

[PATCH v1] sd: sdhci: assert data_count is within fifo_buffer

[P J P]

From: Prasad J Pandit 

While doing multi block SDMA, transfer block size may exceed
the 's->fifo_buffer[s->buf_maxsz]' size. It may leave the
current element pointer 's->data_count' pointing out of bounds.
Leading the subsequent DMA r/w operation to OOB access issue.
Assert that 's->data_count' is within fifo_buffer.

 -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fsdhci_oob_write1
 ==1459837==ERROR: AddressSanitizer: heap-buffer-overflow
 WRITE of size 54722048 at 0x6151e280 thread T3
 #0  __interceptor_memcpy (/lib64/libasan.so.6+0x3a71d)
 #1  flatview_read_continue ../exec.c:3245
 #2  flatview_read ../exec.c:3278
 #3  address_space_read_full ../exec.c:3291
 #4  address_space_rw ../exec.c:3319
 #5  dma_memory_rw_relaxed ../include/sysemu/dma.h:87
 #6  dma_memory_rw ../include/sysemu/dma.h:110
 #7  dma_memory_read ../include/sysemu/dma.h:116
 #8  sdhci_sdma_transfer_multi_blocks ../hw/sd/sdhci.c:629
 #9  sdhci_write ../hw/sd/sdhci.c:1097
 #10 memory_region_write_accessor ../softmmu/memory.c:483
 ...

Reported-by: Ruhr-University 
Suggested-by: Philippe Mathieu-Daudé 
Signed-off-by: Prasad J Pandit 
---
 hw/sd/sdhci.c | 2 ++
 1 file changed, 2 insertions(+)

Update v1: use assert(3) calls
  -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg00966.html

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 1785d7e1f7..023acbed41 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -604,6 +604,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 s->blkcnt--;
 }
 }
+assert(s->data_count <= s->buf_maxsz && s->data_count > begin);
 dma_memory_write(s->dma_as, s->sdmasysad,
  >fifo_buffer[begin], s->data_count - begin);
 s->sdmasysad += s->data_count - begin;
@@ -626,6 +627,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 s->data_count = block_size;
 boundary_count -= block_size - begin;
 }
+assert(s->data_count <= s->buf_maxsz && s->data_count > begin);
 dma_memory_read(s->dma_as, s->sdmasysad,
 >fifo_buffer[begin], s->data_count - begin);
 s->sdmasysad += s->data_count - begin;
-- 
2.26.2

Re: [PATCH] sd: sdhci: check data_count is within fifo_buffer

[P J P]

+-- On Tue, 1 Sep 2020, Philippe Mathieu-Daudé wrote --+
| >  -> 
https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fsdhci_oob_write1
| 
| This directory is 3 months old, I can't find it on the list...
| Did I missed that or did the list eat the report?

  No, it was reported to [qemu-security] contacts. These are few last 
remaining old issues. We shall have better response time now.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH] sd: sdhci: check data_count is within fifo_buffer

[P J P]

+-- On Wed, 2 Sep 2020, Philippe Mathieu-Daudé wrote --+
| > +if (s->data_count <= begin || s->data_count > s->buf_maxsz) {
| > +break;
| > +}
| 
| Thanks for your patch. Note however this kind of security fix hides
| the bug in the model, furthermore it makes the model behaves differently
| that the real hardware (which we aim to model).

  Right, got it.

| I posted a different fix for this problem (fixing the model bug):
| https://www.mail-archive.com/qemu-devel@nongnu.org/msg735715.html
| (you already reviewed it, thank you - I still comment it for the
| other reviewers).
| 
| Can you replace by an assert() call instead? Since this should never
| happen.

  Replace above check with an assert() call? Even with your revised fix above?


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH v2 3/3] hw/sd/sdhci: Fix DMA Transfer Block Size field

[P J P]

+-- On Tue, 1 Sep 2020, Philippe Mathieu-Daudé wrote --+
| The 'Transfer Block Size' field is 12-bit wide.
| See section '2.2.2. Block Size Register (Offset 004h)' in datasheet.
| 
| Buglink: https://bugs.launchpad.net/qemu/+bug/1892960

+ https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fsdhci_oob_write1

| diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
| index 60f083b84c1..ecbf84e9d3f 100644
| --- a/hw/sd/sdhci.c
| +++ b/hw/sd/sdhci.c
| @@ -1104,7 +1104,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
|  break;
|  case SDHC_BLKSIZE:
|  if (!TRANSFERRING_DATA(s->prnsts)) {
| -MASKED_WRITE(s->blksize, mask, value);
| +MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
|  MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);

It helps to fix above issues.

Reviewed-by: Prasad J Pandit 

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH] sd: sdhci: check data_count is within fifo_buffer

[P J P]

+-- On Sun, 30 Aug 2020, Alexander Bulekov wrote --+
| Here's a qtest reproducer for this one:
| 
| cat << EOF |./i386-softmmu/qemu-system-i386 -nodefaults \
| -device sdhci-pci -device sd-card,drive=mydrive \
| -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
| -nographic -accel qtest -qtest stdio -nographic
| outl 0xcf8 0x80001001
| outl 0xcfc 0x7e6f25b7
| outl 0xcf8 0x80001012
| outl 0xcfc 0x842b1212
| writeb 0x12120005 0xff
| writeq 0x12120027 0x5e32b7120584125e
| write 0x0 0x1 0x21
| write 0x8 0x1 0x21
| write 0x10 0x1 0x21
| write 0x18 0x1 0x21
| write 0x20 0x1 0x21
| write 0x23 0x1 0x2b
| writeq 0x1212000c 0x123a0584052da3ab
| writeq 0x1212 0xcfff0002
| writeq 0x12120027 0x5c04c1c9c15e
| clock_step
| EOF
| 
| Is it related to this https://bugs.launchpad.net/qemu/+bug/1892960 ?

  Yes, it's same. This patch fixes it.


| > +++ b/hw/sd/sdhci.c
| > @@ -604,6 +604,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState 
*s)
| >  }
| > +if (s->data_count <= begin || s->data_count > s->buf_maxsz) {
| > +break;
| > +}
| >  dma_memory_write(s->dma_as, s->sdmasysad,
| >   >fifo_buffer[begin], s->data_count - 
begin);
| ...
| > +if (s->data_count <= begin || s->data_count > s->buf_maxsz) {
| > +break;
| > +}
| >  dma_memory_read(s->dma_as, s->sdmasysad,
| >  >fifo_buffer[begin], s->data_count - begin);


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

[PATCH] sd: sdhci: check data_count is within fifo_buffer

[P J P]

From: Prasad J Pandit 

While doing multi block SDMA, transfer block size may exceed
the 's->fifo_buffer[s->buf_maxsz]' size. It may leave the
current element pointer 's->data_count' pointing out of bounds.
Leading the subsequent DMA r/w operation to OOB access issue.
Add check to avoid it.

 -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fsdhci_oob_write1
 ==1459837==ERROR: AddressSanitizer: heap-buffer-overflow
 WRITE of size 54722048 at 0x6151e280 thread T3
#0  __interceptor_memcpy (/lib64/libasan.so.6+0x3a71d)
#1  flatview_read_continue ../exec.c:3245
#2  flatview_read ../exec.c:3278
#3  address_space_read_full ../exec.c:3291
#4  address_space_rw ../exec.c:3319
#5  dma_memory_rw_relaxed ../include/sysemu/dma.h:87
#6  dma_memory_rw ../include/sysemu/dma.h:110
#7  dma_memory_read ../include/sysemu/dma.h:116
#8  sdhci_sdma_transfer_multi_blocks ../hw/sd/sdhci.c:629
#9  sdhci_write ../hw/sd/sdhci.c:1097
#10 memory_region_write_accessor ../softmmu/memory.c:483
...

Reported-by: Ruhr-University 
Signed-off-by: Prasad J Pandit 
---
 hw/sd/sdhci.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 1785d7e1f7..155e25ceee 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -604,6 +604,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 s->blkcnt--;
 }
 }
+if (s->data_count <= begin || s->data_count > s->buf_maxsz) {
+break;
+}
 dma_memory_write(s->dma_as, s->sdmasysad,
  >fifo_buffer[begin], s->data_count - begin);
 s->sdmasysad += s->data_count - begin;
@@ -626,6 +629,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 s->data_count = block_size;
 boundary_count -= block_size - begin;
 }
+if (s->data_count <= begin || s->data_count > s->buf_maxsz) {
+break;
+}
 dma_memory_read(s->dma_as, s->sdmasysad,
 >fifo_buffer[begin], s->data_count - begin);
 s->sdmasysad += s->data_count - begin;
-- 
2.26.2

[PATCH] hw/ide: check null block pointer before blk_drain

[P J P]

From: Prasad J Pandit 

While cancelling an i/o operation via ide_cancel_dma_sync(),
check for null block pointer before calling blk_drain(). Avoid
null pointer dereference.

 -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fide_nullptr1
==1803100==Hint: address points to the zero page.
#0 blk_bs ../block/block-backend.c:714
#1 blk_drain ../block/block-backend.c:1715
#2 ide_cancel_dma_sync ../hw/ide/core.c:723
#3 bmdma_cmd_writeb ../hw/ide/pci.c:298
#4 bmdma_write ../hw/ide/piix.c:75
#5 memory_region_write_accessor ../softmmu/memory.c:483
#6 access_with_adjusted_size ../softmmu/memory.c:544
#7 memory_region_dispatch_write ../softmmu/memory.c:1465
#8 flatview_write_continue ../exec.c:3176
...

Reported-by: Ruhr-University 
Signed-off-by: Prasad J Pandit 
---
 hw/ide/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index d997a78e47..038af1cd6b 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -718,7 +718,7 @@ void ide_cancel_dma_sync(IDEState *s)
  * whole DMA operation will be submitted to disk with a single
  * aio operation with preadv/pwritev.
  */
-if (s->bus->dma->aiocb) {
+if (s->blk && s->bus->dma->aiocb) {
 trace_ide_cancel_dma_sync_remaining();
 blk_drain(s->blk);
 assert(s->bus->dma->aiocb == NULL);
-- 
2.26.2

[PATCH] fdc: check null block pointer before blk_pwrite

[P J P]

From: Prasad J Pandit 

While transferring data via fdctrl_write_data(), 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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index e9ed3eef45..dedadac68a 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2419,7 +2419,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] vvfat: set status to odd fixes

[P J P]

From: Prasad J Pandit 

Virtual VFAT driver is quite old and rarely used. Set its status
to Odd Fixes.

Signed-off-by: Prasad J Pandit 
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6aa54f7f8f..2a15a3987e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2930,7 +2930,7 @@ F: block/vpc.c
 vvfat
 M: Kevin Wolf 
 L: qemu-block@nongnu.org
-S: Supported
+S: Odd Fixes
 F: block/vvfat.c
 
 Image format fuzzer
-- 
2.26.2

Re: [Qemu-block] [QEMU-SECURITY] ide: fix assertion in ide_dma_cb() to prevent qemu DoS from quest

[P J P]

+-- On Tue, 16 Jul 2019, John Snow wrote --+
| I also feel that a privileged DOS by a guest of a legacy device is actually 
| low priority security-wise, unless we can demonstrate that there are side 
| effects that can be exploited.

Right, we are not treating this as a CVE issue as is. Privileged guest user 
has many ways to cause similar DoS effect, without triggering this assert(3).

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-block] [Qemu-devel] [PATCH] scsi-generic: avoid possible out-of-bounds access to r->buf

[P J P]

+-- On Fri, 11 Jan 2019, Paolo Bonzini wrote --+
| diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
| index 7237b4162e..42700e8897 100644
| --- a/hw/scsi/scsi-generic.c
| +++ b/hw/scsi/scsi-generic.c
| @@ -182,7 +182,7 @@ static void scsi_handle_inquiry_reply(SCSIGenericReq *r, 
SCSIDevice *s)
|  /* Also take care of the opt xfer len. */
|  stl_be_p(>buf[12],
|  MIN_NON_ZERO(max_transfer, ldl_be_p(>buf[12])));
| -} else if (s->needs_vpd_bl_emulation && page == 0x00) {
| +} else if (s->needs_vpd_bl_emulation && page == 0x00 && r->buflen >= 
4) {

  Should it be r->buflen > 4?  page_idx > 4 in while()

| + * right place with an in-place insert.  When the while loop
| + * begins the device response is at r[0] to r[page_idx - 1].

  r->buf[0] to r->buf[page_idx - 1] ?


| -for (page_idx = lduw_be_p(r->buf + 2) + 4;
| - page_idx > 4 && r->buf[page_idx - 1] >= 0xb0;
| - page_idx--) {
| +page_idx = lduw_be_p(r->buf + 2) + 4;
| +page_idx = MIN(page_idx, r->buflen);
| +while (page_idx > 4 && r->buf[page_idx - 1] >= 0xb0) {
|  if (page_idx < r->buflen) {
|  r->buf[page_idx] = r->buf[page_idx - 1];
|  }
| +page_idx--;
| +}
| +if (page_idx < r->buflen) {
| +r->buf[page_idx] = 0xb0;
|  }
| -r->buf[page_idx] = 0xb0;
|  stw_be_p(r->buf + 2, lduw_be_p(r->buf + 2) + 1);
|  }


Looks okay. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-block] [PATCH v1] ide: check BlockBackend object in ide_cancel_dma_sync

[P J P]

  Hello Kevin,

+-- On Mon, 17 Jul 2017, Kevin Wolf wrote --+
| I think Stefan didn't only mean a stack trace, but an actual instruction
| how to reproduce this. VM configuration, what actions to take, etc.
| 
| In fact, I will add that we will probably want a qtest case as a
| regression test anyway, and tests are always great for describing how to
| reproduce a problem, too.

It was found via a fuzzing exercise; I've requested a reproducer from original 
reporter, will try to create a qtest once it's available.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

[Qemu-block] [PATCH v1] ide: check BlockBackend object in ide_cancel_dma_sync

[P J P]

From: Prasad J Pandit 

When cancelling pending DMA requests in ide_cancel_dma_sync,
the s->blk object could be null, leading to a null dereference.
Add check to avoid it.

  blk_bs (blk=0x0) at block/block-backend.c:389
  blk_drain (blk=0x0) at block/block-backend.c:1232
  ide_cancel_dma_sync (s=0x7f203241c1a8) at hw/ide/core.c:684
  bmdma_cmd_writeb (bm=0x7f203241cf20, val=104) at hw/ide/pci.c:237
  bmdma_write (opaque=0x7f203241cf20, addr=0, val=104, size=1) at 
hw/ide/piix.c:77

Reported-by: Chensongnian 
Signed-off-by: Prasad J Pandit 
---
 hw/ide/core.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

Update: add call stack trace leading the null dereference.

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 0b48b64..04474b3 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -681,8 +681,10 @@ void ide_cancel_dma_sync(IDEState *s)
 #ifdef DEBUG_IDE
 printf("%s: draining all remaining requests", __func__);
 #endif
-blk_drain(s->blk);
-assert(s->bus->dma->aiocb == NULL);
+if (s->blk) {
+blk_drain(s->blk);
+assert(s->bus->dma->aiocb == NULL);
+}
 }
 }
 
-- 
2.9.4

[Qemu-block] [PATCH] ide: check BlockBackend object in ide_cancel_dma_sync

[P J P]

From: Prasad J Pandit 

When cancelling pending DMA requests in ide_cancel_dma_sync,
the s->blk object could be null, leading to a null dereference.
Add check to avoid it.

Reported-by: Chensongnian 
Signed-off-by: Prasad J Pandit 
---
 hw/ide/core.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 0b48b64..04474b3 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -681,8 +681,10 @@ void ide_cancel_dma_sync(IDEState *s)
 #ifdef DEBUG_IDE
 printf("%s: draining all remaining requests", __func__);
 #endif
-blk_drain(s->blk);
-assert(s->bus->dma->aiocb == NULL);
+if (s->blk) {
+blk_drain(s->blk);
+assert(s->bus->dma->aiocb == NULL);
+}
 }
 }
 
-- 
2.9.4

[Qemu-block] [PATCH] block: check BlockDriverState object before dereference

[P J P]

From: Prasad J Pandit 

When processing ATA_CACHE_FLUSH ide controller command,
BlockDriverState object could be null. Add check to avoid
null pointer dereference.

Reported-by: Kieron Shorrock 
Signed-off-by: Prasad J Pandit 
---
 block/block-backend.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 0df3457..6a543a4 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1168,8 +1168,13 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, 
int64_t offset, int bytes,
 {
 BlkAioEmAIOCB *acb;
 Coroutine *co;
+BlockDriverState *bs = blk_bs(blk);
 
-bdrv_inc_in_flight(blk_bs(blk));
+if (!bs) {
+return NULL;
+}
+
+bdrv_inc_in_flight(bs);
 acb = blk_aio_get(_aio_em_aiocb_info, blk, cb, opaque);
 acb->rwco = (BlkRwCo) {
 .blk= blk,
@@ -1182,7 +1187,7 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, 
int64_t offset, int bytes,
 acb->has_returned = false;
 
 co = qemu_coroutine_create(co_entry, acb);
-bdrv_coroutine_enter(blk_bs(blk), co);
+bdrv_coroutine_enter(bs, co);
 
 acb->has_returned = true;
 if (acb->rwco.ret != NOT_DONE) {
-- 
2.9.4

Re: [Qemu-block] [Qemu-devel] [PATCH] block: nvme: correct the nvme queue id check

[P J P]

+-- On Sat, 22 Oct 2016, Peter Maydell wrote --+
| Secondly, it's almost the same as this cleanup
| patch from Thomas Huth that's already in qemu-trivial:
| http://patchwork.ozlabs.org/patch/681349/
| 
| except that your version is removing the !
| negations from the return value.
| 
| Can you explain in a bit more detail what the bug
| is here and why this is the right fix for it?

   nvme_check_sqid(NvmeCtrl *n, uint16_t sqid)
   {
   return sqid < n->num_queues && n->sq[sqid] != NULL ? 0 : -1;
   }

Both 'nvme_check_sqid' and 'nvme_check_cqid' return zero(0) when the 
respective queue id's are valid and -1 when they are invalid. The '!' was 
causing it to return invalid QID error when the check function returned zero.

The code seems incosistent quite a bit. The current check returns invalid QID 
error for '!cqid' and '!sqid', but these IDs are used as index into 'n->cq' 
array, not sure why index zero(0) is invalid. But then I found this in the 
NVME specification[0]

   "...One entry in each queue is not available for use due 
 to Head and Tail entry pointer definition."

Not sure if this one entry is at index zero(0). If so, there are other calls 
to the 'nvme_check' functions which are not acompanyed with '!cqid' or 
'!sqid' checks. Ex.

nvme_process_db
qid = (addr - 0x1000) >> 3;
if (nvme_check_sqid(n, qid)) {
return;
}

nvme_del_sq
if (!nvme_check_cqid(n, sq->cqid)) {
cq = n->cq[sq->cqid];

I haven't check these in further details yet. When 'cqid' is invalid it 
returns 'NVME_INVALID_CQID' but when 'sqid' is invalid it returns 
'NVME_INVALID_QID'. Not sure why not use 'NVME_INVALID_QID' for both.

[0] 
http://www.nvmexpress.org/wp-content/uploads/NVM-Express-1_2-Gold-20141209.pdf

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

[Qemu-block] [PATCH] block: nvme: correct the nvme queue id check

[P J P]

From: Prasad J Pandit 

NVME Express Controller has two queues, submission & completion
queue. When creating a new queue object, 'nvme_create_sq' and
'nvme_create_cq' routines incorrectly check the queue id field.
It could lead to an OOB access issue. Correct the queue id check
to avoid it.

Reported-by: Qinghao Tang 
Signed-off-by: Prasad J Pandit 
---
 hw/block/nvme.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 2ded247..61bdc9d 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -373,7 +373,7 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeCmd *cmd)
 if (!cqid || nvme_check_cqid(n, cqid)) {
 return NVME_INVALID_CQID | NVME_DNR;
 }
-if (!sqid || (sqid && !nvme_check_sqid(n, sqid))) {
+if (!sqid || nvme_check_sqid(n, sqid)) {
 return NVME_INVALID_QID | NVME_DNR;
 }
 if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) {
@@ -447,7 +447,7 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd *cmd)
 uint16_t qflags = le16_to_cpu(c->cq_flags);
 uint64_t prp1 = le64_to_cpu(c->prp1);
 
-if (!cqid || (cqid && !nvme_check_cqid(n, cqid))) {
+if (!cqid || nvme_check_cqid(n, cqid)) {
 return NVME_INVALID_CQID | NVME_DNR;
 }
 if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) {
-- 
2.7.4

Re: [Qemu-block] Overflow in Virtio-BLK and SCSI Requests?

[P J P]

  Hello Stefan, all

+-- On Mon, 30 May 2016, Peter Lieven wrote --+
| Am 27.05.2016 um 23:22 schrieb Stefan Hajnoczi:
| > On Fri, May 20, 2016 at 11:27:02AM +0200, Peter Lieven wrote:
| > > while working at the iSCSI code in Qemu I came across the following line
| > > in iscsi_aio_ioctl
| > >
| > > memcpy(>task->cdb[0], acb->ioh->cmdp, acb->ioh->cmd_len);
| > >
| > > Is there anything to ensure that the cmd_len is valid when the requests is
| > > e.g. coming in via
| > > virtio_blk_handle_scsi ?
| > >
| > > It seems that virtio-scsi does not allow to pass ioctls directly from
| > > Guest, but at least virtio-blk
| > > does. And in virtio-blk it seems the data is blindly copied from
| > > elem->out_sg[1]. So it would
| > > be possible to overflow the acb->task->cdb. Or am I wrong here?
| > I agree that the guest can trigger a buffer overflow here.
| >
| > I think the bug is in iscsi_aio_ioctl() because ioctl handlers must
| > validates their inputs.  iscsi.c assumes acb->ioh->cmd_len is always
| > less than sizeof(acb->task->cdb[]) (SCSI_CDB_MAX_SIZE).
| >
| > iscsi_aio_ioctl() needs to be audited and checks should be added for
| > assumptions like this.
| >
| > Do you have time to do this?
| 
| I already submitted a patch and Paolo has put it in his queue.
| 
|  block/iscsi: avoid potential overflow of acb->task->cdb

   Great! Thank you so much for CC'ing me. I'll process it further.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] ahci: unmap fixes

[P J P]

> On Monday, 8 February 2016 10:23 PM, John Snow <js...@redhat.com> wrote:
>> PJP, ping? Look good?

Oops, sorry!

> On 01/29/2016 04:41 PM, John Snow wrote:
>> As reported by Zuozhi fzz <zuozhi@alibaba-inc.com>, there's a problem
>> you can expose in AHCI by rewriting the command list buffer and/or FIS
>> receive buffer addresses, then re-starting the AHCI device before bringing
>> it to a stop. Depending on the success of the remap operations, you may
>> be able to transition the device to a state where it thinks it is
>> "running" but no longer has a guest memory mapping.
>>
>> When you try to transition it to the stopped state, QEMU crashes.
>>
>> Tighten up the start/stop conditions, and pepper in a paranoia check inside
>> of the unmap function.
>>
>> John Snow (4):
>>   ahci: Do not unmap NULL addresses
>>   ahci: handle LIST_ON and FIS_ON in map helpers
>>   ahci: explicitly reject bad engine states on post_load

>> ahci: prohibit "restarting" the FIS or CLB engines

  Yes, they look good.

Thank you.

---  -P J P
http://feedmug.com

Re: [Qemu-block] [Qemu-devel] [PATCH v2] ide: ahci: reset ncq object to unused on error

[P J P]

+-- On Mon, 11 Jan 2016, John Snow wrote --+
| On 01/11/2016 10:00 AM, Kevin Wolf wrote:
| >> When processing NCQ commands, ACHI device emulation prepares a
| > 
| > s/ACHI/AHCI/

  Ah, sorry.:(

| > Can you still fix this in your tree, John?
| Yes, thanks.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-block] [Qemu-devel] [PATCH] ide: ahci: reset ncq object to unused on error

[P J P]

+-- On Fri, 8 Jan 2016, John Snow wrote --+
| In both of these error pathways, AIOCB is actually never assigned to
| begin with.

  True, it's mentioned in the commit message.

| So it's not necessarily a use-after-free.

  Yes, right.
  
| I think it should be safe to put ncq_tfs->used = 0 directly inside of
| ncq_err, and that way we won't have any other error pathways omitting
| this in the future.

  Okay, I'll send an updated patch.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-block] [Qemu-devel] [PATCH v2] ide: ahci: reset ncq object to unused on error

[P J P]

+-- On Fri, 8 Jan 2016, John Snow wrote --+
| >  ide_state->status = READY_STAT | ERR_STAT;
| >  ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag);
| > +ncq_tfs->used = 0;
| >  }
| 
| Thanks, applied to my IDE tree:
| 
| https://github.com/jnsnow/qemu/commits/ide
| https://github.com/jnsnow/qemu.git

Great! Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

[Qemu-block] [PATCH] ide: ahci: reset ncq object to unused on error

[P J P]

From: Prasad J Pandit 

When processing NCQ commands, ACHI device emulation prepares a
NCQ transfer object; To which an aio control block(aiocb) object
is assigned in 'execute_ncq_command'. In case, when the NCQ
command is invalid, the 'aiocb' object is not assigned, and NCQ
transfer object is left as 'used'. This leads to a use after
free error in 'bdrv_aio_cancel_async' via 'ahci_reset_port'.
Reset NCQ transfer object to 'unused' to avoid it.

Reported-by: Qinghao Tang 
Signed-off-by: Prasad J Pandit 
---
 hw/ide/ahci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index dd1912e..e359127 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1014,6 +1014,7 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs)
 DPRINTF(port, "error: unsupported NCQ command (0x%02x) received\n",
 ncq_tfs->cmd);
 qemu_sglist_destroy(_tfs->sglist);
+ncq_tfs->used = 0;
 ncq_err(ncq_tfs);
 }
 }
@@ -1081,6 +1082,7 @@ static void process_ncq_command(AHCIState *s, int port, 
uint8_t *cmd_fis,
  "is smaller than the requested size (0x%zx)",
  ncq_tfs->sglist.size, size);
 qemu_sglist_destroy(_tfs->sglist);
+ncq_tfs->used = 0;
 ncq_err(ncq_tfs);
 ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW);
 return;
-- 
2.4.3