* [PATCH] target: Fix target_sense_desc_format NULL pointer dereference @ 2015-09-16 6:31 Nicholas A. Bellinger 2015-09-16 12:08 ` Sagi Grimberg 0 siblings, 1 reply; 4+ messages in thread From: Nicholas A. Bellinger @ 2015-09-16 6:31 UTC (permalink / raw) To: target-devel Cc: linux-scsi, Nicholas Bellinger, Sagi Grimberg, Christoph Hellwig, Hannes Reinecke From: Nicholas Bellinger <nab@linux-iscsi.org> This patch allows target_sense_desc_format() to be called without a valid se_device pointer, which can occur during an early exception ahead of transport_lookup_cmd_lun() setting up se_cmd->se_device. This addresses a v4.3-rc1 specific NULL pointer dereference regression introduced by commit 4e4937e8. Cc: Sagi Grimberg <sagig@mellanox.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Hannes Reinecke <hare@suse.de> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> --- drivers/target/target_core_hba.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/target/target_core_hba.c b/drivers/target/target_core_hba.c index 9522960..22390e0 100644 --- a/drivers/target/target_core_hba.c +++ b/drivers/target/target_core_hba.c @@ -187,5 +187,5 @@ core_delete_hba(struct se_hba *hba) bool target_sense_desc_format(struct se_device *dev) { - return dev->transport->get_blocks(dev) > U32_MAX; + return (dev) ? dev->transport->get_blocks(dev) > U32_MAX : false; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] target: Fix target_sense_desc_format NULL pointer dereference 2015-09-16 6:31 [PATCH] target: Fix target_sense_desc_format NULL pointer dereference Nicholas A. Bellinger @ 2015-09-16 12:08 ` Sagi Grimberg 2015-09-17 4:04 ` Nicholas A. Bellinger 0 siblings, 1 reply; 4+ messages in thread From: Sagi Grimberg @ 2015-09-16 12:08 UTC (permalink / raw) To: Nicholas A. Bellinger, target-devel Cc: linux-scsi, Nicholas Bellinger, Sagi Grimberg, Christoph Hellwig, Hannes Reinecke On 9/16/2015 9:31 AM, Nicholas A. Bellinger wrote: > From: Nicholas Bellinger <nab@linux-iscsi.org> > > This patch allows target_sense_desc_format() to be called without a > valid se_device pointer, which can occur during an early exception > ahead of transport_lookup_cmd_lun() setting up se_cmd->se_device. > > This addresses a v4.3-rc1 specific NULL pointer dereference > regression introduced by commit 4e4937e8. > > Cc: Sagi Grimberg <sagig@mellanox.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Hannes Reinecke <hare@suse.de> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> > --- > drivers/target/target_core_hba.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/target/target_core_hba.c b/drivers/target/target_core_hba.c > index 9522960..22390e0 100644 > --- a/drivers/target/target_core_hba.c > +++ b/drivers/target/target_core_hba.c > @@ -187,5 +187,5 @@ core_delete_hba(struct se_hba *hba) > > bool target_sense_desc_format(struct se_device *dev) > { > - return dev->transport->get_blocks(dev) > U32_MAX; > + return (dev) ? dev->transport->get_blocks(dev) > U32_MAX : false; > } > Can we be sure that the only case we'll call target_sense_desc_format() with a NULL se_device will be when returning a CHECK_CONDITION on a non-existing LUN? We return the sense format in the D_SENSE of the control modepage response and if some future bug will happen to call this function with a NULL se_device we might violate what we reported to the initiator. Maybe we should enforce this by having transport_lookup_cmd_lun() set se_cmd->se_device = TARGET_NON_EXISTENT_LUN reserved identifier and check for that rather then NULL? Thoughts? ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] target: Fix target_sense_desc_format NULL pointer dereference 2015-09-16 12:08 ` Sagi Grimberg @ 2015-09-17 4:04 ` Nicholas A. Bellinger 2015-09-17 6:53 ` Sagi Grimberg 0 siblings, 1 reply; 4+ messages in thread From: Nicholas A. Bellinger @ 2015-09-17 4:04 UTC (permalink / raw) To: Sagi Grimberg Cc: Nicholas A. Bellinger, target-devel, linux-scsi, Sagi Grimberg, Christoph Hellwig, Hannes Reinecke On Wed, 2015-09-16 at 15:08 +0300, Sagi Grimberg wrote: > On 9/16/2015 9:31 AM, Nicholas A. Bellinger wrote: > > From: Nicholas Bellinger <nab@linux-iscsi.org> > > > > This patch allows target_sense_desc_format() to be called without a > > valid se_device pointer, which can occur during an early exception > > ahead of transport_lookup_cmd_lun() setting up se_cmd->se_device. > > > > This addresses a v4.3-rc1 specific NULL pointer dereference > > regression introduced by commit 4e4937e8. > > > > Cc: Sagi Grimberg <sagig@mellanox.com> > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Hannes Reinecke <hare@suse.de> > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> > > --- > > drivers/target/target_core_hba.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/target/target_core_hba.c b/drivers/target/target_core_hba.c > > index 9522960..22390e0 100644 > > --- a/drivers/target/target_core_hba.c > > +++ b/drivers/target/target_core_hba.c > > @@ -187,5 +187,5 @@ core_delete_hba(struct se_hba *hba) > > > > bool target_sense_desc_format(struct se_device *dev) > > { > > - return dev->transport->get_blocks(dev) > U32_MAX; > > + return (dev) ? dev->transport->get_blocks(dev) > U32_MAX : false; > > } > > > > Can we be sure that the only case we'll call target_sense_desc_format() > with a NULL se_device will be when returning a CHECK_CONDITION on a > non-existing LUN? WRITE_PROTECTED returns with a NULL se_device pointer as well. > > We return the sense format in the D_SENSE of the control modepage > response and if some future bug will happen to call this function with > a NULL se_device we might violate what we reported to the initiator. > > Maybe we should enforce this by having transport_lookup_cmd_lun() set > se_cmd->se_device = TARGET_NON_EXISTENT_LUN reserved identifier and > check for that rather then NULL? > No objection. > > Thoughts? How about the following to fix up TCM_WRITE_PROTECT + D_SENSE..? diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index abf2076..ba102c5 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -62,22 +62,13 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) struct se_session *se_sess = se_cmd->se_sess; struct se_node_acl *nacl = se_sess->se_node_acl; struct se_dev_entry *deve; + sense_reason_t ret = TCM_NO_SENSE; rcu_read_lock(); deve = target_nacl_find_deve(nacl, unpacked_lun); if (deve) { atomic_long_inc(&deve->total_cmds); - if ((se_cmd->data_direction == DMA_TO_DEVICE) && - (deve->lun_flags & TRANSPORT_LUNFLAGS_READ_ONLY)) { - pr_err("TARGET_CORE[%s]: Detected WRITE_PROTECTED LUN" - " Access for 0x%08llx\n", - se_cmd->se_tfo->get_fabric_name(), - unpacked_lun); - rcu_read_unlock(); - return TCM_WRITE_PROTECTED; - } - if (se_cmd->data_direction == DMA_TO_DEVICE) atomic_long_add(se_cmd->data_length, &deve->write_bytes); @@ -93,6 +84,17 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) percpu_ref_get(&se_lun->lun_ref); se_cmd->lun_ref_active = true; + + if ((se_cmd->data_direction == DMA_TO_DEVICE) && + (deve->lun_flags & TRANSPORT_LUNFLAGS_READ_ONLY)) { + pr_err("TARGET_CORE[%s]: Detected WRITE_PROTECTED LUN" + " Access for 0x%08llx\n", + se_cmd->se_tfo->get_fabric_name(), + unpacked_lun); + rcu_read_unlock(); + ret = TCM_WRITE_PROTECTED; + goto ref_dev; + } } rcu_read_unlock(); @@ -109,12 +111,6 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) unpacked_lun); return TCM_NON_EXISTENT_LUN; } - /* - * Force WRITE PROTECT for virtual LUN 0 - */ - if ((se_cmd->data_direction != DMA_FROM_DEVICE) && - (se_cmd->data_direction != DMA_NONE)) - return TCM_WRITE_PROTECTED; se_lun = se_sess->se_tpg->tpg_virt_lun0; se_cmd->se_lun = se_sess->se_tpg->tpg_virt_lun0; @@ -123,6 +119,15 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) percpu_ref_get(&se_lun->lun_ref); se_cmd->lun_ref_active = true; + + /* + * Force WRITE PROTECT for virtual LUN 0 + */ + if ((se_cmd->data_direction != DMA_FROM_DEVICE) && + (se_cmd->data_direction != DMA_NONE)) { + ret = TCM_WRITE_PROTECTED; + goto ref_dev; + } } /* * RCU reference protected by percpu se_lun->lun_ref taken above that @@ -130,6 +135,7 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) * pointer can be kfree_rcu() by the final se_lun->lun_group put via * target_core_fabric_configfs.c:target_fabric_port_release */ +ref_dev: se_cmd->se_dev = rcu_dereference_raw(se_lun->lun_se_dev); atomic_long_inc(&se_cmd->se_dev->num_cmds); @@ -140,7 +146,7 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) atomic_long_add(se_cmd->data_length, &se_cmd->se_dev->read_bytes); - return 0; + return ret; } EXPORT_SYMBOL(transport_lookup_cmd_lun); ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] target: Fix target_sense_desc_format NULL pointer dereference 2015-09-17 4:04 ` Nicholas A. Bellinger @ 2015-09-17 6:53 ` Sagi Grimberg 0 siblings, 0 replies; 4+ messages in thread From: Sagi Grimberg @ 2015-09-17 6:53 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: Nicholas A. Bellinger, target-devel, linux-scsi, Sagi Grimberg, Christoph Hellwig, Hannes Reinecke > How about the following to fix up TCM_WRITE_PROTECT + D_SENSE..? > > diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c > index abf2076..ba102c5 100644 > --- a/drivers/target/target_core_device.c > +++ b/drivers/target/target_core_device.c > @@ -62,22 +62,13 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) > struct se_session *se_sess = se_cmd->se_sess; > struct se_node_acl *nacl = se_sess->se_node_acl; > struct se_dev_entry *deve; > + sense_reason_t ret = TCM_NO_SENSE; > > rcu_read_lock(); > deve = target_nacl_find_deve(nacl, unpacked_lun); > if (deve) { > atomic_long_inc(&deve->total_cmds); > > - if ((se_cmd->data_direction == DMA_TO_DEVICE) && > - (deve->lun_flags & TRANSPORT_LUNFLAGS_READ_ONLY)) { > - pr_err("TARGET_CORE[%s]: Detected WRITE_PROTECTED LUN" > - " Access for 0x%08llx\n", > - se_cmd->se_tfo->get_fabric_name(), > - unpacked_lun); > - rcu_read_unlock(); > - return TCM_WRITE_PROTECTED; > - } > - > if (se_cmd->data_direction == DMA_TO_DEVICE) > atomic_long_add(se_cmd->data_length, > &deve->write_bytes); > @@ -93,6 +84,17 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) > > percpu_ref_get(&se_lun->lun_ref); > se_cmd->lun_ref_active = true; > + > + if ((se_cmd->data_direction == DMA_TO_DEVICE) && > + (deve->lun_flags & TRANSPORT_LUNFLAGS_READ_ONLY)) { > + pr_err("TARGET_CORE[%s]: Detected WRITE_PROTECTED LUN" > + " Access for 0x%08llx\n", > + se_cmd->se_tfo->get_fabric_name(), > + unpacked_lun); > + rcu_read_unlock(); > + ret = TCM_WRITE_PROTECTED; > + goto ref_dev; > + } > } > rcu_read_unlock(); > > @@ -109,12 +111,6 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) > unpacked_lun); > return TCM_NON_EXISTENT_LUN; > } > - /* > - * Force WRITE PROTECT for virtual LUN 0 > - */ > - if ((se_cmd->data_direction != DMA_FROM_DEVICE) && > - (se_cmd->data_direction != DMA_NONE)) > - return TCM_WRITE_PROTECTED; > > se_lun = se_sess->se_tpg->tpg_virt_lun0; > se_cmd->se_lun = se_sess->se_tpg->tpg_virt_lun0; > @@ -123,6 +119,15 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) > > percpu_ref_get(&se_lun->lun_ref); > se_cmd->lun_ref_active = true; > + > + /* > + * Force WRITE PROTECT for virtual LUN 0 > + */ > + if ((se_cmd->data_direction != DMA_FROM_DEVICE) && > + (se_cmd->data_direction != DMA_NONE)) { > + ret = TCM_WRITE_PROTECTED; > + goto ref_dev; > + } > } > /* > * RCU reference protected by percpu se_lun->lun_ref taken above that > @@ -130,6 +135,7 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) > * pointer can be kfree_rcu() by the final se_lun->lun_group put via > * target_core_fabric_configfs.c:target_fabric_port_release > */ > +ref_dev: > se_cmd->se_dev = rcu_dereference_raw(se_lun->lun_se_dev); > atomic_long_inc(&se_cmd->se_dev->num_cmds); So transport_lookup_cmd_lun() will always assign se_dev. Looks fine. You can add when resubmitting: Reviewed-by: Sagi Grimberg <sagig@mellanox.com> Thanks Nic. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-09-17 6:53 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-09-16 6:31 [PATCH] target: Fix target_sense_desc_format NULL pointer dereference Nicholas A. Bellinger 2015-09-16 12:08 ` Sagi Grimberg 2015-09-17 4:04 ` Nicholas A. Bellinger 2015-09-17 6:53 ` Sagi Grimberg
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.