On Wed, 17 September 2014 12:54:23 -0700, Nicholas A. Bellinger wrote:
> On Tue, 2014-09-02 at 17:49 -0400, Joern Engel wrote:
> > Found by coverity. It appears as if the initiator can cause a kernel
> > NULL pointer dereference at will. Some might consider such behaviour
> > bad. My trivial patch will avoid such badness, at the cost of
> > potentially introducing unexpected behaviour - the internals of
> > iscsit_handle_nop_out() are complicated and don't always dereference
> > NULL.
> >
> > Better patches are welcome. But in the absence of a better patch, this
> > at least doesn't leave trivial DoS vectors open to the public.
> >
> > Signed-off-by: Joern Engel <joern@xxxxxxxxx>
> > ---
> > drivers/target/iscsi/iscsi_target.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> > index dcaebe712259..05932daaf79c 100644
> > --- a/drivers/target/iscsi/iscsi_target.c
> > +++ b/drivers/target/iscsi/iscsi_target.c
> > @@ -4007,9 +4007,9 @@ static int iscsi_target_rx_opcode(struct iscsi_conn *conn, unsigned char *buf)
> > cmd = NULL;
> > if (hdr->ttt == cpu_to_be32(0xFFFFFFFF)) {
> > cmd = iscsit_allocate_cmd(conn, TASK_INTERRUPTIBLE);
> > - if (!cmd)
> > - goto reject;
> > }
> > + if (!cmd)
> > + goto reject;
> > ret = iscsit_handle_nop_out(conn, cmd, buf);
> > break;
> > case ISCSI_OP_SCSI_TMFUNC:
>
> This patch is unnecessary (and introduces a regression) because *cmd is
> only allocated and passed into iscsit_handle_nop_out() when TTT ==
> 0xFFFFFFFF, eg: *cmd is valid only when the incoming NOP-OUT generates a
> NOP-IN response, but not when the incoming NOP-OUT is a response to a
> locally generated NOP-IN.
>
> That said, iscsit_handle_nop_out() already expects *cmd to be NULL when
> TTT != 0xFFFFFFFF, and locates the associated cmd descriptor from the
> connection list before stopping the nopin response timer, and releasing
> the associated resources.
You are right. Not sure if coverity could be smarter here - it would
have to assume that hdr->ttt doesn't get changed by some other thread,
which seems unreasonable as a general rule. Anyway, drop this patch.
Jörn
--
Money can buy bandwidth, but latency is forever.
-- John R. Mashey
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
