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.

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

Multiple fixes for ModeSelect #1405

Merged
merged 25 commits into from
Jan 7, 2024
Merged

Multiple fixes for ModeSelect #1405

merged 25 commits into from
Jan 7, 2024

Conversation

kkaempf
Copy link
Contributor

@kkaempf kkaempf commented Dec 25, 2023

This PR covers multiple issues

  • The DEC Alpha SRM console (aka BIOS) calls ModeSelect6 with no payload

  • OpenVMS Alpha calls ModeSelect6 with a page code of 1

  • ModeSelect had an off-by-one error in computing the length

Fixes #1402

Tony Kuker and others added 20 commits August 1, 2022 02:21
RaSCSI version 22.08.01
RaSCSI version 22.10.01
PiSCSI version 22.12.1
PiSCSI Version 23.02.01
PiSCSI version 23.04.01
PiSCSI version 23.11.01
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Copy link
Member

@rdmark rdmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look reasonable to me, given your reading of the SCSI spec.

Can you please update the unit tests to get them passing with this new interpretation of the expected behavior? Please let us know if you need help getting set up with googletest locally.

@kkaempf
Copy link
Contributor Author

kkaempf commented Dec 27, 2023

@rdmark frankly, I'm at loss with fixing the tests :-( Is there any documentation ?

@rdmark
Copy link
Member

rdmark commented Dec 28, 2023

@kkaempf The closest we have to documentation is the issue ticket where the tests were initially introduced! #794

Running the tests should be as easy as installing the googletest libraries, and then running make test. You can also see the failing CI jobs here in this PR.

The actual test code should be fairly self-explanatory I think, but let me know if you need further pointers ;)

@kkaempf
Copy link
Contributor Author

kkaempf commented Jan 4, 2024

The test output is quite unhelpful 😞

[ RUN      ] ScsiCommandUtilTest.ModeSelect6
unknown file: Failure
C++ exception with description "std::exception" thrown in the test body.
[  FAILED  ] ScsiCommandUtilTest.ModeSelect6 (0 ms)

Signed-off-by: Klaus Kämpf <[email protected]>
Signed-off-by: Klaus Kämpf <[email protected]>
@kkaempf
Copy link
Contributor Author

kkaempf commented Jan 4, 2024

Tests fixed (somewhat hackish imho)

@kkaempf kkaempf requested a review from rdmark January 4, 2024 16:16
@rdmark
Copy link
Member

rdmark commented Jan 5, 2024

@kkaempf What makes you say the tests are hackish?

@kkaempf
Copy link
Contributor Author

kkaempf commented Jan 5, 2024

@kkaempf What makes you say the tests are hackish?

I can't say (yet ?) that I actually know what I'm doing there 🙈

Signed-off-by: Klaus Kämpf <[email protected]>
@kkaempf
Copy link
Contributor Author

kkaempf commented Jan 5, 2024

This should be complete now I believe.

Copy link
Member

@rdmark rdmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

@rdmark rdmark changed the base branch from main to develop January 7, 2024 04:33
@rdmark
Copy link
Member

rdmark commented Jan 7, 2024

Rebasing with the develop branch. This is where all new code goes. We then rebase with main after each stable release.

Copy link

sonarcloud bot commented Jan 7, 2024

@rdmark rdmark merged commit ad5eae9 into PiSCSI:develop Jan 7, 2024
8 checks passed
@kkaempf kkaempf deleted the issue-1402 branch January 7, 2024 12:48
kkaempf added a commit to kkaempf/piscsi that referenced this pull request Apr 1, 2024
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 <[email protected]>
kkaempf added a commit to kkaempf/piscsi that referenced this pull request Apr 1, 2024
Follow up on PiSCSI#1402, PiSCSI#1405 which had a full test case missing.

Signed-off-by: Klaus Kämpf <[email protected]>
kkaempf added a commit to kkaempf/piscsi that referenced this pull request Apr 1, 2024
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 <[email protected]>
kkaempf added a commit to kkaempf/piscsi that referenced this pull request Apr 2, 2024
* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>
rdmark added a commit that referenced this pull request Apr 9, 2024
rdmark added a commit that referenced this pull request Apr 13, 2024
…ging (#1451)

* Revert "Don't ResizeCache on sector change if no filename is defined (#1438)"

This reverts commit dd9a329.

* Revert "Add ModeSense page 0x25 (DEC special function control page) (#1412)"

This reverts commit 1121b8d.

* Revert "DiskCache needs a size"

This reverts commit 7cc8df2.

* Revert "Honor sector size change via ModeSelect6 in scsicd (#1406)"

This reverts commit b7f65d3.

* Revert "Multiple fixes for ModeSelect (#1405)"

This reverts commit ad5eae9.
rdmark pushed a commit that referenced this pull request May 1, 2024
* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>
rdmark added a commit that referenced this pull request May 1, 2024
…ging (#1451)

* Revert "Don't ResizeCache on sector change if no filename is defined (#1438)"

This reverts commit dd9a329.

* Revert "Add ModeSense page 0x25 (DEC special function control page) (#1412)"

This reverts commit 1121b8d.

* Revert "DiskCache needs a size"

This reverts commit 7cc8df2.

* Revert "Honor sector size change via ModeSelect6 in scsicd (#1406)"

This reverts commit b7f65d3.

* Revert "Multiple fixes for ModeSelect (#1405)"

This reverts commit ad5eae9.
kkaempf added a commit to kkaempf/piscsi that referenced this pull request Aug 9, 2024
* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

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 <[email protected]>

Improve buffer overflow checking in scsi_command_util::ModeSelect

Signed-off-by: Klaus Kämpf <[email protected]>
kkaempf added a commit to kkaempf/piscsi that referenced this pull request Aug 9, 2024
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 <[email protected]>
kkaempf added a commit to kkaempf/piscsi that referenced this pull request Aug 9, 2024
Follow up on PiSCSI#1402, PiSCSI#1405 which had a full test case missing.

Signed-off-by: Klaus Kämpf <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ModeSelect6 fails with DEC Alpha system
4 participants