Re: [PATCH 3/3] Farther clean up of stex.c driver

Re: [PATCH 3/3] Farther clean up of stex.c driver

[Boaz Harrosh]

>From 2b82909202cab8dc35184daef45b4b388f93112a Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <bharrosh@bh-buildlin2.(none)>
Date: Thu, 14 Jun 2007 19:14:40 +0300
Subject: [PATCH] Farther clean up of stex.c driver

 - now that scsi-ml accessors do not allow modifying of sg_count bufflen and
   sglist. The code in question is open coded to directly hack the scsi_cmnd
   structure. When implementation changes the driver will need to change with it.

 - Maintainer of this driver should please review again if we absolutely need this
   read-sense length fixing. What if less bytes are read and 0 are left at the end.
   Also no check is made if the buffer is large enough.
---
 drivers/scsi/stex.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
index adda296..d784b58 100644
--- a/drivers/scsi/stex.c
+++ b/drivers/scsi/stex.c
@@ -719,7 +719,7 @@ static void stex_ys_commands(struct st_hba *hba,

 	if (ccb->cmd->cmnd[0] == MGT_CMD &&
 		resp->scsi_status != SAM_STAT_CHECK_CONDITION) {
-		scsi_bufflen(ccb->cmd) =
+		ccb->cmd->request_bufflen =
 			le32_to_cpu(*(__le32 *)&resp->variable[0]);
 		return;
 	}
-- 
1.5.0.4.402.g8035

RE: [PATCH 3/3] Farther clean up of stex.c driver

[Ed Lin]

> -----Original Message-----
> From: Boaz Harrosh [mailto:bharrosh@panasas.com] 
> Sent: Thursday, June 14, 2007 9:34 AM
> To: FUJITA Tomonori; Ed Lin; Guennadi Liakhovetski
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 3/3] Farther clean up of stex.c driver
> 
> 
> From 2b82909202cab8dc35184daef45b4b388f93112a Mon Sep 17 00:00:00 2001
> From: Boaz Harrosh <bharrosh@bh-buildlin2.(none)>
> Date: Thu, 14 Jun 2007 19:14:40 +0300
> Subject: [PATCH] Farther clean up of stex.c driver
> 
>  - now that scsi-ml accessors do not allow modifying of 
> sg_count bufflen and
>    sglist. The code in question is open coded to directly 
> hack the scsi_cmnd
>    structure. When implementation changes the driver will 
> need to change with it.
> 
>  - Maintainer of this driver should please review again if we 
> absolutely need this
>    read-sense length fixing. What if less bytes are read and 
> 0 are left at the end.
>    Also no check is made if the buffer is large enough.
> ---
>  drivers/scsi/stex.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
> index adda296..d784b58 100644
> --- a/drivers/scsi/stex.c
> +++ b/drivers/scsi/stex.c
> @@ -719,7 +719,7 @@ static void stex_ys_commands(struct st_hba *hba,
> 
>  	if (ccb->cmd->cmnd[0] == MGT_CMD &&
>  		resp->scsi_status != SAM_STAT_CHECK_CONDITION) {
> -		scsi_bufflen(ccb->cmd) =
> +		ccb->cmd->request_bufflen =
>  			le32_to_cpu(*(__le32 *)&resp->variable[0]);
>  		return;
>  	}
> -- 
> 1.5.0.4.402.g8035
> 
> 
> 

The software allocates a big enough buffer(so don't worry about this),
and it trims the data size based on returned length. So it needs the
actual data length. So this length fix is necessary if software doesn't
change policy. The whole thing is guaranteed by both software and
firmware, software will do a check, firmware will do a check, so a check
in driver is not necessary.

I wonder whether the following will go upstream:
#define scsi_sg_count(cmd) ((cmd)->sg_table ? (cmd)->sg_table->sg_count
: 0)
#define scsi_sglist(cmd) ((cmd)->sg_table ? (cmd)->sg_table->sglist :
NULL)
#define scsi_bufflen(cmd) ((cmd)->sg_table ? (cmd)->sg_table->length :
0)

