Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ModeSelect6 fails with DEC Alpha system #1402

Closed
kkaempf opened this issue Dec 20, 2023 · 6 comments · Fixed by #1405
Closed

ModeSelect6 fails with DEC Alpha system #1402

kkaempf opened this issue Dec 20, 2023 · 6 comments · Fixed by #1405

Comments

@kkaempf
Copy link
Contributor

kkaempf commented Dec 20, 2023

Info

  • Which version of Pi are you using: 3
  • Which github revision of software: HEAD (tag 23.11.01)
  • Which board version: 2.3B
  • Which computer is the PiSCSI connected to: DEC Personal Workstation 433au, running OpenVMS/Alpha 7.3
  • Which OS you are using (output of 'lsb_release -a'): Debian 11 (bullseye)

Describe the issue

Booting from piscsi fails in the SRM console (aka 'BIOS') with failed to open
Running with debug reveals

[2023-12-20 16:40:20.184] [debug] (ID:LUN 0:0) - Device is executing TestUnitReady ($00)
[2023-12-20 16:40:20.194] [debug] (ID:LUN 0:0) - Device is executing ModeSelect6 ($15)
[2023-12-20 16:40:20.194] [debug] (ID 0) - Error status: Sense Key $05, ASC $26
@kkaempf
Copy link
Contributor Author

kkaempf commented Dec 20, 2023

Digging through the code (and adding more log messages) reveals that ModeSelect6 is called with a page code of 0 (== "supported diagnostic pages") but the code in cpp/devices/scsi_command_util.cpp only recognizes page 3.

@rdmark
Copy link
Member

rdmark commented Dec 21, 2023

Thanks for reporting, and tracking down the code that’s responsible.

The next step here would be to track down what the SCSI specification says about ModeSelect6 page code 0.

We don’t have an active core developer in this project right now, so if you’re up for it we would very much welcome your contribution.

@rdmark
Copy link
Member

rdmark commented Dec 21, 2023

Additionally, it would be helpful if you could test with the previous stable release version, April 2023. There was a good amount of refactoring that happened, which also touched the ModeSelect code.

https://github.com/PiSCSI/piscsi/releases/tag/v23.04.01

@kkaempf
Copy link
Contributor Author

kkaempf commented Dec 21, 2023

v23.04.01 shows the same behaviour.

Digging deeper now and trying to come up with a PR 🤞🏻

@kkaempf kkaempf mentioned this issue Dec 25, 2023
Merged
@kkaempf
Copy link
Contributor Author

kkaempf commented Dec 25, 2023

So turns out it was actually three bugs, one generic, two specific to DEC Alpha, in ModeSelect6 😆

@rdmark
Copy link
Member

rdmark commented Jan 9, 2024 •
edited
Loading

Merged. Thanks for the patches!

@rdmark rdmark closed this as completed Jan 9, 2024
kkaempf added a commit to kkaempf/piscsi that referenced this issue Apr 1, 2024
@kkaempf
Add SetSectorSize test in SCSIHD
09ea598 
Follow-up on PiSCSI#1402, PiSCSI#1405, PiSCSI#1427

This is the first (yet incomplete) test to cover this behavior.

Signed-off-by: Klaus Kämpf <kkaempf@gmail.com>
kkaempf added a commit to kkaempf/piscsi that referenced this issue Apr 1, 2024
@kkaempf
Add test for ModeSelect page 1
b3c6f8c 
Follow up on PiSCSI#1402, PiSCSI#1405 which had a full test case missing.

Signed-off-by: Klaus Kämpf <kkaempf@gmail.com>
kkaempf added a commit to kkaempf/piscsi that referenced this issue Apr 1, 2024
@kkaempf
Fix length calculation in scsi_command_util::ModeSelect
73917bc 
OpenVMS Alpha sends a strange ModeSelect payload, apparently one byte
too large. This was 'fixed' by a (wrong) length calculation in PiSCSI#1405, breaking PiSCSI#1427.

This PR
- fixes the wrong length calculation
- improves the loop test in scsi_command_util::ModeSelect to prevent a
  buffer overflow. (Remaining length was checked for > 0, but buffer
  access is at offset and offset + 1, effectively requiring 2 bytes.)
- the loop test fix makes PiSCSI#1402 pass
- adds a testcase for PiSCSI#1402
- adds a testcase for PiSCSI#1427

Fixes issue PiSCSI#1427

