| Message ID | 20211118115733.4038610-2-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/block/fdc: Fix CVE-2021-3507 | expand |
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;
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;
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: * 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(+)