[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Hanna Reitz
Subject: Re: [PATCH-for-6.2 1/2] hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
Date: Tue, 23 Nov 2021 16:56:18 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.0

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>


   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-

* 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 
          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;

reply via email to

[Prev in Thread] Current Thread [Next in Thread]