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

[John Snow]

On 07/11/2017 01:08 PM, P J P wrote:
> From: Prasad J Pandit <address@hidden>
> 
> When processing ATA_CACHE_FLUSH ide controller command,
> BlockDriverState object could be null. Add check to avoid
> null pointer dereference.
> 

This happens through 0xE7, what QEMU calls WIN_CACHE_FLUSH and described
in ATA8 ACS3 as "FLUSH CACHE"

The core of the matter here is that any ATA device associated with a
BlockBackend that has no media inserted will accept the command and call
blk_aio_flush, which will later fail because blk_aio_prwv assumes that
the BlockBackend it was given definitely has a BlockDriverState attached.

The associated bdrv_inc_in_flight causes the null pointer dereference.

It's not immediately clear to me what the right fix is here:

(1) Should blk_ functions not assume they will have a BlockDriverState?
(i.e. should an attempted blk_flush on an empty blk just succeed
quietly, or is that inherently an error?)

(2) Should ATA reject such commands entirely?

(3) Both?

ATA says that CDROM drives /may/ accept flush cache, but it doesn't
really say what it has to do about if there's no media. This is for
flushing write cache, after all, and our media are RO for CDROMs.

We could possibly make flush cache a NOP for CDROMs entirely, or I can
remove our support for the command, as it is "optional" and I can't find
any information on what, if anything, it should actually do.

Grumble Grumble, ATA specs.

> Reported-by: Kieron Shorrock <address@hidden>

Thank you for your report.

> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  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;
> +    }
> +

This hotfix as posted is inappropriate I think, because this path has
never been able to return NULL previously and many callers don't check
to see if their command was registered successfully, and rely on the
callback to be executed.

> +    bdrv_inc_in_flight(bs);
>      acb = blk_aio_get(&blk_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) {
>