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

LBA endian error for SCSIOP_{READ,WRITE}6 #889

Open
chrehrhardt opened this issue Mar 4, 2023 · 4 comments
Open

LBA endian error for SCSIOP_{READ,WRITE}6 #889

chrehrhardt opened this issue Mar 4, 2023 · 4 comments
Assignees
Labels

Comments

@chrehrhardt
Copy link

In viostor/virtio_stor_hw_helper.c:RhelGetLba() the case for 6-byte operations looks like this:

        EIGHT_BYTE lba;
        [...]
        case SCSIOP_READ6:
        case SCSIOP_WRITE6: {
            lba.Byte0 = Cdb->CDB6READWRITE.LogicalBlockMsb1;
            lba.Byte1 = Cdb->CDB6READWRITE.LogicalBlockMsb0;
            lba.Byte2 = Cdb->CDB6READWRITE.LogicalBlockLsb;

From reading the code and the other cases it very much looks as if this gets the endianness wrong.
The LSB should go into Byte0 not the MSB.

As this does not seem to cause issues in real life, I'd assume that Windows does not uses these SCSI opcodes.

@vrozenfe
Copy link
Collaborator

vrozenfe commented Mar 4, 2023

Windows does support 6-byte CDBs, but viostor doesn't.
viostor StartIo routine completes 6-byte CDBs as SRB_STATUS_INVALID_REQUEST

Do you have any particular reason for sending SCSIOP_READ6/SCSIOP_WRITE6 requests
to viostor miniport?

Best,
Vadim

@chrehrhardt
Copy link
Author

chrehrhardt commented Mar 4, 2023

No, I was just reading code because I'm trying to do something similar. And the code looks more or less obviously wrong, so I thought I'd let you know. I didn't realize that the code is in fact unreachable.

@vrozenfe
Copy link
Collaborator

vrozenfe commented Mar 5, 2023

This piece of code is quite old, Definitely needs to be cleaned up.
Thank you for catching that.
Vadim.

@YanVugenfirer
Copy link
Collaborator

Reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants