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.r...@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.r...@nutanix.com>
> Cc: Felipe Franciosi <fel...@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
>
