[Stgt-devel] [PATCH] task data leak

[Pete Wyckoff]

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.

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.

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.

		-- Pete