mbox series

[v4,0/4] target: fix NULL pointer dereference

Message ID 1591559913-8388-1-git-send-email-sudhakar.panneerselvam@oracle.com
Headers show
Series target: fix NULL pointer dereference | expand

Message

Sudhakar Panneerselvam June 7, 2020, 7:58 p.m. UTC
The following set of commits address a NULL pointer dereference and some
refactoring around this issue.

v4:
 - initialize the LUN in transport_init_se_cmd()

v3:
 - fix NULL pointer dereference when cdb initialization fails

v2:
 - new helper is named as target_cmd_init_cdb()
 - existing function, target_setup_cmd_from_cdb is renamed as
   target_cmd_parse_cdb()

Sudhakar Panneerselvam (4):
  target: factor out a new helper, target_cmd_init_cdb()
  target: initialize LUN in transport_init_se_cmd().
  target: fix NULL pointer dereference
  target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb()

 drivers/target/iscsi/iscsi_target.c    | 29 ++++++++++--------
 drivers/target/target_core_device.c    | 19 +++++-------
 drivers/target/target_core_tmr.c       |  4 +--
 drivers/target/target_core_transport.c | 55 ++++++++++++++++++++++++++--------
 drivers/target/target_core_xcopy.c     |  9 ++++--
 drivers/usb/gadget/function/f_tcm.c    |  6 ++--
 include/target/target_core_fabric.h    |  9 +++---
 7 files changed, 83 insertions(+), 48 deletions(-)

Comments

Martin Wilck Sept. 2, 2020, 1:49 p.m. UTC | #1
Hello Sudhakar,

On Sun, 2020-06-07 at 19:58 +0000, Sudhakar Panneerselvam wrote:
> Initialization of orig_fe_lun is moved to transport_init_se_cmd()

> from

> transport_lookup_cmd_lun(). This helps for the cases where the scsi

> request

> fails before the call to transport_lookup_cmd_lun() so that

> trace_target_cmd_complete() can print the LUN information to the

> trace

> buffer. Due to this change, the lun parameter is removed from

> transport_lookup_cmd_lun() and transport_lookup_tmr_lun().

> 

> Signed-off-by: Sudhakar Panneerselvam <

> sudhakar.panneerselvam@oracle.com>

> ---

>  drivers/target/iscsi/iscsi_target.c    | 11 +++++------

>  drivers/target/target_core_device.c    | 19 ++++++++-----------

>  drivers/target/target_core_tmr.c       |  4 ++--

>  drivers/target/target_core_transport.c | 12 +++++++-----

>  drivers/target/target_core_xcopy.c     |  4 ++--

>  drivers/usb/gadget/function/f_tcm.c    |  6 ++++--

>  include/target/target_core_fabric.h    |  6 +++---

>  7 files changed, 31 insertions(+), 31 deletions(-)

> 

> [...]

> diff --git a/drivers/target/target_core_transport.c

> b/drivers/target/target_core_transport.c

> index f2f7c5b818cc..7ea77933e64d 100644

> --- a/drivers/target/target_core_transport.c

> +++ b/drivers/target/target_core_transport.c

> [...]

> @@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd,

> struct se_session *se_sess,

>  	BUG_ON(!se_tpg);

>  

>  	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,

> -			      0, DMA_NONE, TCM_SIMPLE_TAG, sense);

> +			      0, DMA_NONE, TCM_SIMPLE_TAG, sense,

> unpacked_lun);

>  	/*

>  	 * FIXME: Currently expect caller to handle se_cmd->se_tmr_req

>  	 * allocation failure.


Between this hunk and the next one in target_submit_tmr(), there
is this code:

        /*
         * If this is ABORT_TASK with no explicit fabric provided LUN,
         * go ahead and search active session tags for a match to figure
         * out unpacked_lun for the original se_cmd.
         */
        if (tm_type == TMR_ABORT_TASK && (flags & TARGET_SCF_LOOKUP_LUN_FROM_TAG)) {
                if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun))
                        goto failure;
        }

> @@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd,

> struct se_session *se_sess,

>  			goto failure;

>  	}

>  

> -	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);

> +	ret = transport_lookup_tmr_lun(se_cmd);

>  	if (ret)

>  		goto failure;

>  


AFAICS, your patch breaks the case where the above code is executed to
derive unpacked_lun from the tag. The updated value of unpacked_lun is 
never used. That would break aborts for the qla2xxx target.

Am I overlooking something? Can you please explain?

Regards
Martin
Sudhakar Panneerselvam Sept. 2, 2020, 3:14 p.m. UTC | #2
Hi Martin,

> -----Original Message-----

> From: Martin Wilck [mailto:mwilck@suse.com]

> Sent: Wednesday, September 2, 2020 7:50 AM

> To: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>; Martin

> Petersen <martin.petersen@oracle.com>; Michael Christie

> <michael.christie@oracle.com>; target-devel@vger.kernel.org; linux-

> scsi@vger.kernel.org

> Cc: Shirley Ma <shirley.ma@oracle.com>; Hannes Reinecke <hare@suse.com>;

> Daniel Wagner <daniel.wagner@suse.com>; Arun Easi <aeasi@marvell.com>

> Subject: Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().

> 

> Hello Sudhakar,

> 

> On Sun, 2020-06-07 at 19:58 +0000, Sudhakar Panneerselvam wrote:

> > Initialization of orig_fe_lun is moved to transport_init_se_cmd()

> > from

> > transport_lookup_cmd_lun(). This helps for the cases where the scsi

> > request

> > fails before the call to transport_lookup_cmd_lun() so that

