Re: [PATCH-for-6.2 1/2] hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)

[Hanna Reitz]

On 18.11.21 12:57, Philippe Mathieu-Daudé wrote:
> Per the 82078 datasheet, if the end-of-track (EOT byte in
> the FIFO) is more than the number of sectors per side, the
> command is terminated unsuccessfully:

Patch looks OK to me (can’t believe I’ve looked into the spec...), just 
one question (note from later self: I really believed this, and now I 
ended up with this wall of text...):

Isn’t the problem that the EOT is smaller than the start sector index?  
AFAIU fifo[6] is the EOT, ks (fifo[4]) is the start sector number, and 
so the problem occurs if fifo[6] < fifo[4] - 1 (EOT < start sector index).

I don’t think there is any problem if the EOT exceeds the number of 
sectors per anything (e.g. sectors per track or per cylinder). Well, the 
spec might say it should cause an error, but in that case tmp (and thus 
data_len) just become very large.  AFAIU fd_seek() checks that the start 
sector is in bounds, so in fact passing an EOT that is larger than 
cur_drv->last_sect would never be negative, and so never trigger the tmp 
< 0 condition.

The explanation below however seems to be precisely for the case where 
EOT would be larger than last_sect (say 18), and so e.g. sector 19 can’t 
be found (“second occurrence of a pulse on the INDX# pin”).  Contrarily, 
the spec doesn’t seem to say anything for the case where EOT is in 
bounds but less than the start sector index. It doesn’t even seem to say 
how the EOT is evaluated; is it a “read until sector >= EOT”?  (I.e. EOT 
< start sector would yield a zero-byte read)  Or is it a “read until 
sector == EOT”?  (I.e. EOT < start sector would yield an error because 
that condition is never reached, and we go beyond last_sect, yielding 
the same error as if EOT were out of bounds.)

Now that raises another question to me.  Say EOT is out of bounds; 
wouldn’t the FDC still read all sectors until the end of the 
track/cylinder?  And only then raise an error that the sector beyond 
doesn’t exist?

I have no idea if any of that made sense and even if it did, whether I’m 
right.  But even if I am, I don’t think it’s a problem.  Our problem at 
hand is the `tmp` underflow, and it’s good to handle that by returning 
some form of error to the guest instead of crashing. It’s just that I’m 
not sure this solution is actually what the spec requires (because I 
have no idea what the spec requires in that case), and the explanation 
given in the commit message to me seems to define an error for a very 
different case (where tmp would not be negative) that we just execute 
without an error.

When I look into this I see many more things that look like they aren’t 
according to spec (like not checking the sector size, or that we honor 
fifo[0] & 0x80 for READ TRACK even though the spec says no multi-track 
operations for READ TRACK), so I don’t personally care.  It’s good that 
this patch handles the `tmp` underflow, because that’s important, 
compliance with the spec isn’t.

(Yes, the entire reason for this wall of text is that I wonder why the 
commit message goes into so much detail quoting the spec, while I can’t 
find it applies.)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

> * 5.2.5 DATA TRANSFER TERMINATION
>
>    The 82078 supports terminal count explicitly through
>    the TC pin and implicitly through the underrun/over-
>    run and end-of-track (EOT) functions. For full sector
>    transfers, the EOT parameter can define the last
>    sector to be transferred in a single or multisector
>    transfer. If the last sector to be transferred is a par-
>    tial sector, the host can stop transferring the data in
>    mid-sector, and the 82078 will continue to complete
>    the sector as if a hardware TC was received. The
>    only difference between these implicit functions and
>    TC is that they return "abnormal termination" result
>    status. Such status indications can be ignored if they
>    were expected.
>
> * 6.1.3 READ TRACK
>
>    This command terminates when the EOT specified
>    number of sectors have been read. If the 82078
>    does not find an I D Address Mark on the diskette
>    after the second· occurrence of a pulse on the
>    INDX# pin, then it sets the IC code in Status Regis-
>    ter 0 to "01" (Abnormal termination), sets the MA bit
>    in Status Register 1 to "1", and terminates the com-
>    mand.
>
> * 6.1.6 VERIFY
>
>    Refer to Table 6-6 and Table 6-7 for information
>    concerning the values of MT and EC versus SC and
>    EOT value.
>
> * Table 6·6. Result Phase Table
>
> * Table 6-7. Verify Command Result Phase Table
>
> Fix by aborting the transfer when EOT > # Sectors Per Side.
>
> Cc: qemu-stable@nongnu.org
> Cc: Hervé Poussineau <hpoussin@reactos.org>
> Fixes: baca51faff0 ("floppy driver: disk geometry auto detect")
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/339
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   hw/block/fdc.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index fa933cd3263..d21b717b7d6 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1512,6 +1512,14 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int 
> direction)
>           int tmp;
>           fdctrl->data_len = 128 << (fdctrl->fifo[5] > 7 ? 7 : fdctrl->fifo[5]);
>           tmp = (fdctrl->fifo[6] - ks + 1);
> +        if (tmp < 0) {
> +            FLOPPY_DPRINTF("invalid EOT: %d\n", tmp);
> +            fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
> +            fdctrl->fifo[3] = kt;
> +            fdctrl->fifo[4] = kh;
> +            fdctrl->fifo[5] = ks;
> +            return;
> +        }
>           if (fdctrl->fifo[0] & 0x80)
>               tmp += fdctrl->fifo[6];
>           fdctrl->data_len *= tmp;