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

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

[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.

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 <bart.vanassche@gmail.com>
Cc: James E.J. Bottomley <James.Bottomley@suse.de>
Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Brian King <brking@linux.vnet.ibm.com>

diff --git a/drivers/scsi/libsrp.c b/drivers/scsi/libsrp.c
index 9ad38e8..710919f 100644
--- a/drivers/scsi/libsrp.c
+++ b/drivers/scsi/libsrp.c
@@ -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)

Re: [PATCH] SCSI/libsrp: fix bug in ADDITIONAL CDB LENGTH interpretation

[Matthew Wilcox]

On Wed, Dec 09, 2009 at 07:52:19PM +0100, Bart Van Assche wrote:
> 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.

> -	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;

-- 
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."

Re: [PATCH] SCSI/libsrp: fix bug in ADDITIONAL CDB LENGTH interpretation

[Bart Van Assche]

On Wed, Dec 9, 2009 at 8:04 PM, Matthew Wilcox <matthew@wil.cx> wrote:
>
> On Wed, Dec 09, 2009 at 07:52:19PM +0100, Bart Van Assche wrote:
> > 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.
>
> > -     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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SCSI/libsrp: fix bug in ADDITIONAL CDB LENGTH interpretation

[FUJITA Tomonori]

On Wed, 9 Dec 2009 19:52:19 +0100
Bart Van Assche <bart.vanassche@gmail.com> wrote:

> 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 <bart.vanassche@gmail.com>
> Cc: James E.J. Bottomley <James.Bottomley@suse.de>
> Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Brian King <brking@linux.vnet.ibm.com>

Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

Thanks,

Re: [PATCH] SCSI/libsrp: fix bug in ADDITIONAL CDB LENGTH interpretation

[Bart Van Assche]

On Thu, Dec 10, 2009 at 12:37 AM, FUJITA Tomonori
<fujita.tomonori@lab.ntt.co.jp> wrote:
>
> On Wed, 9 Dec 2009 19:52:19 +0100
> Bart Van Assche <bart.vanassche@gmail.com> wrote:
>
> > 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 <bart.vanassche@gmail.com>
> > Cc: James E.J. Bottomley <James.Bottomley@suse.de>
> > Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Cc: Brian King <brking@linux.vnet.ibm.com>
>
> Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html