For this particular case, because there is data exchange, so
(cmd)->sg_table is not NULL, so we can have something like:
		ccb->cmd->sg_table->length =
  			le32_to_cpu(*(__le32 *)&resp->variable[0]);
Although I am not sure whether the software can work without change
after the scsi_sgtable approach becomes upstream. But I just wonder why
we use some code that is known to be temporary even before its
introduction.

--Ed Lin

RE: [PATCH 3/3] Farther clean up of stex.c driver

[FUJITA Tomonori]

From: "Ed Lin" <ed.lin@promise.com>
Subject: RE: [PATCH 3/3] Farther clean up of stex.c driver
Date: Thu, 14 Jun 2007 12:29:05 -0700

> 
> 
> > -----Original Message-----
> > From: Boaz Harrosh [mailto:bharrosh@panasas.com] 
> > Sent: Thursday, June 14, 2007 9:34 AM
> > To: FUJITA Tomonori; Ed Lin; Guennadi Liakhovetski
> > Cc: linux-scsi@vger.kernel.org
> > Subject: Re: [PATCH 3/3] Farther clean up of stex.c driver
> > 
> > 
> > From 2b82909202cab8dc35184daef45b4b388f93112a Mon Sep 17 00:00:00 2001
> > From: Boaz Harrosh <bharrosh@bh-buildlin2.(none)>
> > Date: Thu, 14 Jun 2007 19:14:40 +0300
> > Subject: [PATCH] Farther clean up of stex.c driver
> > 
> >  - now that scsi-ml accessors do not allow modifying of 
> > sg_count bufflen and
> >    sglist. The code in question is open coded to directly 
> > hack the scsi_cmnd
> >    structure. When implementation changes the driver will 
> > need to change with it.
> > 
> >  - Maintainer of this driver should please review again if we 
> > absolutely need this
> >    read-sense length fixing. What if less bytes are read and 
> > 0 are left at the end.
> >    Also no check is made if the buffer is large enough.
> > ---
> >  drivers/scsi/stex.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
> > index adda296..d784b58 100644
> > --- a/drivers/scsi/stex.c
> > +++ b/drivers/scsi/stex.c
> > @@ -719,7 +719,7 @@ static void stex_ys_commands(struct st_hba *hba,
> > 
> >  	if (ccb->cmd->cmnd[0] == MGT_CMD &&
> >  		resp->scsi_status != SAM_STAT_CHECK_CONDITION) {
> > -		scsi_bufflen(ccb->cmd) =
> > +		ccb->cmd->request_bufflen =
> >  			le32_to_cpu(*(__le32 *)&resp->variable[0]);
> >  		return;
> >  	}
> > -- 
> > 1.5.0.4.402.g8035
> > 
> > 
> > 
> 
> The software allocates a big enough buffer(so don't worry about this),
> and it trims the data size based on returned length. So it needs the
> actual data length.

What's the software? I mean that who trims the data size based on
returned length?

> So this length fix is necessary if software doesn't
> change policy. The whole thing is guaranteed by both software and
> firmware, software will do a check, firmware will do a check, so a check
> in driver is not necessary.


> I wonder whether the following will go upstream:
> #define scsi_sg_count(cmd) ((cmd)->sg_table ? (cmd)->sg_table->sg_count
> : 0)
> #define scsi_sglist(cmd) ((cmd)->sg_table ? (cmd)->sg_table->sglist :
> NULL)
> #define scsi_bufflen(cmd) ((cmd)->sg_table ? (cmd)->sg_table->length :
> 0)
> 
> For this particular case, because there is data exchange, so
> (cmd)->sg_table is not NULL, so we can have something like:
> 		ccb->cmd->sg_table->length =
>   			le32_to_cpu(*(__le32 *)&resp->variable[0]);
> Although I am not sure whether the software can work without change
> after the scsi_sgtable approach becomes upstream. But I just wonder why
> we use some code that is known to be temporary even before its
> introduction.

I think that it would be better to apply Boaz patchset with scsi-bidi
support (not now). Then you can avoid adding temporary hack.

