Repository navigation
Bound what a G64 tells the drive about its tracks (#823) - #840
Conversation
chrisgleissner
added
3.16
chrisgleissner
left a comment
chrisgleissner
left a comment
There was a problem hiding this comment.
Blocking: the PR confirms only that each two-byte track header was read, not that the declared track data was read. A 14-byte GCR-1541 file with track 0 offset 12 and declared length 0x1E0C passes the new header check and capacity check, then sends gcr_data+14 through gcr_data+7705 to the drive even though those bytes were never read and can be stale after a prior mount. Please require offset + 2 + length <= bytes_read without overflow, and add a red/green truncated-file regression. I am resolving the workflow-only merge conflict separately, retaining both the current HTTP integer test and this G64 test.
GcrImage::load() took each track's length straight from the image, masked it to 14 bits and stored it. That mask admits 16383, while GCRIMAGE_MAXTRACKLEN is 7928, and nothing compared the length against the space left in the buffer behind the track's own offset. insert_disk() then programs that length into the drive engine's parameter RAM, and floppy_mem.vhd both reads and writes memory bounded by it, so a crafted image could point the hardware about 8 KB past the end of the allocation. Three smaller holes on the same path: the offset test admitted offset == GCRIMAGE_MAXSIZE, so tr[0] and tr[1] were read one and two bytes past the buffer; the offset was never compared against how much the file actually delivered, so a length could be taken from a part of the buffer no read had filled; and a track declared zero bytes long reached insert_disk()'s division by track_length. A track whose metadata does not survive these checks is now left as invalidate() left it -- absent -- so add_blank_tracks() gives it a blank track and the mount goes on. add_blank_tracks() in the same file already bound-checks this way, so the missing check in load() was an omission rather than a choice. The checks live in a new header of free functions over plain integers rather than in load() itself, because none of the drive headers can be compiled on a build host and this is the part worth testing there. Reported in GideonZ#823, problems 2 and 4.
insert_disk() declared bit_time and track_len without initialisers, assigned them only inside the branch taken for a track that is present, and read them in the branch taken for one that is not. Two consequences. The first is reachable with nothing but a malformed file. GcrImage::load() returns false on a bad signature after invalidate() has zeroed the whole track table, but mount_g64() ignored what it returned and inserted the image anyway. Mounting any .g64 whose first eight bytes are not GCR-1541 or GCR-1571 therefore programmed all 84 parameter slots with dummy_track -- 7692 bytes, no slack -- paired with whatever those two stack slots happened to hold. A stale length above that points the drive engine, which reads and writes there, past the end of the heap block. The second is on every image. add_blank_tracks() fills only even indices, so every odd one takes the absent branch and was handed dummy_track together with the preceding track's length. That is safe today by exactly zero margin: the largest entry in track_lengths[] equals GCRIMAGE_DUMMYTRACKLEN. Both loops now build their parameter pair through one function, which gives an absent track the dummy address with the same short, safe length init() and remove_disk() already program, and which cannot see a neighbour at all. A zero or negative length is treated as absent there too, so the division has no way to reach a zero divisor even if a length slips past the checks at load time. mount_g64() now acts on load()'s answer: the file is closed and the drive is left empty, which is the state remove_disk() has already put it in. Reported in GideonZ#823, problems 1 and 3.
94d1312 to
3475e9e
Compare
The first version of this fix compared a declared track length against GCRIMAGE_MAXSIZE, so a track only had to fit the allocation. It did not have to be in the file. A 14 byte GCR-1541 image whose track 0 points at offset 12 and declares 0x1E0C passed both the header check and the length check, and then gcr_data+14 through gcr_data+7705 were programmed into the drive engine -- bytes no read ever wrote, holding whatever the previous mount left behind, since the buffer is zeroed at allocation and not between images. gcr_validated_track_length() now takes bytes_read instead of the capacity and requires offset + 2 + length <= bytes_read, written as subtractions so nothing can wrap, and load() passes what f->read() reported. Since bytes_read never exceeds GCRIMAGE_MAXSIZE, this is strictly the stronger bound and the buffer cases the earlier tests pin down still hold. Five cases added under TruncatedImage, including the reported 14 byte file. Against this commit the suite is 23 green; against a header restored to the capacity bound, 4 of the 5 fail -- the fifth is the positive boundary, a track ending exactly where the file does, which must stay accepted either way. Reported by @chrisgleissner in review of GideonZ#840.
The first version of this fix compared a declared track length against GCRIMAGE_MAXSIZE, so a track only had to fit the allocation. It did not have to be in the file. A 14 byte GCR-1541 image whose track 0 points at offset 12 and declares 0x1E0C passed both the header check and the length check, and then gcr_data+14 through gcr_data+7705 were programmed into the drive engine -- bytes no read ever wrote, holding whatever the previous mount left behind, since the buffer is zeroed at allocation and not between images. gcr_validated_track_length() now takes bytes_read instead of the capacity and requires offset + 2 + length <= bytes_read, written as subtractions so nothing can wrap, and load() passes what f->read() reported. Since bytes_read never exceeds GCRIMAGE_MAXSIZE, this is strictly the stronger bound and the buffer cases the earlier tests pin down still hold. Five cases added under TruncatedImage, including the reported 14 byte file. Against this commit the suite is 23 green; against a header restored to the capacity bound, 4 of the 5 fail -- the fifth is the positive boundary, a track ending exactly where the file does, which must stay accepted either way. Reported by @chrisgleissner in review of GideonZ#840.
4628013 to
b1e6040
Compare
|
You are right, and the hole is exactly where you put it: the check was against the
Red and green, both measured: The four are the truncated cases, your 14-byte file among them, each returning 7692 CI is green on the same commit, and it runs Leaving the workflow-only conflict to you, as you proposed. Ready for another look. |
chrisgleissner
left a comment
chrisgleissner
left a comment
There was a problem hiding this comment.
The new range check is necessary, but one downstream size assumption remains unchecked. I left one blocking inline finding: a short accepted track can still make the MFM mapping path read beyond gcr_data or underflow its writable-space bound.
Length and MFM marker share one word in a G64, so 0x8001 -- one byte of track, marked MFM -- is a legal thing to put in an image, and the bound added in the previous commit passes it: the one declared byte really was delivered. map_gcr_image_to_mfm() then believed the marker and read the whole 162 byte metadata area out of that one byte. Placed at the end of the buffer, as in the reported case -- a file of exactly GCRIMAGE_MAXSIZE with track 0 pointing at GCRIMAGE_MAXSIZE - 3 and a sector count of 32 in the final byte -- the sector loop runs past the allocation. The space behind the area was taken as a subtraction into reservedSpace, a uint32_t that gates every track write, where 1 - 162 cannot come out negative. MFM_TRACK_HEADER_SIZE was also unparenthesised, so x - MFM_TRACK_HEADER_SIZE expanded to x - 2 + 160. Every mapped track reserved 320 bytes more than it had, and the underflow above never happened because the subtraction was not one. The macro's three other uses -- an addition, a comparison, and - 2 -- come out right either way, which is why this stayed hidden. The static_assert beside it fails if the brackets are lost again; comparing the macro against 162 would not, since 2 + 160 equals 162. disk_image.h now includes mfmdisk.h, which defines the WD_MAX_SECTORS_PER_TRACK the macro has always used. A track shorter than the metadata area is left as init() zeroed it: no sectors, no reserved space, so UpdateTrack() admits no write for it. Six cases added under MfmHeader, including the reported end of buffer file. Against this commit the suite is 29 green; with gcr_mfm_reserved_space() restored to a bare subtraction the short track reserves 4294967135 bytes, and with the header check restored to believing the marker the reported case is mapped. Reported by @chrisgleissner in review of GideonZ#840.
|
Confirmed, and closed in The out-of-bounds read. The underflow could not have happened, which is the more interesting half. #define MFM_TRACK_HEADER_SIZE 2 + (WD_MAX_SECTORS_PER_TRACK * 5)so the one place that subtracts it, Parenthesising alone would have created the underflow you predicted; the guard alone would have left the 320 bytes. Both are in. The All 29 tests pass on this commit. To show that is not vacuous, I put each guard back the way it was, one at a time, and reran: In the first revert the one-byte track reserves 4294967135 bytes instead of 0; in the second, the reported file is mapped again. Both reverts were thrown away afterwards; they exist only to prove the new cases bite. CI is green on this commit and ran Adjacent finding, reported separately as #921. While measuring this I noticed that the WRITE TRACK handler discards Scope, so this is not mistaken for an endorsement. Whether a G64 should be carrying MFM tracks at all is a separate question, now raised as #920: bit 15 of the length word is not part of the G64 specification, and VICE rejects such a track outright rather than loading it. This PR takes the mechanism exactly as it stands and only makes it safe to walk over. For the record, my own view is that it should absolutely not stay. A file whose signature reads Ready for another look. |
The length word carries the MFM marker in bit 15, so the length is what the
other fifteen bits say. load() masked with 0x3FFF and took fourteen, and
nothing anywhere gives bit 14 a meaning: it is not a flag in this firmware,
not in Denise, which reads the same marker and masks with 0x7fff, and not in
the published G64 format, where the field is a plain size.
The writers never masked at all. save() at disk_image.cc:847 and write_track()
at :924 both build the word from reported_length >> 8 and then set bit 15 on
top, so an image could not be read back as it was written. A declared 0x4123
came out as 0x0123: 291 bytes, small enough to pass every check in
gcr_validated_track_length() and be programmed as a track.
That is the part worth stating plainly, because the mask defeats the function
it sits in. Read as a length, 0x4123 is 16675, above GCRIMAGE_MAXTRACKLEN, and
the track is refused with the diagnostic this file already prints. Masked, it
became a plausible short track and was programmed instead -- "rejected here
rather than programmed" is what the comment above the function promises.
Where 0x3FFF came from is not a mystery: the drive engine's parameter field
is exactly fourteen bits wide, so masking the declared length to the same
width reads like the obvious thing to do. It holds everywhere except here.
Decoding the file and asking what the drive can do are separate steps. The
drive engine's ceiling is 16383: max_offset in floppy_param_mem.vhd is
fourteen bits wide (line 36), and floppy_mem.vhd wraps offset_count at it for
reads and writes alike. That ceiling never comes into play here, because the
bound this function already applies -- GCRIMAGE_MAXTRACKLEN, 7928 -- is less
than half of it. So the parse can stay faithful to the file and let the bound
do the rejecting, which is what it is for.
Both facts are now written into the comment above the function, so the next
reader does not have to rediscover where 0x3FFF came from.
against this commit 31 passed
against the same header masking with 0x3FFF 2 failed
The two are the new cases, 0x4123 with and without the MFM marker, each
returning 291 where it has to return 0. No hardware is involved: these are
free functions over plain integers, and make -C software/drive/tests runs
them in about a second.
The checks in gcr_track_bounds.h are host tests over plain integers, which is
what makes them worth having, but nothing so far mounts an image that actually
trips them on a device. This suite does, and it is the first thing anywhere to
drive the firmware's mixed GCR/MFM path on real hardware.
Three G71 images, built in the test rather than committed, because what is
interesting about them is the two-byte length word in front of each track and
a binary fixture would hide it:
mixed 35 GCR tracks and one track carrying the MFM payload this
firmware writes -- sector count, version byte of zero, five bytes
per sector, sector data from offset 162 on, exactly as
mfm_update_callback() lays it out.
shortmfm the same disk with the MFM-marked track declaring 0x8001. The
metadata area alone is 162 bytes, so there is nothing to read.
overlong the same disk with a track declaring 0x4123: 16675 read as a
length, 291 read as fourteen bits.
Each case asserts the same two things. The device is still reachable, and
track 18 sector 1 still comes back. A malformed track on track 39 or 40 may
not cost the rest of the disk, and that is the property these bounds are for.
The block comes back through the drive's own U1 command rather than a job
queue, so the reader needs no ROM entry point and does not change when the
drive model does; an I before it makes the DOS re-read the BAM instead of
refusing a block whose header carries an ID it does not expect yet.
The GCR encoder is 30 lines and is checked by its own output: the sentinel
written into track 18 sector 1 decodes back byte for byte through the inverse
table, and the longest run of zero bits in the encoded stream is two, which is
the defining property of the code. Track lengths land inside each speed zone's
nominal size with room to spare -- 6897 of 7142 on track 18.
lint_test OK (ruff 0.16.5 over tests, run-tests)
registry_test OK (71 suites)
runner_policy OK (99 checks)
64tass g71_block_reader.asm assembles, 259 bytes at $0801
Not run on a device yet: the suite is registered and its fixtures are
verified, but the device half needs hardware on the network. Whoever runs it
first should expect the two malformed cases to be the interesting ones.
|
Two commits on top, fast-forward, no force-push — your anchors survive. Bit 14 was a lost length bit, not a flag: New e2e suite It has not run on a device yet and the commit says so outright; my C64U is working again, so that plus the two manual checks #823 asks for are next, and I will report them here. Your second review is answered by |
|
@chrisgleissner Please comment below each of my findings how they were addressed. This will simplify review. Is this merge ready from your perspective and have you performed exhaustive red green tests of it against a U64 and c64? Could you please provide test results? I appreciate your PR description shows these, but I am not sure if they were repeated for the most recent code version, and what their results were. Also, your original PR description still mentions your device is bricked. It would be great if you could update and rewrite your original PR description to reflect the very latest state and latest tests you have performed. Thank you. |
chrisgleissner
added
the
storage
The block reader sent "i0" and "u1 2 0 <track> <sector>" in lower case. 64tass passes .text through as ASCII, $69 and $75, and the DOS knows its commands only as the PETSCII $49 "I" and $55 "U": both came back as 31, SYNTAX ERROR, and the suite failed on master and on this branch alike with dos=$31. In upper case the initialise then freed the buffer channel opened just before it, and U1 answered 70, NO CHANNEL. The initialise now comes before "#" is opened. On an Ultimate II+L in a C64, master 7117d88: 9/9, the three G71s read track 18 sector 1 back.
Addressed in Five cases under |
|
@chrisgleissner Both findings are answered above, and the description is rewritten for the current state, including the old line about a bricked device. Merge-ready from my side, yes. Tested on the current head:
One thing the device run caught: the suite itself failed on both firmwares with |
2 participants
Fixes #823. A G64 or G71 tells the drive engine where each track is and how long it is, and the firmware used to believe it.
insert_disk()programs the drive engine's parameter RAM from the track table, andfloppy_mem.vhdreads and writes memory bounded by what it finds there, so every unchecked value in that table reached the engine.What was wrong
w & 0x3FFFadmitted 16383 whileGCRIMAGE_MAXTRACKLENis 7928, and nothing compared the length with what the file delivered.offset > GCRIMAGE_MAXSIZEadmittedoffset == GCRIMAGE_MAXSIZE, so the length word was read past the buffer; and a track declared zero bytes long reachedrotation_speed / tr->track_length.0x1E0Csentgcr_data+14throughgcr_data+7705to the drive: bytes no read wrote, holding whatever the previous mount left, as the buffer is zeroed at allocation and not between images.0x8001, one byte marked MFM, mademap_gcr_image_to_mfm()read the 162-byte metadata area out of one byte, past the allocation at the end of the buffer, and1 - 162went into theuint32_t reservedSpacethat gates every track write.MFM_TRACK_HEADER_SIZEwas unparenthesised, sox - MFM_TRACK_HEADER_SIZEexpanded tox - 2 + 160: every mapped track reserved 320 bytes more than it had, which is why the underflow never showed.save()andwrite_track()write it that way, and Denise reads it that way. Masked with0x3FFF, a declared0x4123(16675, above the maximum) became 291 and was programmed as a plausible short track.bit_timeandtrack_lenwere assigned only for a present track and read for an absent one, so half-tracks were handeddummy_trackwith the neighbour's length, andmount_g64()ignored whatload()returned.What this does
software/drive/gcr_track_bounds.hholds the checks as free functions over plain integers, so they run on a build host:gcr_track_header_is_readable(): the two-byte length word lies inside what the file delivered.gcr_validated_track_length(): the length from fifteen bits, or 0 when the track cannot be used, for zero, above the format maximum, oroffset + 2 + length > bytes_read, written as subtractions so nothing wraps.gcr_mfm_reserved_space()and the MFM header check: a track shorter thanMFM_TRACK_HEADER_SIZEgets no sectors and no reserved space, soUpdateTrack()admits no write for it. Astatic_assertfails if the macro's brackets go missing again.gcr_track_parameters(): an absent track gets the dummy address with the short, safe lengthinit()andremove_disk()already use.A refused track stays as
invalidate()left it,add_blank_tracks()gives it a blank one, and the mount goes on. A fileload()refuses as a whole leaves the drive empty.Tested
Host tests, red/green.
make -C software/drive/tests, 31 cases, in CI: 31 passed onde633b34. Each guard reverted on its own turns cases red (measured on4c79cd1a;gcr_track_bounds.hand its tests have not changed since): the truncated-file bound 5 (4 of them fail, the fifth is the boundary that must pass either way), the MFM header check 3, the bare subtraction intoreservedSpace3, the fourteen-bit mask 2, the header-readable check 2. The zero-length check has no case of its own that can fail: a zero length is refused by the other bounds too.On a device, an Ultimate II+L in a C64, master
7117d885against a local build of this branch on the same master (e2b9f49f, 2 October; the branch's last firmware change,be8452c5, is from 25 September):g71-mfm-bounds: a mixed GCR/MFM G71, one with a track declaring0x8001, one declaring0x4123; track 18 sector 1 reads back, the device answersU1reads every sector triedGCR-XXXX21, READ ERROR0x3FFFwith all 16383 bytes in the file21, the rest of the disk reads21, the rest readsThe device shows no difference: the over-read these bounds stop goes past the buffer, but the drive gets no usable GCR out of it on either firmware, so the drive answers the same. What the device run shows is that green stays green: the malformed tracks are refused, the rest of each disk reads, and nothing hangs. The red/green proof is the host tests.
The suite failed on both firmwares with
dos=$31untilde633b34: its block reader sent the DOS commands in lower case, which 64tass passes through as ASCII, and initialised the drive after opening the buffer channel, which frees it.Not run on a U64 or a C64 Ultimate. The changed code is the drive emulation every device shares; a run there would show the same, and I can add one if wanted.
Built
make u64iiinghcr.io/gideonz/riscvonde633b34, everyoutput/andresult/undertarget/removed first: exit 0, no warning fromdisk_image.cc,c1541.ccorgcr_track_bounds.h,app_spacePASS (38.6 % free on U64E2_50T, 34.2 % on U64E2_100T),update.ue2andupdate.cfwproduced.