Signed-off-by: Klaus Kämpf <kkaempf@gmail.com>
@kkaempf kkaempf mentioned this issue Apr 1, 2024
Closed
@rdmark rdmark mentioned this issue Apr 4, 2024
Closed
kkaempf added a commit to kkaempf/piscsi that referenced this issue Aug 9, 2024
@kkaempf
Multiple fixes for ModeSelect (PiSCSI#1405)
0d72614 
* Allow 'empty' ModeSelect6

tl;dr

Treat a computed length of 0 as `has_valid_page_code`.

Details:

The SRM console (aka 'BIOS') of DEC Alpha sends an empty
ModeSelect6 with the following data:
~~~
ModeSelect6, CDB $151000000c00
~~~

That makes 12 byte(s) as follows
~~~
  0  1  2  3   4   5  6  7   8   9 10 11
 00 00 00 08  00  00 00 00  00  00 02 00
~~~

decoding it (accoring to [1], Section 8.3.3, Table 94) gives us

Mode Data Length 0
Medium Type      0
Device-specific  0
Block desc len   8

Density Code     0
Number of blks   0
Reserved         0
Block length     512

`scsi_command_util::ModeSelect` computes
~~~
offset = 4 + buf[3];
~~~

giving 12 and

~~~
length -= offset;
~~~

giving 0.

Thus it never enters the `while` loop and `has_valid_page_code` stays
`false`, raising an error.

[1] [Small Computer System Interface - 2 rev 10L.pdf](https://dn790004.ca.archive.org/0/items/SCSISpecificationDocumentsSCSIDocuments/Small%20Computer%20System%20Interface%20-%202%20rev%2010L.pdf)

Signed-off-by: Klaus Kämpf <kkaempf@gmail.com>

* Allow ModeSelect with page code 1

OpenVMS Alpha (the operating system, not the SRM BIOS) uses
ModeSelect6 with a page code of 1.

The semantics are unknown, just accepting it works for me.

Signed-off-by: Klaus Kämpf <kkaempf@gmail.com>

* Fix page length computation in ModeSelect

tl;dr

The 'skip to next ModeSelect page' computation was off-by-one, either not
taking the page code itself into account or missing the fact that the
page length is given as `n - 1`.

Fix:

Add 1 to the computed length.

Details:

OpenVMS Alpha sends a ModeSelect6 as follows
~~~
command:

ModeSelect6, CDB $151000001900

payload:

 0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24
00 00 00 08 00 00 00 00 00 00 02 00 01 0a 24 00 00 00 00 00 00 00 00 00 00
~~~

This translates to (accoring to [1], Section 8.3.3)

~~~
Mode Data Length 0
Medium Type      0
Device-specific  0
Block desc len   8
~~~

with the following offset / length computation _before_ the `while` loop
~~~
offset = 12
length = 13
~~~

The first payload section is
~~~
 4  5  6  7  8  9 10 11
00 00 00 00 00 00 02 00
~~~

translating to

~~~
Density Code     0
Number of blks   0
Reserved         0
Block length   0x200 512
~~~

Then follows a pagecode 1 as
~~~
12 13 14 15 16 17 18 19 20 21 22 23 24
01 0a 24 00 00 00 00 00 00 00 00 00 00
~~~

translating to
~~~~
Page code        1
Page length -1  10
Mode parameters 24 00 00 00 00 00 00 00 00 00 00
~~~

computing (inside the `while` loop, as `// Advance to the next page`)

~~~
size =  10 + 2 = 12
~~~

followed by new `offset` and `length` values

~~~
offset = 25
length = 1
~~~

So it stays in the `while` loop (and has a larger-than-buffer `offset`
value)

Signed-off-by: Klaus Kämpf <kkaempf@gmail.com>

Fix length calculation in scsi_command_util::ModeSelect

OpenVMS Alpha sends a strange ModeSelect payload, apparently one byte
too large. This was 'fixed' by a (wrong) length calculation in PiSCSI#1405, breaking PiSCSI#1427.

This PR
- fixes the wrong length calculation
- improves the loop test in scsi_command_util::ModeSelect to prevent a
  buffer overflow. (Remaining length was checked for > 0, but buffer
  access is at offset and offset + 1, effectively requiring 2 bytes.)
- the loop test fix makes PiSCSI#1402 pass
- adds a testcase for PiSCSI#1402
- adds a testcase for PiSCSI#1427

Fixes issue PiSCSI#1427

Signed-off-by: Klaus Kämpf <kkaempf@gmail.com>

Improve buffer overflow checking in scsi_command_util::ModeSelect

Signed-off-by: Klaus Kämpf <kkaempf@gmail.com>
kkaempf added a commit to kkaempf/piscsi that referenced this issue Aug 9, 2024
@kkaempf
Fix length calculation in scsi_command_util::ModeSelect
44162b4 
OpenVMS Alpha sends a strange ModeSelect payload, apparently one byte
too large. This was 'fixed' by a (wrong) length calculation in PiSCSI#1405, breaking PiSCSI#1427.

This PR
- fixes the wrong length calculation
- improves the loop test in scsi_command_util::ModeSelect to prevent a
  buffer overflow. (Remaining length was checked for > 0, but buffer
  access is at offset and offset + 1, effectively requiring 2 bytes.)
- the loop test fix makes PiSCSI#1402 pass
- adds a testcase for PiSCSI#1402
- adds a testcase for PiSCSI#1427

Fixes issue PiSCSI#1427

Signed-off-by: Klaus Kämpf <kkaempf@gmail.com>
kkaempf added a commit to kkaempf/piscsi that referenced this issue Aug 9, 2024
@kkaempf
Add test for ModeSelect page 1
f7dcc5b 
Follow up on PiSCSI#1402, PiSCSI#1405 which had a full test case missing.

Signed-off-by: Klaus Kämpf <kkaempf@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Multiple fixes for ModeSelect kkaempf/piscsi
2 participants
@kkaempf @rdmark