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