[Stgt-devel] [PATCH] task data leak

[FUJITA Tomonori]

On Sat, 8 Dec 2007 10:30:16 -0500
Pete Wyckoff <pw at osc.edu> wrote:

> tomof at acm.org wrote on Sat, 08 Dec 2007 21:59 +0900:
> > From: Pete Wyckoff <pw at osc.edu>
> > Subject: [Stgt-devel] [PATCH] task data leak
> > Date: Fri, 7 Dec 2007 15:23:53 -0500
> > 
> > > iscsi_scsi_cmd_rx_start always allocates a buffer of 4096 to accommodate
> > > assumptions in spc, sbc, etc.  Even when a SCSI command asks for data
> > > length of zero, task->data is allocated to 4096.  However this is never
> > > assigned as in or out buf on scmd.  Thus never freed.
> > > 
> > > This works around that by freeing an orphaned task->data.  Again, though,
> > > the better solution is to fix up all the little functions like inquiry
> > > that cause this situation in the first place.
> > > 
> > > Signed-off-by: Pete Wyckoff <pw at osc.edu>
> > > ---
> > >  usr/iscsi/iscsid.c |   11 +++++++++++
> > >  1 files changed, 11 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/usr/iscsi/iscsid.c b/usr/iscsi/iscsid.c
> > > index 1e0172a..ab1999d 100644
> > > --- a/usr/iscsi/iscsid.c
> > > +++ b/usr/iscsi/iscsid.c
> > > @@ -1024,6 +1024,17 @@ void iscsi_free_task(struct iscsi_task *task)
> > >  {
> > >  	struct iscsi_connection *conn = task->conn;
> > >  
> > > +	/*
> > > +	 * Catch case when data_len is zero but pushed up to 4096
> > > +	 * to work around spc allocation assumption, but then later
> > > +	 * determined to be DATA_NONE and not used as either in or
> > > +	 * out buffer.
> > > +	 */
> > > +	if (task->data &&
> > > +	    task->data != scsi_get_in_buffer(&task->scmd) &&
> > > +	    task->data != scsi_get_out_buffer(&task->scmd))
> > > +		conn->tp->free_data_buf(conn, task->data);
> > > +
> > 
> > Nice catch, thanks.
> > 
> > How about this patch?
> > 
> > diff --git a/usr/iscsi/iscsid.c b/usr/iscsi/iscsid.c
> > index 1e0172a..e27c74c 100644
> > --- a/usr/iscsi/iscsid.c
> > +++ b/usr/iscsi/iscsid.c
> > @@ -1435,7 +1435,7 @@ static int iscsi_scsi_cmd_rx_start(struct iscsi_connection *conn)
> >  	/*
> >  	 * fix spc, sbc, etc. they assume that buffer is long enough.
> >  	 */
> > -	if (data_len < 4096)
> > +	if (data_len && data_len < 4096)
> >  		data_len = 4096;
> >  
> >  	ext_len = ahs_len ? sizeof(req->cdb) + ahs_len : 0;
> > 
> 
> Would be simpler, yes, but less safe.  Client could do an inquiry
> with data_len of 0 and lead to a crash in spc_inquiry, e.g.

But it is true even before I rewrote data buffer handling code, isn't
it?


> We really should just fix those functions.  Adding a bunch of "if
> (data_len > ...)" is one way, although one would need many of these
> tests for all the various cases.  I had proposed allocating a temp
> buffer, then memcpy with bounds checking into whatever out_buffer is
> available as the final step.  See the old 03/20 iser-transport-buf
> patch if you think that has merit.

I prefer (data_len > ...) way since I always like to use the same way
(report_luns already does that check). But it might become too
ugly. I'll fix it to see how things look like.


> The other issue is that inquiry etc. expect a zeroed buffer, and
> trying to guess if scsi_is_io_opcode() to determine if the handler
> function doesn't need zeroing is insufficient.  I'll send a terrible
> patch to work around this for OSD, but would prefer to fix inquiry
> and friends instead.

Yeah, we need to fix this too.