RE: [PATCH 3/3] Farther clean up of stex.c driver

[Ed Lin]

> -----Original Message-----
> From: FUJITA Tomonori [mailto:fujita.tomonori@lab.ntt.co.jp] 
> Sent: Thursday, June 14, 2007 3:21 PM
> To: Ed Lin
> Cc: bharrosh@panasas.com; fujita.tomonori@lab.ntt.co.jp; 
> g.liakhovetski@gmx.de; linux-scsi@vger.kernel.org
> Subject: RE: [PATCH 3/3] Farther clean up of stex.c driver
> 
> 
> From: "Ed Lin" <ed.lin@promise.com>
> Subject: RE: [PATCH 3/3] Farther clean up of stex.c driver
> Date: Thu, 14 Jun 2007 12:29:05 -0700
> 
> > 
> > 
> > > -----Original Message-----
> > > From: Boaz Harrosh [mailto:bharrosh@panasas.com] 
> > > Sent: Thursday, June 14, 2007 9:34 AM
> > > To: FUJITA Tomonori; Ed Lin; Guennadi Liakhovetski
> > > Cc: linux-scsi@vger.kernel.org
> > > Subject: Re: [PATCH 3/3] Farther clean up of stex.c driver
> > > 
> > > 
> > > From 2b82909202cab8dc35184daef45b4b388f93112a Mon Sep 17 
> 00:00:00 2001
> > > From: Boaz Harrosh <bharrosh@bh-buildlin2.(none)>
> > > Date: Thu, 14 Jun 2007 19:14:40 +0300
> > > Subject: [PATCH] Farther clean up of stex.c driver
> > > 
> > >  - now that scsi-ml accessors do not allow modifying of 
> > > sg_count bufflen and
> > >    sglist. The code in question is open coded to directly 
> > > hack the scsi_cmnd
> > >    structure. When implementation changes the driver will 
> > > need to change with it.
> > > 
> > >  - Maintainer of this driver should please review again if we 
> > > absolutely need this
> > >    read-sense length fixing. What if less bytes are read and 
> > > 0 are left at the end.
> > >    Also no check is made if the buffer is large enough.
> > > ---
> > >  drivers/scsi/stex.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
> > > index adda296..d784b58 100644
> > > --- a/drivers/scsi/stex.c
> > > +++ b/drivers/scsi/stex.c
> > > @@ -719,7 +719,7 @@ static void stex_ys_commands(struct 
> st_hba *hba,
> > > 
> > >  	if (ccb->cmd->cmnd[0] == MGT_CMD &&
> > >  		resp->scsi_status != SAM_STAT_CHECK_CONDITION) {
> > > -		scsi_bufflen(ccb->cmd) =
> > > +		ccb->cmd->request_bufflen =
> > >  			le32_to_cpu(*(__le32 *)&resp->variable[0]);
> > >  		return;
> > >  	}
> > > -- 
> > > 1.5.0.4.402.g8035
> > > 
> > > 
> > > 
> > 
> > The software allocates a big enough buffer(so don't worry 
> about this),
> > and it trims the data size based on returned length. So it needs the
> > actual data length.
> 
> What's the software? I mean that who trims the data size based on
> returned length?
> 

The management software provided by Promise.

> > So this length fix is necessary if software doesn't
> > change policy. The whole thing is guaranteed by both software and
> > firmware, software will do a check, firmware will do a 
> check, so a check
> > in driver is not necessary.
> 
> 
> > I wonder whether the following will go upstream:
> > #define scsi_sg_count(cmd) ((cmd)->sg_table ? 
> (cmd)->sg_table->sg_count
> > : 0)
> > #define scsi_sglist(cmd) ((cmd)->sg_table ? 
> (cmd)->sg_table->sglist :
> > NULL)
> > #define scsi_bufflen(cmd) ((cmd)->sg_table ? 
> (cmd)->sg_table->length :
> > 0)
> > 
> > For this particular case, because there is data exchange, so
> > (cmd)->sg_table is not NULL, so we can have something like:
> > 		ccb->cmd->sg_table->length =
> >   			le32_to_cpu(*(__le32 *)&resp->variable[0]);
> > Although I am not sure whether the software can work without change
> > after the scsi_sgtable approach becomes upstream. But I 
> just wonder why
> > we use some code that is known to be temporary even before its
> > introduction.
> 
> I think that it would be better to apply Boaz patchset with scsi-bidi
> support (not now). Then you can avoid adding temporary hack.
> 

