Re: [PATCH 1/1] hw/ide/core: terminate in-flight DMA on IDE bus reset

[John Snow]

Niklas, I'm sorry to lean on you here a little bit - You've been
working on the SATA side of this a bit more often, can you let me know
if you think this patch is safe?

I'm not immediately sure what the impact of applying it is, but I have
some questions about it:

(1) When does ide_dma_cb get invoked when DRQ_STAT is set but the
return code we were passed is not < 0, and

(2) what's the impact of just *not* executing the end-of-transfer
block here; what happens to the state machine?

On Thu, Sep 21, 2023 at 12:07 PM Simon Rowe <simon.rowe@nutanix.com> wrote:
>
> When an IDE controller is reset, its internal state is being cleared
> before any outstanding I/O is cancelled. If a response to DMA is
> received in this window, the aio callback will incorrectly continue
> with the next part of the transfer (now using sector 0 from
> the cleared controller state).

Eugh, yikes. It feels like we should fix the cancellation ... I'm
worried that if we've reset the state machine and we need to bail on a
DMA callback that the heuristics we use for that will eventually be
wrong, unless I am mistaken and this is totally safe and reliable for
a reason I don't intuitively see right away.

Thoughts?

>
> For a write operation, this results in user data being written to the
> MBR, replacing the first stage bootloader and/or partition table. A
> malicious user could exploit this bug to first read the MBR and then
> rewrite it with user-controller bootloader code.

Oh, good.

>
> This addresses the bug by checking if DRQ_STAT is still set in the DMA
> callback (as it is otherwise cleared at the start of the bus
> reset). If it is not, treat the transfer as ended.
>
> This only appears to affect SATA controllers, plain IDE does not use
> aio.
>
> Fixes: CVE-2023-5088
> Signed-off-by: Simon Rowe <simon.rowe@nutanix.com>
> Cc: Felipe Franciosi <felipe@nutanix.com>
> ---
>  hw/ide/core.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index b5e0dcd29b..826b7eaeeb 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -906,8 +906,12 @@ static void ide_dma_cb(void *opaque, int ret)
>          s->nsector -= n;
>      }
>
> -    /* end of transfer ? */
> -    if (s->nsector == 0) {
> +    /*
> +     * End of transfer ?
> +     * If a bus reset occurs immediately before the callback is invoked the
> +     * bus state will have been cleared. Terminate the transfer.
> +     */
> +    if (s->nsector == 0 || !(s->status & DRQ_STAT)) {
>          s->status = READY_STAT | SEEK_STAT;
>          ide_bus_set_irq(s->bus);
>          goto eot;
> --
> 2.22.3
>