> > trace_target_cmd_complete() can print the LUN information to the

> > trace

> > buffer. Due to this change, the lun parameter is removed from

> > transport_lookup_cmd_lun() and transport_lookup_tmr_lun().

> >

> > Signed-off-by: Sudhakar Panneerselvam <

> > sudhakar.panneerselvam@oracle.com>

> > ---

> >  drivers/target/iscsi/iscsi_target.c    | 11 +++++------

> >  drivers/target/target_core_device.c    | 19 ++++++++-----------

> >  drivers/target/target_core_tmr.c       |  4 ++--

> >  drivers/target/target_core_transport.c | 12 +++++++-----

> >  drivers/target/target_core_xcopy.c     |  4 ++--

> >  drivers/usb/gadget/function/f_tcm.c    |  6 ++++--

> >  include/target/target_core_fabric.h    |  6 +++---

> >  7 files changed, 31 insertions(+), 31 deletions(-)

> >

> > [...]

> > diff --git a/drivers/target/target_core_transport.c

> > b/drivers/target/target_core_transport.c

> > index f2f7c5b818cc..7ea77933e64d 100644

> > --- a/drivers/target/target_core_transport.c

> > +++ b/drivers/target/target_core_transport.c

> > [...]

> > @@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd,

> > struct se_session *se_sess,

> >  	BUG_ON(!se_tpg);

> >

> >  	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,

> > -			      0, DMA_NONE, TCM_SIMPLE_TAG, sense);

> > +			      0, DMA_NONE, TCM_SIMPLE_TAG, sense,

> > unpacked_lun);

> >  	/*

> >  	 * FIXME: Currently expect caller to handle se_cmd->se_tmr_req

> >  	 * allocation failure.

> 

> Between this hunk and the next one in target_submit_tmr(), there

> is this code:

> 

>         /*

>          * If this is ABORT_TASK with no explicit fabric provided LUN,

>          * go ahead and search active session tags for a match to figure

>          * out unpacked_lun for the original se_cmd.

>          */

>         if (tm_type == TMR_ABORT_TASK && (flags &

> TARGET_SCF_LOOKUP_LUN_FROM_TAG)) {

>                 if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun))

>                         goto failure;

>         }

> 

> > @@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd,

> > struct se_session *se_sess,

> >  			goto failure;

> >  	}

> >

> > -	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);

> > +	ret = transport_lookup_tmr_lun(se_cmd);

> >  	if (ret)

> >  		goto failure;

> >

> 

> AFAICS, your patch breaks the case where the above code is executed to

> derive unpacked_lun from the tag. The updated value of unpacked_lun is

> never used. That would break aborts for the qla2xxx target.

> 

> Am I overlooking something? Can you please explain?

> 


You are right. This change breaks the qlogic abort task code path, since it is the only transport that sets the TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My apologies. I can send out a patch if you have not written one already. Please let me know.

Thanks
Sudhakar

> Regards

> Martin

> 

>
Martin Wilck Sept. 3, 2020, 1 p.m. UTC | #3
On Wed, 2020-09-02 at 08:14 -0700, Sudhakar Panneerselvam wrote:
> Hi Martin,

> 

> > 

> > AFAICS, your patch breaks the case where the above code is executed

> > to

> > derive unpacked_lun from the tag. The updated value of unpacked_lun

> > is

> > never used. That would break aborts for the qla2xxx target.

> > 

> > Am I overlooking something? Can you please explain?

> > 

> 

> You are right. This change breaks the qlogic abort task code path,

> since it is the only transport that sets the

> TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My

> apologies. I can send out a patch if you have not written one

> already. Please let me know.


Please go ahead. I haven't written a patch - I'm not experienced enough
with the target code to quickly grok whether simply moving the
target_lookup_lun_from_tag() code upward would work, in particular wrt
handling failures and cleaning up.

Regards,
Martin
Martin Wilck Sept. 3, 2020, 1:13 p.m. UTC | #4
On Thu, 2020-09-03 at 15:00 +0200, Martin Wilck wrote:
> On Wed, 2020-09-02 at 08:14 -0700, Sudhakar Panneerselvam wrote:

> > Hi Martin,

> > 

> > > AFAICS, your patch breaks the case where the above code is

> > > executed

> > > to

> > > derive unpacked_lun from the tag. The updated value of

> > > unpacked_lun

> > > is

> > > never used. That would break aborts for the qla2xxx target.

> > > 

> > > Am I overlooking something? Can you please explain?

> > > 

> > 

> > You are right. This change breaks the qlogic abort task code path,

> > since it is the only transport that sets the

> > TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My

> > apologies. I can send out a patch if you have not written one

> > already. Please let me know.

> 

> Please go ahead. I haven't written a patch - I'm not experienced

> enough

> with the target code to quickly grok whether simply moving the

> target_lookup_lun_from_tag() code upward would work, in particular

> wrt

> handling failures and cleaning up.


We may want to discuss whether TARGET_SCF_LOOKUP_LUN_FROM_TAG should be
removed entirely. As you said, qla2xxx is the only user of this
feature. And it actually uses it only in a single code path, 
__qlt_24xx_handle_abts() (everywhere else the LUN is known beforehand,
AFAICT).

If you look at the code of __qlt_24xx_handle_abts(), you can see that
it calls tcm_qla2xxx_find_cmd_by_tag(), which does the same thing
as target_lookup_lun_from_tag(): iterate over se_sess->sess_cmd_list
until a matching tag is found. It would clearly be equivalent, and more
efficient, if qla2xxx set the lun directly rather than relying on
common code iterating through the same list again, later.

Martin