search

PiSCSI / piscsi

PiSCSI allows a Raspberry Pi to function as emulated SCSI devices (hard disk, CD-ROM, and others) for vintage SCSI-based computers and devices. This is a fork of the RaSCSI project by GIMONS.
https://piscsi.org
BSD 3-Clause "New" or "Revised" License
545 stars 82 forks source link

MODE SELECT crashes in various scenarios #1426

Closed uweseimet closed 7 months ago

uweseimet commented 10 months ago

This ticket refers to commit a23d642 (current develop branch).

While testing new tools for the PiSCSI board I stumbled upon some bugs in the mode page handling.

When you try to change the sector size of a CD-ROM drive with no medium inserted piscsi crashes with a assertion.

When a medium is present, changing the sector size to 512 bytes works fine with MODE SELECT(6) and these command parameters for the DATA OUT phase:

00:00:00:08:00:00:00:00:00:00:02:00

When doing the same with MODE SELECT(10) piscsi crashes with an exception. The command used was equivalent to the MODE SELECT(6) above, i.e. it should have the same effect and work. These were the command parameters for DATA OUT:

00:00:00:00:00:00:00:08:00:00:00:00:00:00:02:00

MODE SELECT(6) crashes with an exception when trying to set a valid but unsupported sector size:

00:00:00:08:00:00:00:00:00:00:04:00

It also crashes when the requested sector size is not a multiple of 512:

00:00:00:08:00:00:00:00:00:01:02:01

I also tested with a real CD-ROM drive, where everything worked as expected, including the error handling. Note that the real drive does not require the SP bit to be set when changing the sector size, whereas piscsi requires this bit to be set.

rdmark commented 9 months ago

@uweseimet Thanks for reporting the bugs / corner cases. What do you recommend is a proper handling of the two latter invalid inputs? Ignore the command, log a warning and continue?

@kkaempf This has to do with the CD-ROM sector size changing functionality. Several scenarios that we didn't consider. Would you be able to spend a little time adding some more logic here?

uweseimet commented 9 months ago

@rdmark Regarding the invalid input the commands should be rejected with INVALID FIELD IN PARAMETER LIST. Also see the SCSI specification. This is also what a real drive does.

uweseimet commented 9 months ago

By the way, lauching piscsi with the requested sector size for the respective drive (-b option) would quite likely also work, without any code change in the v23.11.01 codebase, or at least with a smaller change.

rdmark commented 9 months ago

@kkaempf I plan to tag a new stable piscsi software release in 2 weeks time. Do you think you'll have a chance to look at this before that?

kkaempf commented 9 months ago

I fail to reproduce these issues with added tests - what am I missing ?

uweseimet commented 9 months ago

Are you running your tests with assertions enabled?

kkaempf commented 9 months ago

Are you running your tests with assertions enabled?

