| Message ID | 20220304025608.1874516-2-haowenchao@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] iscsi_tcp: Fix NULL pointer dereference in iscsi_sw_tcp_conn_get_param() | expand |
On 3/3/22 8:56 PM, Wenchao Hao wrote: > iscsi_create_conn() would add newly alloced iscsi_cls_conn to connlist, > it means when userspace sends ISCSI_UEVENT_SET_PARAM, iscsi_conn_lookup() > would found this iscsi_cls_conn and call the set_param callback which is > iscsi_sw_tcp_conn_set_param(). While the iscsi_conn's dd_data might not > been initialized. > > Signed-off-by: Wenchao Hao <haowenchao@huawei.com> > Signed-off-by: Wu Bo <wubo40@huawei.com> > --- > drivers/scsi/iscsi_tcp.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c > index 14db224486be..a42449df6156 100644 > --- a/drivers/scsi/iscsi_tcp.c > +++ b/drivers/scsi/iscsi_tcp.c > @@ -716,13 +716,17 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn, > { > struct iscsi_conn *conn = cls_conn->dd_data; > struct iscsi_tcp_conn *tcp_conn = conn->dd_data; > - struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data; > + struct iscsi_sw_tcp_conn *tcp_sw_conn; > > switch(param) { > case ISCSI_PARAM_HDRDGST_EN: > iscsi_set_param(cls_conn, param, buf, buflen); > break; > case ISCSI_PARAM_DATADGST_EN: > + if (!tcp_conn || !tcp_conn->dd_data) > + return -ENOTCONN; > + > + tcp_sw_conn = tcp_conn->dd_data; > iscsi_set_param(cls_conn, param, buf, buflen); > tcp_sw_conn->sendpage = conn->datadgst_en ? > sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage; Is this something you hit or from code review? We have those state checks: if ((conn->state == ISCSI_CONN_BOUND) || (conn->state == ISCSI_CONN_UP)) { err = transport->set_param(conn, ev->u.set_param.param, so we don't call set_param until after we have bound the connection which will be after ISCSI_UEVENT_CREATE_CONN has returned. Also for this specific bug iscsi_if_recv_msg is called with the rx_queue_mutex, so set_param can only be called after the ISCSI_UEVENT_CREATE_CONN cmd has returned.
On 2022/3/3 22:59, Mike Christie wrote: > On 3/3/22 8:56 PM, Wenchao Hao wrote: >> iscsi_create_conn() would add newly alloced iscsi_cls_conn to connlist, >> it means when userspace sends ISCSI_UEVENT_SET_PARAM, iscsi_conn_lookup() >> would found this iscsi_cls_conn and call the set_param callback which is >> iscsi_sw_tcp_conn_set_param(). While the iscsi_conn's dd_data might not >> been initialized. >> >> Signed-off-by: Wenchao Hao <haowenchao@huawei.com> >> Signed-off-by: Wu Bo <wubo40@huawei.com> >> --- >> drivers/scsi/iscsi_tcp.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c >> index 14db224486be..a42449df6156 100644 >> --- a/drivers/scsi/iscsi_tcp.c >> +++ b/drivers/scsi/iscsi_tcp.c >> @@ -716,13 +716,17 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn, >> { >> struct iscsi_conn *conn = cls_conn->dd_data; >> struct iscsi_tcp_conn *tcp_conn = conn->dd_data; >> - struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data; >> + struct iscsi_sw_tcp_conn *tcp_sw_conn; >> >> switch(param) { >> case ISCSI_PARAM_HDRDGST_EN: >> iscsi_set_param(cls_conn, param, buf, buflen); >> break; >> case ISCSI_PARAM_DATADGST_EN: >> + if (!tcp_conn || !tcp_conn->dd_data) >> + return -ENOTCONN; >> + >> + tcp_sw_conn = tcp_conn->dd_data; >> iscsi_set_param(cls_conn, param, buf, buflen); >> tcp_sw_conn->sendpage = conn->datadgst_en ? >> sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage; > > Is this something you hit or from code review? > It's from code review. I reviewed the code because the panic mentioned in my first patch. The issue seems would not happen, so just ignore it. > We have those state checks: > > if ((conn->state == ISCSI_CONN_BOUND) || > (conn->state == ISCSI_CONN_UP)) { > err = transport->set_param(conn, ev->u.set_param.param, > > so we don't call set_param until after we have bound the > connection which will be after ISCSI_UEVENT_CREATE_CONN has returned. > > Also for this specific bug iscsi_if_recv_msg is called with the > rx_queue_mutex, so set_param can only be called after the > ISCSI_UEVENT_CREATE_CONN cmd has returned. > . >
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index 14db224486be..a42449df6156 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -716,13 +716,17 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn, { struct iscsi_conn *conn = cls_conn->dd_data; struct iscsi_tcp_conn *tcp_conn = conn->dd_data; - struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data; + struct iscsi_sw_tcp_conn *tcp_sw_conn; switch(param) { case ISCSI_PARAM_HDRDGST_EN: iscsi_set_param(cls_conn, param, buf, buflen); break; case ISCSI_PARAM_DATADGST_EN: + if (!tcp_conn || !tcp_conn->dd_data) + return -ENOTCONN; + + tcp_sw_conn = tcp_conn->dd_data; iscsi_set_param(cls_conn, param, buf, buflen); tcp_sw_conn->sendpage = conn->datadgst_en ? sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage;