| Message ID | 20210407012450.97754-3-haowenchao@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix use-after-free in iscsi_sw_tcp_host_get_param() | expand |
On 4/6/21 8:24 PM, Wenchao Hao wrote: > iscsi_sw_tcp_host_get_param() would access struct iscsi_session, while > struct iscsi_session might be freed by session destroy flow in > iscsi_free_session(). This commit fix this condition by freeing session > after host has already been removed. > > Signed-off-by: Wenchao Hao <haowenchao@huawei.com> > --- > drivers/scsi/iscsi_tcp.c | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c > index dd33ce0e3737..d559abd3694c 100644 > --- a/drivers/scsi/iscsi_tcp.c > +++ b/drivers/scsi/iscsi_tcp.c > @@ -839,6 +839,18 @@ iscsi_sw_tcp_conn_get_stats(struct iscsi_cls_conn *cls_conn, > iscsi_tcp_conn_get_stats(cls_conn, stats); > } > > +static void > +iscsi_sw_tcp_session_teardown(struct iscsi_cls_session *cls_session) > +{ > + struct Scsi_Host *shost = iscsi_session_to_shost(cls_session); > + > + iscsi_session_destroy(cls_session); > + iscsi_host_remove(shost); > + > + iscsi_free_session(cls_session); > + iscsi_host_free(shost); > +} Can you add a comment about the iscsi_tcp dependency with the host and session or maybe convert ib_iser too? ib_iser does the same session per host scheme and so if you were just scanning the code and going to make a API change, it's not really clear why the drivers do it differently.
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index dd33ce0e3737..d559abd3694c 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -839,6 +839,18 @@ iscsi_sw_tcp_conn_get_stats(struct iscsi_cls_conn *cls_conn, iscsi_tcp_conn_get_stats(cls_conn, stats); } +static void +iscsi_sw_tcp_session_teardown(struct iscsi_cls_session *cls_session) +{ + struct Scsi_Host *shost = iscsi_session_to_shost(cls_session); + + iscsi_session_destroy(cls_session); + iscsi_host_remove(shost); + + iscsi_free_session(cls_session); + iscsi_host_free(shost); +} + static struct iscsi_cls_session * iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max, uint16_t qdepth, uint32_t initial_cmdsn) @@ -884,12 +896,13 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max, tcp_sw_host = iscsi_host_priv(shost); tcp_sw_host->session = session; - if (iscsi_tcp_r2tpool_alloc(session)) - goto remove_session; + if (iscsi_tcp_r2tpool_alloc(session)) { + iscsi_sw_tcp_session_teardown(cls_session); + return NULL; + } + return cls_session; -remove_session: - iscsi_session_teardown(cls_session); remove_host: iscsi_host_remove(shost); free_host: @@ -899,17 +912,13 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max, static void iscsi_sw_tcp_session_destroy(struct iscsi_cls_session *cls_session) { - struct Scsi_Host *shost = iscsi_session_to_shost(cls_session); struct iscsi_session *session = cls_session->dd_data; if (WARN_ON_ONCE(session->leadconn)) return; iscsi_tcp_r2tpool_free(cls_session->dd_data); - iscsi_session_teardown(cls_session); - - iscsi_host_remove(shost); - iscsi_host_free(shost); + iscsi_sw_tcp_session_teardown(cls_session); } static umode_t iscsi_sw_tcp_attr_is_visible(int param_type, int param)
iscsi_sw_tcp_host_get_param() would access struct iscsi_session, while struct iscsi_session might be freed by session destroy flow in iscsi_free_session(). This commit fix this condition by freeing session after host has already been removed. Signed-off-by: Wenchao Hao <haowenchao@huawei.com> --- drivers/scsi/iscsi_tcp.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)