🤦🏻 - I only compiled the test/*cpp files with DEBUG.

Now I see the "no medium in drive" problem - it's the DiskCache initializer checking blocks > 0 - to be expected.

I'll skip DiskCache initialization if no medium (== image size) is known.

uweseimet commented 9 months ago

I re-opened the ticket, because of the 3 bugs in mentioned in this ticket only 1 appears to have been addressed ...

kkaempf commented 9 months ago

I'm unable to reproduce the other bugs and would need some advice.

uweseimet commented 9 months ago

I have provided the data to use for MODE SELECT in the ticket. With these data you can reproduce the crashes or assertions. When you add unit tests with these data you should also get them. And MODE SELECT(10) does not work the same way as MODE SELECT(6) anymore, which is also not correct.

uweseimet commented 9 months ago

I just double checked and can reproduce everything with the latest develop branch. With these data for the DATA IN phase of MODE SELECT(6) for a device of type SCCD:

00:00:00:08:00:00:00:00:00:00:04:00

the result is:

terminate called after throwing an instance of 'io_exception'
  what():  Invalid sector size of 1024 byte(s)
Aborted

With:

00:00:00:08:00:00:00:00:00:01:02:01
terminate called after throwing an instance of 'io_exception'
  what():  Invalid sector size of 513 byte(s)
Aborted

Further, from my tests it looks as if when using MODE SELECT(10) instad of MODE SELECT(6) the sector size cannot be changed at all, even though MODE SELECT(10) must work like MODE SELECT(6). It is just an extended version.

Regarding the SP bit, the difference between a real drive and piscsi is a bit different than I initially said, but my note on this is essentially the same: The real drive changes the sector size regardless of whether the SP bit is set or not. piscsi reports INVALID FIELD IN CDB if the SP bit is set. Is this difference intended?

kkaempf commented 9 months ago

I'm still confused. Running scsicd_test on latest develop gives me

[2024-02-27 13:46:20.075] [warning] (ID -1) - Unknown MODE SELECT page code: $00
[2024-02-27 13:46:20.075] [debug] (ID -1) - Changing sector size from 2048 to 512
[2024-02-27 13:46:20.075] [warning] (ID -1) - Unknown MODE SELECT page code: $00
[2024-02-27 13:46:20.075] [debug] (ID -1) - Changing sector size from 512 to 2048
[2024-02-27 13:46:20.075] [warning] (ID -1) - Unknown MODE SELECT page code: $00
[2024-02-27 13:46:20.075] [debug] (ID -1) - Changing sector size from 2048 to 512
[2024-02-27 13:46:20.075] [warning] (ID -1) - Unknown MODE SELECT page code: $00
[2024-02-27 13:46:20.075] [debug] (ID -1) - Changing sector size from 512 to 1024
[2024-02-27 13:46:20.075] [debug] (ID -1) - Changing sector size from 512 to 768
[2024-02-27 13:46:20.075] [debug] (ID -1) - Changing sector size from 2048 to 512
[2024-02-27 13:46:20.075] [warning] (ID -1) - Unknown MODE SELECT page code: $00

There are three 2048 to 512 byte switches. The first is via ModeSelect6, the second via ModeSelect10, and the last via ModeSelect6 again ('no media' case).

So the ModeSelect10 case seems to be working.

kkaempf commented 9 months ago

For the "non-multiple of 512 bytes" cases I'd assume that you don't want an exception raised ?!

uweseimet commented 9 months ago

Yes, I can confirm that the MODE SELECT(10) case indeed appears to be working. But just like MODE SELECT(6) it crashes with the sector sizes from my examples above:

2024-02-27 20:23:52.468] [debug] (ID:LUN 0:0) - Changing sector size from 2048 to 1024
terminate called after throwing an instance of 'io_exception'
  what():  Invalid sector size of 1024 byte(s)
Aborted

My bug reports are not about what I want, but more about crashes and what the SCSI spec says. piscsi terminating is something nobody would want ;-). I have provided all MODE SELECT data I have used to reproduce the crashes (v23.11.01 does not crash), and I have shown the piscsi crash messages, I'm sorry, but I don't think I can do more than that. Just use the same data for your tests and I am sure you will see the same issues.

rdmark commented 8 months ago

@kkaempf So ultimately I think it's up to us to decide if we want to treat "non-multiple of 512 bytes" cases as exceptions, or to log a warning and proceed. I'm ok either way. It seems like a highly unlikely case in real life. And if it turns out users encounter this scenario, they are surely to report it if we keep the exception.

Please let me know what you think is the best solution.

kkaempf commented 8 months ago

@rdmark - frankly, I'm at a loss with this one. The scsicd test checks for a raised exception for unsupported sector sizes. What else should SCSICD::ModeSelect (being void) do ?

uweseimet commented 8 months ago

@kkaempf It should react to unsupported command parameters according to the SCSI specification. A crash (an uncaught exception is a crash, just like a segmentation fault) means something is broken. Does you real hard drive or CD-ROM drive crash when it receives a command or parameters it does not support? Do you have to switch your drives off and on in such a case? I bet not ;-).

@rdmark I don't think it is a good idea to define an uncaught exception, i.e. a crash, as a valid reaction on a SCSI command. But If you are OK with a change for an exotic platform triggering several reproducable bugs and crashing piscsi, good luck!

rdmark commented 8 months ago

@kkaempf Wouldn't you say that this scenario corresponds to condition d) found under the 8.2.8 MODE SELECT(6) command section from the SCSI spec?

The target shall terminate the MODE SELECT command with CHECK CONDITION status, set the sense key to ILLEGAL REQUEST, set the additional sense code to INVALID FIELD IN PARAMETER LIST, and shall not change any mode parameters for the following conditions: a) If the initiator sets any field that is reported as not changeable by the target to a value other than its current value. b) If the initiator sets any field in the mode parameter header or block descriptor(s) to an unsupported value. c) If an initiator sends a mode page with a page length not equal to the page length returned by the MODE SENSE command for that page. d) If the initiator sends a unsupported value for a mode parameter and rounding is not implemented for that mode parameter. e) If the initiator sets any reserved field in the mode parameter list to a non-zero value.

Right now we allow the change to an invalid sector size and then bail out with this assertion:

https://github.com/PiSCSI/piscsi/blob/dd9a3296d4f0060b923bf1297c5849cfae297333/cpp/devices/disk.cpp#L699

So I think we should detect the invalid sector size earlier and, for instance, add a check in SCSICD::ModeSelect before calling SetSectorSizeInBytes(), along the lines of...

if (!GetSupportedSectorSizes().contains(size_in_bytes)) {
    throw scsi_exception(sense_key::illegal_request, asc::invalid_field_in_parameter_list);
}

What do you think?

rdmark commented 7 months ago

Whole changeset reverted with https://github.com/PiSCSI/piscsi/pull/1451