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