piscsi: MODE SELECT crashes in various scenarios

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.

About this issue

Original URL
State: closed
Created 5 months ago
Comments: 19 (19 by maintainers)

Commits related to this issue

Don't resize DiskCache without a file DiskCache computes its size from a file. Skip ResizeCache Without file (i.e. 'empty' CD) Fixes #1426 Signed-off-by: Klaus Kämpf <kkaempf@gmail.com> — committed to kkaempf/piscsi by kkaempf 4 months ago
Don't resize DiskCache without a file DiskCache computes its size from a file. Skip ResizeCache Without file (i.e. 'empty' CD) Fixes #1426 Signed-off-by: Klaus Kämpf <kkaempf@gmail.com> — committed to kkaempf/piscsi by kkaempf 4 months ago
Don't ResizeCache on sector change if no filename is defined (#1438) * Add tests for issue#1426 Signed-off-by: Klaus Kämpf <kkaempf@gmail.com> * add test without medium Signed-off-by: Klaus ... — committed to PiSCSI/piscsi by kkaempf 4 months ago
Don't ResizeCache on sector change if no filename is defined (#1438) * Add tests for issue#1426 Signed-off-by: Klaus Kämpf <kkaempf@gmail.com> * add test without medium Signed-off-by: Klaus ... — committed to kkaempf/piscsi by kkaempf 4 months ago
Don't ResizeCache on sector change if no filename is defined (#1438) * Add tests for issue#1426 Signed-off-by: Klaus Kämpf <kkaempf@gmail.com> * add test without medium Signed-off-by: Klaus ... — committed to PiSCSI/piscsi by kkaempf 4 months ago

Most upvoted comments

@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 on Mar 21, 2024

@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?

rdmark on Feb 18, 2024

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.

uweseimet on Feb 2, 2024

@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 on Feb 2, 2024

@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?

rdmark on Feb 2, 2024