[PATCH] iscsi_tcp: fix the NULL pointer dereference

[PATCH] iscsi_tcp: fix the NULL pointer dereference

[Li Feng]

iscsi_sw_tcp_conn_set_param should check the pointer before using it.

I got this ops when starting my os:
[   27.158355] sd 27:0:0:1: [sdco] Mode Sense: 83 00 00 08
[   27.158377] scsi 27:0:0:3: Direct-Access     ZBS      VOLUME           0000 PQ: 0 ANSI: 5
[   27.167665]  connection39:0: detected conn error (1020)
[   27.174681] BUG: kernel NULL pointer dereference, address: 0000000000000020
[   27.174706] #PF: supervisor read access in kernel mode
[   27.174719] #PF: error_code(0x0000) - not-present page
[   27.174731] PGD 0 P4D 0
[   27.174739] Oops: 0000 [#1] SMP NOPTI
[   27.174749] CPU: 8 PID: 1044 Comm: iscsid Not tainted 5.11.12-300.fc34.x86_64 #1
[   27.174767] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 12/12/2018
[   27.174790] RIP: 0010:iscsi_sw_tcp_conn_set_param+0x6a/0x80 [iscsi_tcp]
[   27.174807] Code: 85 d2 74 23 48 89 83 e0 00 00 00 31 c0 5b 5d c3 e8 bb 21 fb ff 31 c0 5b 5d c3 48 89 ef 5b 48 89 d6 5d e9 99 62 ff ff 48 8b 03 <48> 8b 40 20 48 8b 80 a0 00 00 00 eb cd 66 0f 1f 84 00 00 00 00 00
[   27.174847] RSP: 0018:ffffba8a46103b90 EFLAGS: 00010246
[   27.174860] RAX: 0000000000000000 RBX: ffff98cbf22766b8 RCX: ffffffffc096d6b9
[   27.174877] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff98cbdbcb6049
[   27.174893] RBP: ffff98cbf2276348 R08: 00000000000000ff R09: 000000000000000a
[   27.174909] R10: 000000000000000a R11: f000000000000000 R12: ffff98cbdbcb6010
[   27.174925] R13: ffff98cbdbcb6048 R14: ffff98cbf2279800 R15: 0000000000000414
[   27.174941] FS:  00007fa88bab7cc0(0000) GS:ffff98d1e0000000(0000) knlGS:0000000000000000
[   27.175188] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   27.175441] CR2: 0000000000000020 CR3: 000000022522c000 CR4: 00000000003506e0
[   27.175734] Call Trace:
[   27.176003]  iscsi_if_rx+0xdda/0x1adc [scsi_transport_iscsi]
[   27.176291]  ? __kmalloc_node_track_caller+0xec/0x280
[   27.176585]  ? netlink_sendmsg+0x30c/0x440
[   27.176880]  netlink_unicast+0x1d3/0x2a0
[   27.177191]  netlink_sendmsg+0x22a/0x440
[   27.177494]  sock_sendmsg+0x5e/0x60
[   27.177799]  ____sys_sendmsg+0x22c/0x270
[   27.178107]  ? import_iovec+0x17/0x20
[   27.178422]  ? sendmsg_copy_msghdr+0x59/0x90
[   27.178747]  ? _copy_from_user+0x3c/0x80
[   27.179072]  ___sys_sendmsg+0x81/0xc0
[   27.179402]  ? ___sys_recvmsg+0x86/0xe0
[   27.179736]  ? handle_mm_fault+0x113f/0x1970
[   27.180076]  ? enqueue_hrtimer+0x32/0x70
[   27.180422]  __sys_sendmsg+0x49/0x80
[   27.180778]  do_syscall_64+0x33/0x40
[   27.181132]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

Signed-off-by: Li Feng <fengli@smartx.com>
---
 drivers/scsi/iscsi_tcp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 1bc37593c88f..2ec1405d272d 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -724,6 +724,8 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn,
 		break;
 	case ISCSI_PARAM_DATADGST_EN:
 		iscsi_set_param(cls_conn, param, buf, buflen);