Does the scsi_sgtable above belong to the scsi-bidi patchset? If it
does, then this patch is just a temporary hack. The code needs to be
changed again at that time. This is stated in the comment of this patch.

--Ed Lin

RE: [PATCH 3/3] Farther clean up of stex.c driver

[FUJITA Tomonori]

From: "Ed Lin" <ed.lin@promise.com>
Subject: RE: [PATCH 3/3] Farther clean up of stex.c driver
Date: Thu, 14 Jun 2007 16:02:30 -0700

> 
> 
> > -----Original Message-----
> > From: FUJITA Tomonori [mailto:fujita.tomonori@lab.ntt.co.jp] 
> > Sent: Thursday, June 14, 2007 3:21 PM
> > To: Ed Lin
> > Cc: bharrosh@panasas.com; fujita.tomonori@lab.ntt.co.jp; 
> > g.liakhovetski@gmx.de; linux-scsi@vger.kernel.org
> > Subject: RE: [PATCH 3/3] Farther clean up of stex.c driver
> > 
> > 
> > From: "Ed Lin" <ed.lin@promise.com>
> > Subject: RE: [PATCH 3/3] Farther clean up of stex.c driver
> > Date: Thu, 14 Jun 2007 12:29:05 -0700
> > 
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Boaz Harrosh [mailto:bharrosh@panasas.com] 
> > > > Sent: Thursday, June 14, 2007 9:34 AM
> > > > To: FUJITA Tomonori; Ed Lin; Guennadi Liakhovetski
> > > > Cc: linux-scsi@vger.kernel.org
> > > > Subject: Re: [PATCH 3/3] Farther clean up of stex.c driver
> > > > 
> > > > 
> > > > From 2b82909202cab8dc35184daef45b4b388f93112a Mon Sep 17 
> > 00:00:00 2001
> > > > From: Boaz Harrosh <bharrosh@bh-buildlin2.(none)>
> > > > Date: Thu, 14 Jun 2007 19:14:40 +0300
> > > > Subject: [PATCH] Farther clean up of stex.c driver
> > > > 
> > > >  - now that scsi-ml accessors do not allow modifying of 
> > > > sg_count bufflen and
> > > >    sglist. The code in question is open coded to directly 
> > > > hack the scsi_cmnd
> > > >    structure. When implementation changes the driver will 
> > > > need to change with it.
> > > > 
> > > >  - Maintainer of this driver should please review again if we 
> > > > absolutely need this
> > > >    read-sense length fixing. What if less bytes are read and 
> > > > 0 are left at the end.
> > > >    Also no check is made if the buffer is large enough.
> > > > ---
> > > >  drivers/scsi/stex.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
> > > > index adda296..d784b58 100644
> > > > --- a/drivers/scsi/stex.c
> > > > +++ b/drivers/scsi/stex.c
> > > > @@ -719,7 +719,7 @@ static void stex_ys_commands(struct 
> > st_hba *hba,
> > > > 
> > > >  	if (ccb->cmd->cmnd[0] == MGT_CMD &&
> > > >  		resp->scsi_status != SAM_STAT_CHECK_CONDITION) {
> > > > -		scsi_bufflen(ccb->cmd) =
> > > > +		ccb->cmd->request_bufflen =
> > > >  			le32_to_cpu(*(__le32 *)&resp->variable[0]);
> > > >  		return;
> > > >  	}
> > > > -- 
> > > > 1.5.0.4.402.g8035
> > > > 
> > > > 
> > > > 
> > > 
> > > The software allocates a big enough buffer(so don't worry 
> > about this),
> > > and it trims the data size based on returned length. So it needs the
> > > actual data length.
> > 
> > What's the software? I mean that who trims the data size based on
> > returned length?
> > 
> 
> The management software provided by Promise.

Any chance to use the resid field to tell a returned length to the
software?


> > > I wonder whether the following will go upstream:
> > > #define scsi_sg_count(cmd) ((cmd)->sg_table ? 
> > (cmd)->sg_table->sg_count
> > > : 0)
> > > #define scsi_sglist(cmd) ((cmd)->sg_table ? 
> > (cmd)->sg_table->sglist :
> > > NULL)
> > > #define scsi_bufflen(cmd) ((cmd)->sg_table ? 
> > (cmd)->sg_table->length :
> > > 0)
> > > 
> > > For this particular case, because there is data exchange, so
> > > (cmd)->sg_table is not NULL, so we can have something like:
> > > 		ccb->cmd->sg_table->length =
> > >   			le32_to_cpu(*(__le32 *)&resp->variable[0]);
> > > Although I am not sure whether the software can work without change
> > > after the scsi_sgtable approach becomes upstream. But I 
> > just wonder why
> > > we use some code that is known to be temporary even before its
> > > introduction.
> > 
> > I think that it would be better to apply Boaz patchset with scsi-bidi
> > support (not now). Then you can avoid adding temporary hack.
> > 
> 
> Does the scsi_sgtable above belong to the scsi-bidi patchset?

