SCSI/libsrp: fix bug in ADDITIONAL CDB LENGTH interpretation

PATCH In review Bart Van Assche 3 participants 5 messages Dec 9, 2009 – Jan 19, 2010 SCSI

Share & tools

Bart Van Assche

author

2009-12-09 18:52 UTC #0 lore raw

PATCH 1/1 SCSI/libsrp: fix bug in ADDITIONAL CDB LENGTH interpretation

Fix a bug in the interpretation of the ADDITIONAL CDB LENGTH (add_cdb_len) field of SRP_CMD requests. According to the SRP specification, the layout of this single-byte field is as follows:
* Bits 0 and 1 are reserved.
* Bits 2 to 7 represent the ADDITIONAL CDB LENGTH field, symbolically represented as n.
* Still according to the SRP specification, the ADDITIONAL CDB section takes 4*n bytes.

Currently libsrp is only used by the ibmvscsi driver. Since the ibmvscsi driver doesn't support large CDB's, this bug hasn't caused any problems yet.

Signed-off-by Bart Van Assche <[email protected]>

Cc James E.J. Bottomley <[email protected]>

Cc FUJITA Tomonori <[email protected]>

Cc Brian King <[email protected]>

drivers/scsi/libsrp.c +2 -2

index 9ad38e8..710919f 100644
@@ -327,7 +327,7 @@ int srp_transfer_data(struct scsi_cmnd *sc, struct srp_cmd *cmd,
 	int offset, err = 0;
 	u8 format;
 
-	offset = cmd->add_cdb_len * 4;
+	offset = (cmd->add_cdb_len >> 2) * 4;
 
 	dir = srp_cmd_direction(cmd);
 	if (dir == DMA_FROM_DEVICE)

@@ -365,7 +365,7 @@ static int vscsis_data_length(struct srp_cmd *cmd, enum dma_data_direction dir)
 {
 	struct srp_direct_buf *md;
 	struct srp_indirect_buf *id;
-	int len = 0, offset = cmd->add_cdb_len * 4;
+	int len = 0, offset = (cmd->add_cdb_len >> 2) * 4;
 	u8 fmt;
 
 	if (dir == DMA_TO_DEVICE)

Matthew Wilcox

2009-12-09 19:04 UTC #1 lore raw

Quoted 8 lines · Bart Van Assche

Fix a bug in the interpretation of the ADDITIONAL CDB LENGTH (add_cdb_len) field of SRP_CMD requests. According to the SRP specification, the layout of this single-byte field is as follows:
* Bits 0 and 1 are reserved.
* Bits 2 to 7 represent the ADDITIONAL CDB LENGTH field, symbolically represented as n.
* Still according to the SRP specification, the ADDITIONAL CDB section takes 4*n bytes.

Your interpretation of the SRP spec does seem to be correct, and I can totally see how the original author of this code got it wrong.

drivers/scsi/libsrp.c int srp_transfer_data(struct scsi_cmnd *sc, struct srp_cmd *cmd, · line 330

Quoted 2 lines

drivers/scsi/libsrp.c +1 -1

-	offset = cmd->add_cdb_len * 4;
+	offset = (cmd->add_cdb_len >> 2) * 4;

Would this not be better written as:

	offset = cmd->add_cdb_len & ~3;

Signature · 4 lines

Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Bart Van Assche

author

2009-12-09 19:17 UTC #2 lore raw

Quoted 14 lines · Matthew Wilcox

Quoted 8 lines · Bart Van Assche
Fix a bug in the interpretation of the ADDITIONAL CDB LENGTH (add_cdb_len) field of SRP_CMD requests. According to the SRP specification, the layout of this single-byte field is as follows:
* Bits 0 and 1 are reserved.
* Bits 2 to 7 represent the ADDITIONAL CDB LENGTH field, symbolically represented as n.
* Still according to the SRP specification, the ADDITIONAL CDB section takes 4*n bytes.
Your interpretation of the SRP spec does seem to be correct, and I can totally see how the original author of this code got it wrong.

Quoted 6 lines

Quoted 2 lines
- offset = cmd->add_cdb_len * 4;
+ offset = (cmd->add_cdb_len >> 2) * 4;
Would this not be better written as:
offset = cmd->add_cdb_len & ~3;

I chose the former because of better readability for someone familiar with the specs. But I do not have a strong opinion about this -- more opinions are welcome.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

FUJITA Tomonori

Acked-by on v1 1/1

2009-12-09 23:37 UTC #3 lore raw

In reply to Bart Van Assche

Quoted 9 lines · On Wed, 9 Dec 2009 19:52:19 +0100 Bart Van Assche <[email protected]>…

Fix a bug in the interpretation of the ADDITIONAL CDB LENGTH (add_cdb_len) field of SRP_CMD requests. According to the SRP specification, the layout of this single-byte field is as follows:
* Bits 0 and 1 are reserved.
* Bits 2 to 7 represent the ADDITIONAL CDB LENGTH field, symbolically represented as n.
* Still according to the SRP specification, the ADDITIONAL CDB section takes 4*n bytes.

Quoted 7 lines

Currently libsrp is only used by the ibmvscsi driver. Since the ibmvscsi driver doesn't support large CDB's, this bug hasn't caused any problems yet.
Signed-off-by Bart Van Assche <[email protected]>
Cc James E.J. Bottomley <[email protected]>
Cc FUJITA Tomonori <[email protected]>
Cc Brian King <[email protected]>

Acked-by FUJITA Tomonori <[email protected]>

Thanks,

Bart Van Assche

author

2010-01-19 07:04 UTC #4 lore raw

Quoted 16 lines · FUJITA Tomonori

Quoted 12 lines · On Wed, 9 Dec 2009 19:52:19 +0100 Bart Van Assche <[email protected]>…
Fix a bug in the interpretation of the ADDITIONAL CDB LENGTH (add_cdb_len) field of SRP_CMD requests. According to the SRP specification, the layout of this single-byte field is as follows:
* Bits 0 and 1 are reserved.
* Bits 2 to 7 represent the ADDITIONAL CDB LENGTH field, symbolically represented as n.
* Still according to the SRP specification, the ADDITIONAL CDB section takes 4*n bytes.
Currently libsrp is only used by the ibmvscsi driver. Since the ibmvscsi driver doesn't support large CDB's, this bug hasn't caused any problems yet.

Quoted 6 lines

Quoted 4 lines
Signed-off-by Bart Van Assche <[email protected]>
Cc James E.J. Bottomley <[email protected]>
Cc FUJITA Tomonori <[email protected]>
Cc Brian King <[email protected]>
Acked-by FUJITA Tomonori <[email protected]>

Hello James,

In which kernel version will this patch be included ?

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html