+		if (!tcp_sw_conn || !tcp_sw_conn->sock)
+			return -ENOTCONN;
 		tcp_sw_conn->sendpage = conn->datadgst_en ?
 			sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage;
 		break;
-- 
2.31.1

Re: [PATCH] iscsi_tcp: fix the NULL pointer dereference

[michael.christie]

On 10/10/21 2:19 AM, Li Feng wrote:
>  drivers/scsi/iscsi_tcp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 1bc37593c88f..2ec1405d272d 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -724,6 +724,8 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn,
>  		break;
>  	case ISCSI_PARAM_DATADGST_EN:
>  		iscsi_set_param(cls_conn, param, buf, buflen);
> +		if (!tcp_sw_conn || !tcp_sw_conn->sock)
> +			return -ENOTCONN;
>  		tcp_sw_conn->sendpage = conn->datadgst_en ?
>  			sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage;
>  		break;
> 

Hi,

Thanks for the patch. This was supposed to be fixed in:

commit 9e67600ed6b8565da4b85698ec659b5879a6c1c6
Author: Gulam Mohamed <gulam.mohamed@oracle.com>
Date:   Thu Mar 25 09:32:48 2021 +0000

    scsi: iscsi: Fix race condition between login and sync thread

because it was not supposed to allow set_param to be called on
an unbound connection. However, it looks like there was a mistake in
the patch:

                err = transport->set_param(conn, ev->u.set_param.param,
                                           data, ev->u.set_param.len);
+               if ((conn->state == ISCSI_CONN_BOUND) ||
+                       (conn->state == ISCSI_CONN_UP)) {
+                       err = transport->set_param(conn, ev->u.set_param.param,
+                                       data, ev->u.set_param.len);
+               } else {
+                       return -ENOTCONN;
+               }


and that first set_param call was supposed to be deleted and
replaced with the one that was added in the conn->state check.

We should just need a patch to remove that first set_param call.

Re: [PATCH] iscsi_tcp: fix the NULL pointer dereference

[Li Feng]

Thanks,
Feng Li

<michael.christie@oracle.com> 于2021年10月11日周一 上午12:05写道：
>
> On 10/10/21 2:19 AM, Li Feng wrote:
> >  drivers/scsi/iscsi_tcp.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> > index 1bc37593c88f..2ec1405d272d 100644
> > --- a/drivers/scsi/iscsi_tcp.c
> > +++ b/drivers/scsi/iscsi_tcp.c
> > @@ -724,6 +724,8 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn,
> >               break;
> >       case ISCSI_PARAM_DATADGST_EN:
> >               iscsi_set_param(cls_conn, param, buf, buflen);
> > +             if (!tcp_sw_conn || !tcp_sw_conn->sock)
> > +                     return -ENOTCONN;
> >               tcp_sw_conn->sendpage = conn->datadgst_en ?
> >                       sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage;
> >               break;
> >
>
> Hi,
>
> Thanks for the patch. This was supposed to be fixed in:
>
> commit 9e67600ed6b8565da4b85698ec659b5879a6c1c6
> Author: Gulam Mohamed <gulam.mohamed@oracle.com>
> Date:   Thu Mar 25 09:32:48 2021 +0000
>
>     scsi: iscsi: Fix race condition between login and sync thread
>
> because it was not supposed to allow set_param to be called on
> an unbound connection. However, it looks like there was a mistake in
> the patch:
>
>                 err = transport->set_param(conn, ev->u.set_param.param,
>                                            data, ev->u.set_param.len);
> +               if ((conn->state == ISCSI_CONN_BOUND) ||
> +                       (conn->state == ISCSI_CONN_UP)) {
> +                       err = transport->set_param(conn, ev->u.set_param.param,
> +                                       data, ev->u.set_param.len);
> +               } else {
> +                       return -ENOTCONN;
> +               }
>
>
> and that first set_param call was supposed to be deleted and
> replaced with the one that was added in the conn->state check.
>
> We should just need a patch to remove that first set_param call.

Yes, I have checked the upstream code, this is an obvious mistake here.
I encountered this issue on 5.11.12-300.fc34.x86_64, it doesn't
include this patch,
so I check the pointer like my patch.
Thanks.