Yeah.

> If it does, then this patch is just a temporary hack.

Yeah. That's why I prefer to apply this patch with the scsi-bidi
patchset as I said in the previous mail.


> The code needs to be changed again at that time. This is stated in
> the comment of this patch.

Re: [PATCH 3/3] Farther clean up of stex.c driver

[Boaz Harrosh]

Ed Lin wrote:
> 
>>>>
>>> The software allocates a big enough buffer(so don't worry about this),
>>> and it trims the data size based on returned length. So it needs the
>>> actual data length.

This sounds like a security breach to me. An untrusted user-mode
with knowledge of such hardware in the machine can gain access to kernel
through the wrong( cracker-right) sized request.

>> What's the software? I mean that who trims the data size based on
>> returned length?
>>
> 
> The management software provided by Promise.
> 

I had a deeper look into this, and I am totally confused.
The code in question at stex.c::stex_ys_commands() is called
at softirq from stex_mu_intr(). It will than immediately go 
on to eventually call cmd->scsi_done(cmd);
Now what is the upper layer that calls this command??!

There can be 3 possibilities that I know. From unlikely to likely order:
1. REQ_TYPE_FS command which is not possible because I don't know
   of any one that can issue. a ccb->cmd->cmnd[0] == MGT_CMD
   as an REQ_TYPE_FS. And anyway if bufflen is truncated like that
   a REQ_TYPE_FS command will retry the command until complete.

2. REQ_TYPE_BLOCK_PC through sg, bsg or others at which truncating
   a command like that will leak BIOs when trying to release the 
   command, since scsi-ml relies on bufflen in the call to 
   end_that_request_first.

3. A special upper layer that overrides the done functions at 
   scsi_cmnd or request levels and interprets the return info
   and/or frees buffers. If this is the case, can such a driver
   use a reserved member of scsi_cmnd like the fields in 
   struct scsi_pointer or something hanging on host_scribble?
   Than later return to user-mode what it used to return before
   but leave bufflen alone?

Setting of bufflen like that in a driver sounds like a highway
rubbery to me. Please stop that Hack. Now!

>>> So this length fix is necessary if software doesn't
>>> change policy. The whole thing is guaranteed by both software and
>>> firmware, software will do a check, firmware will do a check, 
>>> so a check in driver is not necessary.


As I said. I'm sure we can change upper layer polices to not
break user-mode but to also not touch a delicate and private member
of scsi-ml. It limits the scsi-ml ability to change and evolve.
The most I can do for now is declare this driver broken once 
scsi_sgtable goes in because I sure am not going to do:
ccb->cmd->sg_table->length = le32_to_cpu(*(__le32 *)&resp->variable[0]);
(Do we need an explanation why this is plain broken, leaking code)

If you point me to the upper layer driver used, I can see if I can change it
in a way that will not touch bufflen.

Another thing I do not understand is where in the SCSI standard
such an operation is defined. because if it is a standard SCSI
command or extended or vendor-specific command than, current code
is not built to cope with such scenario. How would one defined
it? a "truncated read"?

Boaz Harrosh

RE: [PATCH 3/3] Farther clean up of stex.c driver

[Lin Yu]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Boaz Harrosh
> Sent: Sunday, July 01, 2007 6:56 AM
> To: Ed Lin; FUJITA Tomonori
> Cc: g.liakhovetski@gmx.de; linux-scsi@vger.kernel.org; James Bottomley
> Subject: Re: [PATCH 3/3] Farther clean up of stex.c driver
> 
> 
> Ed Lin wrote:
> > 
> >>>>
> >>> The software allocates a big enough buffer(so don't worry 
> about this),
> >>> and it trims the data size based on returned length. So 
> it needs the
> >>> actual data length.
> 
> This sounds like a security breach to me. An untrusted user-mode
> with knowledge of such hardware in the machine can gain 
> access to kernel
> through the wrong( cracker-right) sized request.
> 
> >> What's the software? I mean that who trims the data size based on
> >> returned length?
> >>
> > 
> > The management software provided by Promise.
> > 
> 
> I had a deeper look into this, and I am totally confused.
> The code in question at stex.c::stex_ys_commands() is called
> at softirq from stex_mu_intr(). It will than immediately go 
> on to eventually call cmd->scsi_done(cmd);
> Now what is the upper layer that calls this command??!
> 
> There can be 3 possibilities that I know. From unlikely to 
> likely order:
> 1. REQ_TYPE_FS command which is not possible because I don't know
>    of any one that can issue. a ccb->cmd->cmnd[0] == MGT_CMD
>    as an REQ_TYPE_FS. And anyway if bufflen is truncated like that
>    a REQ_TYPE_FS command will retry the command until complete.
> 
> 2. REQ_TYPE_BLOCK_PC through sg, bsg or others at which truncating
>    a command like that will leak BIOs when trying to release the 
>    command, since scsi-ml relies on bufflen in the call to 
>    end_that_request_first.
> 
> 3. A special upper layer that overrides the done functions at 
>    scsi_cmnd or request levels and interprets the return info
>    and/or frees buffers. If this is the case, can such a driver
>    use a reserved member of scsi_cmnd like the fields in 
>    struct scsi_pointer or something hanging on host_scribble?
>    Than later return to user-mode what it used to return before
>    but leave bufflen alone?
> 
> Setting of bufflen like that in a driver sounds like a highway
> rubbery to me. Please stop that Hack. Now!
> 
> >>> So this length fix is necessary if software doesn't
> >>> change policy. The whole thing is guaranteed by both software and
> >>> firmware, software will do a check, firmware will do a check, 
> >>> so a check in driver is not necessary.
> 
> 
> As I said. I'm sure we can change upper layer polices to not
> break user-mode but to also not touch a delicate and private member
> of scsi-ml. It limits the scsi-ml ability to change and evolve.
> The most I can do for now is declare this driver broken once 
> scsi_sgtable goes in because I sure am not going to do:
> ccb->cmd->sg_table->length = le32_to_cpu(*(__le32 
> *)&resp->variable[0]);
> (Do we need an explanation why this is plain broken, leaking code)
> 
> If you point me to the upper layer driver used, I can see if 
> I can change it
> in a way that will not touch bufflen.
> 
> Another thing I do not understand is where in the SCSI standard
> such an operation is defined. because if it is a standard SCSI
> command or extended or vendor-specific command than, current code
> is not built to cope with such scenario. How would one defined
> it? a "truncated read"?
> 
> Boaz Harrosh
> 

We are investigating it. Please wait a while. We are also trying
to make this driver better.
Thanks for your findings and efforts.

Ed Lin