Re: iscsi target: Set conn->sess to NULL when iscsi_login_set_conn_values fails
On 08/15/2018 10:59 AM, Mike Christie wrote: > On 08/15/2018 05:19 AM, Vincent Pelletier wrote: >> Fixes a use-after-free reported by KASAN when later >> iscsi_target_login_sess_out gets called and it tries to access >> conn->sess->se_sess: >> >> Disabling lock debugging due to kernel taint >> iSCSI Login timeout on Network Portal [::]:3260 >> iSCSI Login negotiation failed. >> == >> BUG: KASAN: use-after-free in iscsi_target_login_sess_out.cold.12+0x58/0xff >> [iscsi_target_mod] >> Read of size 8 at addr 880109d070c8 by task iscsi_np/980 >> >> CPU: 1 PID: 980 Comm: iscsi_np Tainted: G O >> 4.17.8kasan.sess.connops+ #4 >> Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS >> 5.6.5 05/19/2014 >> Call Trace: >> dump_stack+0x71/0xac >> print_address_description+0x65/0x22e >> ? iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod] >> kasan_report.cold.6+0x241/0x2fd >> iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod] >> iscsi_target_login_thread+0x1086/0x1710 [iscsi_target_mod] >> ? __sched_text_start+0x8/0x8 >> ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod] >> ? __kthread_parkme+0xcc/0x100 >> ? parse_args.cold.14+0xd3/0xd3 >> ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod] >> kthread+0x1a0/0x1c0 >> ? kthread_bind+0x30/0x30 >> ret_from_fork+0x35/0x40 >> >> Allocated by task 980: >> kasan_kmalloc+0xbf/0xe0 >> kmem_cache_alloc_trace+0x112/0x210 >> iscsi_target_login_thread+0x816/0x1710 [iscsi_target_mod] >> kthread+0x1a0/0x1c0 >> ret_from_fork+0x35/0x40 >> >> Freed by task 980: >> __kasan_slab_free+0x125/0x170 >> kfree+0x90/0x1d0 >> iscsi_target_login_thread+0x1577/0x1710 [iscsi_target_mod] >> kthread+0x1a0/0x1c0 >> ret_from_fork+0x35/0x40 >> >> The buggy address belongs to the object at 880109d06f00 >> which belongs to the cache kmalloc-512 of size 512 >> The buggy address is located 456 bytes inside of >> 512-byte region [880109d06f00, 880109d07100) >> The buggy address belongs to the page: >> page:ea0004274180 count:1 mapcount:0 mapping: index:0x0 >> compound_mapcount: 0 >> flags: 0x17fffc08100(slab|head) >> raw: 017fffc08100 0001000c000c >> raw: dead0100 dead0200 88011b002e00 >> page dumped because: kasan: bad access detected >> >> Memory state around the buggy address: >> 880109d06f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >> 880109d07000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>> 880109d07080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >> ^ >> 880109d07100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >> 880109d07180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> == >> >> Signed-off-by: Vincent Pelletier >> --- >> drivers/target/iscsi/iscsi_target_login.c | 9 - >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/target/iscsi/iscsi_target_login.c >> b/drivers/target/iscsi/iscsi_target_login.c >> index df81b2f7cad9..c95f4261a089 100644 >> --- a/drivers/target/iscsi/iscsi_target_login.c >> +++ b/drivers/target/iscsi/iscsi_target_login.c >> @@ -296,10 +296,8 @@ static int iscsi_login_zero_tsih_s1( >> } >> >> ret = iscsi_login_set_conn_values(sess, conn, pdu->cid); >> -if (unlikely(ret)) { >> -kfree(sess); >> -return ret; >> -} >> +if (unlikely(ret)) >> +goto free_sess; >> sess->init_task_tag = pdu->itt; >> memcpy(&sess->isid, pdu->isid, 6); >> sess->exp_cmd_sn= be32_to_cpu(pdu->cmdsn); >> @@ -333,6 +331,7 @@ static int iscsi_login_zero_tsih_s1( >> pr_err("idr_alloc() for sess_idr failed\n"); >> iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, >> ISCSI_LOGIN_STATUS_NO_RESOURCES); >> +ret = -ENOMEM; Sorry to cause more confusion. I misunderstood what you were saying in the other mails. Here, I think it will be ok to not set ret. >> goto free_sess; >> } But, you need to set ret = -ENOMEM in the goto remove_idr and goto free_ops paths below the above chunk, because ret will be set to >= 0 at those points and the caller checks for ret < 0. >> >> @@ -370,7 +369,7 @@ static int iscsi_login_zero_tsih_s1( >> free_sess: >> kfree(sess); >> conn->sess = NULL; >> -return -ENOMEM; >> +return ret; >> } >> >> static int iscsi_login_zero_tsih_s2( >> > > Reviewed-by: Mike Christie > Please drop that reviewed-by.
Re: iscsi target: Set conn->sess to NULL when iscsi_login_set_conn_values fails
On 08/15/2018 05:19 AM, Vincent Pelletier wrote: > Fixes a use-after-free reported by KASAN when later > iscsi_target_login_sess_out gets called and it tries to access > conn->sess->se_sess: > > Disabling lock debugging due to kernel taint > iSCSI Login timeout on Network Portal [::]:3260 > iSCSI Login negotiation failed. > == > BUG: KASAN: use-after-free in iscsi_target_login_sess_out.cold.12+0x58/0xff > [iscsi_target_mod] > Read of size 8 at addr 880109d070c8 by task iscsi_np/980 > > CPU: 1 PID: 980 Comm: iscsi_np Tainted: G O > 4.17.8kasan.sess.connops+ #4 > Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS > 5.6.5 05/19/2014 > Call Trace: > dump_stack+0x71/0xac > print_address_description+0x65/0x22e > ? iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod] > kasan_report.cold.6+0x241/0x2fd > iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod] > iscsi_target_login_thread+0x1086/0x1710 [iscsi_target_mod] > ? __sched_text_start+0x8/0x8 > ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod] > ? __kthread_parkme+0xcc/0x100 > ? parse_args.cold.14+0xd3/0xd3 > ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod] > kthread+0x1a0/0x1c0 > ? kthread_bind+0x30/0x30 > ret_from_fork+0x35/0x40 > > Allocated by task 980: > kasan_kmalloc+0xbf/0xe0 > kmem_cache_alloc_trace+0x112/0x210 > iscsi_target_login_thread+0x816/0x1710 [iscsi_target_mod] > kthread+0x1a0/0x1c0 > ret_from_fork+0x35/0x40 > > Freed by task 980: > __kasan_slab_free+0x125/0x170 > kfree+0x90/0x1d0 > iscsi_target_login_thread+0x1577/0x1710 [iscsi_target_mod] > kthread+0x1a0/0x1c0 > ret_from_fork+0x35/0x40 > > The buggy address belongs to the object at 880109d06f00 > which belongs to the cache kmalloc-512 of size 512 > The buggy address is located 456 bytes inside of > 512-byte region [880109d06f00, 880109d07100) > The buggy address belongs to the page: > page:ea0004274180 count:1 mapcount:0 mapping: index:0x0 > compound_mapcount: 0 > flags: 0x17fffc08100(slab|head) > raw: 017fffc08100 0001000c000c > raw: dead0100 dead0200 88011b002e00 > page dumped because: kasan: bad access detected > > Memory state around the buggy address: > 880109d06f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > 880109d07000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >> 880109d07080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ^ > 880109d07100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc > 880109d07180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > == > > Signed-off-by: Vincent Pelletier > --- > drivers/target/iscsi/iscsi_target_login.c | 9 - > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/target/iscsi/iscsi_target_login.c > b/drivers/target/iscsi/iscsi_target_login.c > index df81b2f7cad9..c95f4261a089 100644 > --- a/drivers/target/iscsi/iscsi_target_login.c > +++ b/drivers/target/iscsi/iscsi_target_login.c > @@ -296,10 +296,8 @@ static int iscsi_login_zero_tsih_s1( > } > > ret = iscsi_login_set_conn_values(sess, conn, pdu->cid); > - if (unlikely(ret)) { > - kfree(sess); > - return ret; > - } > + if (unlikely(ret)) > + goto free_sess; > sess->init_task_tag = pdu->itt; > memcpy(&sess->isid, pdu->isid, 6); > sess->exp_cmd_sn= be32_to_cpu(pdu->cmdsn); > @@ -333,6 +331,7 @@ static int iscsi_login_zero_tsih_s1( > pr_err("idr_alloc() for sess_idr failed\n"); > iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, > ISCSI_LOGIN_STATUS_NO_RESOURCES); > + ret = -ENOMEM; > goto free_sess; > } > > @@ -370,7 +369,7 @@ static int iscsi_login_zero_tsih_s1( > free_sess: > kfree(sess); > conn->sess = NULL; > - return -ENOMEM; > + return ret; > } > > static int iscsi_login_zero_tsih_s2( > Reviewed-by: Mike Christie Martin, this was made over that patch that was going through Mathew's tree. It is in next here https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/target/iscsi?id=ad86353cbddbb6f1c95072ead744d547a9ac8211
Re: iscsi target: Set conn->sess to NULL when iscsi_login_set_conn_values fails
On 08/15/2018 10:44 AM, Mike Christie wrote: > On 08/15/2018 05:19 AM, Vincent Pelletier wrote: >> Fixes a use-after-free reported by KASAN when later >> iscsi_target_login_sess_out gets called and it tries to access >> conn->sess->se_sess: >> >> Disabling lock debugging due to kernel taint >> iSCSI Login timeout on Network Portal [::]:3260 >> iSCSI Login negotiation failed. >> == >> BUG: KASAN: use-after-free in iscsi_target_login_sess_out.cold.12+0x58/0xff >> [iscsi_target_mod] >> Read of size 8 at addr 880109d070c8 by task iscsi_np/980 >> >> CPU: 1 PID: 980 Comm: iscsi_np Tainted: G O >> 4.17.8kasan.sess.connops+ #4 >> Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS >> 5.6.5 05/19/2014 >> Call Trace: >> dump_stack+0x71/0xac >> print_address_description+0x65/0x22e >> ? iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod] >> kasan_report.cold.6+0x241/0x2fd >> iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod] >> iscsi_target_login_thread+0x1086/0x1710 [iscsi_target_mod] >> ? __sched_text_start+0x8/0x8 >> ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod] >> ? __kthread_parkme+0xcc/0x100 >> ? parse_args.cold.14+0xd3/0xd3 >> ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod] >> kthread+0x1a0/0x1c0 >> ? kthread_bind+0x30/0x30 >> ret_from_fork+0x35/0x40 >> >> Allocated by task 980: >> kasan_kmalloc+0xbf/0xe0 >> kmem_cache_alloc_trace+0x112/0x210 >> iscsi_target_login_thread+0x816/0x1710 [iscsi_target_mod] >> kthread+0x1a0/0x1c0 >> ret_from_fork+0x35/0x40 >> >> Freed by task 980: >> __kasan_slab_free+0x125/0x170 >> kfree+0x90/0x1d0 >> iscsi_target_login_thread+0x1577/0x1710 [iscsi_target_mod] >> kthread+0x1a0/0x1c0 >> ret_from_fork+0x35/0x40 >> >> The buggy address belongs to the object at 880109d06f00 >> which belongs to the cache kmalloc-512 of size 512 >> The buggy address is located 456 bytes inside of >> 512-byte region [880109d06f00, 880109d07100) >> The buggy address belongs to the page: >> page:ea0004274180 count:1 mapcount:0 mapping: index:0x0 >> compound_mapcount: 0 >> flags: 0x17fffc08100(slab|head) >> raw: 017fffc08100 0001000c000c >> raw: dead0100 dead0200 88011b002e00 >> page dumped because: kasan: bad access detected >> >> Memory state around the buggy address: >> 880109d06f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >> 880109d07000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>> 880109d07080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >> ^ >> 880109d07100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >> 880109d07180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> == >> >> Signed-off-by: Vincent Pelletier >> --- >> drivers/target/iscsi/iscsi_target_login.c | 9 - >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/target/iscsi/iscsi_target_login.c >> b/drivers/target/iscsi/iscsi_target_login.c >> index df81b2f7cad9..c95f4261a089 100644 >> --- a/drivers/target/iscsi/iscsi_target_login.c >> +++ b/drivers/target/iscsi/iscsi_target_login.c >> @@ -296,10 +296,8 @@ static int iscsi_login_zero_tsih_s1( >> } >> >> ret = iscsi_login_set_conn_values(sess, conn, pdu->cid); >> -if (unlikely(ret)) { >> -kfree(sess); >> -return ret; >> -} >> +if (unlikely(ret)) >> +goto free_sess; >> sess->init_task_tag = pdu->itt; >> memcpy(&sess->isid, pdu->isid, 6); >> sess->exp_cmd_sn= be32_to_cpu(pdu->cmdsn); >> @@ -333,6 +331,7 @@ static int iscsi_login_zero_tsih_s1( >> pr_err("idr_alloc() for sess_idr failed\n"); >> iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, >> ISCSI_LOGIN_STATUS_NO_RESOURCES); >> +ret = -ENOMEM; >> goto free_sess; >> } >> >> @@ -370,7 +369,7 @@ static int iscsi_login_zero_tsih_s1( >> free_sess: >> kfree(sess); >> conn->sess = NULL; >> -return -ENOMEM; >> +return ret; >> } >> >> static int iscsi_login_zero_tsih_s2( >> > > This is the issue I said was fixed in: > > https://www.spinics.net/lists/target-devel/msg17018.html > Sorry. I see your patch is made over mine. You are right. We need your patch too, because iscsi_login_set_conn_values does not clear the sess on failure before returning. The new return value in your patch fine.
Re: iscsi target: Set conn->sess to NULL when iscsi_login_set_conn_values fails
On Wed, 15 Aug 2018 10:44:34 -0500, Mike Christie wrote: > This is the issue I said was fixed in: > > https://www.spinics.net/lists/target-devel/msg17018.html I did apply this patch, yes. It misses the "if(...){kfree(sess); return ret;}" right after the iscsi_login_set_conn_values call, which is what my patch addresses. > Your patch still has issues where the sess_ops can be double freed, > and there is a issue with the idr. I believe you mean the other issues fixed by the patch at above URL. And as I applied that patch, I indeed did not touch these. Regards, -- Vincent Pelletier
Re: iscsi target: Set conn->sess to NULL when iscsi_login_set_conn_values fails
On 08/15/2018 05:19 AM, Vincent Pelletier wrote: > Fixes a use-after-free reported by KASAN when later > iscsi_target_login_sess_out gets called and it tries to access > conn->sess->se_sess: > > Disabling lock debugging due to kernel taint > iSCSI Login timeout on Network Portal [::]:3260 > iSCSI Login negotiation failed. > == > BUG: KASAN: use-after-free in iscsi_target_login_sess_out.cold.12+0x58/0xff > [iscsi_target_mod] > Read of size 8 at addr 880109d070c8 by task iscsi_np/980 > > CPU: 1 PID: 980 Comm: iscsi_np Tainted: G O > 4.17.8kasan.sess.connops+ #4 > Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS > 5.6.5 05/19/2014 > Call Trace: > dump_stack+0x71/0xac > print_address_description+0x65/0x22e > ? iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod] > kasan_report.cold.6+0x241/0x2fd > iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod] > iscsi_target_login_thread+0x1086/0x1710 [iscsi_target_mod] > ? __sched_text_start+0x8/0x8 > ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod] > ? __kthread_parkme+0xcc/0x100 > ? parse_args.cold.14+0xd3/0xd3 > ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod] > kthread+0x1a0/0x1c0 > ? kthread_bind+0x30/0x30 > ret_from_fork+0x35/0x40 > > Allocated by task 980: > kasan_kmalloc+0xbf/0xe0 > kmem_cache_alloc_trace+0x112/0x210 > iscsi_target_login_thread+0x816/0x1710 [iscsi_target_mod] > kthread+0x1a0/0x1c0 > ret_from_fork+0x35/0x40 > > Freed by task 980: > __kasan_slab_free+0x125/0x170 > kfree+0x90/0x1d0 > iscsi_target_login_thread+0x1577/0x1710 [iscsi_target_mod] > kthread+0x1a0/0x1c0 > ret_from_fork+0x35/0x40 > > The buggy address belongs to the object at 880109d06f00 > which belongs to the cache kmalloc-512 of size 512 > The buggy address is located 456 bytes inside of > 512-byte region [880109d06f00, 880109d07100) > The buggy address belongs to the page: > page:ea0004274180 count:1 mapcount:0 mapping: index:0x0 > compound_mapcount: 0 > flags: 0x17fffc08100(slab|head) > raw: 017fffc08100 0001000c000c > raw: dead0100 dead0200 88011b002e00 > page dumped because: kasan: bad access detected > > Memory state around the buggy address: > 880109d06f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > 880109d07000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >> 880109d07080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ^ > 880109d07100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc > 880109d07180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > == > > Signed-off-by: Vincent Pelletier > --- > drivers/target/iscsi/iscsi_target_login.c | 9 - > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/target/iscsi/iscsi_target_login.c > b/drivers/target/iscsi/iscsi_target_login.c > index df81b2f7cad9..c95f4261a089 100644 > --- a/drivers/target/iscsi/iscsi_target_login.c > +++ b/drivers/target/iscsi/iscsi_target_login.c > @@ -296,10 +296,8 @@ static int iscsi_login_zero_tsih_s1( > } > > ret = iscsi_login_set_conn_values(sess, conn, pdu->cid); > - if (unlikely(ret)) { > - kfree(sess); > - return ret; > - } > + if (unlikely(ret)) > + goto free_sess; > sess->init_task_tag = pdu->itt; > memcpy(&sess->isid, pdu->isid, 6); > sess->exp_cmd_sn= be32_to_cpu(pdu->cmdsn); > @@ -333,6 +331,7 @@ static int iscsi_login_zero_tsih_s1( > pr_err("idr_alloc() for sess_idr failed\n"); > iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, > ISCSI_LOGIN_STATUS_NO_RESOURCES); > + ret = -ENOMEM; > goto free_sess; > } > > @@ -370,7 +369,7 @@ static int iscsi_login_zero_tsih_s1( > free_sess: > kfree(sess); > conn->sess = NULL; > - return -ENOMEM; > + return ret; > } > > static int iscsi_login_zero_tsih_s2( > This is the issue I said was fixed in: https://www.spinics.net/lists/target-devel/msg17018.html Your patch still has issues where the sess_ops can be double freed, and there is a issue with the idr.
Re: iscsi target: Set conn->sess to NULL when iscsi_login_set_conn_values fails
On Wed, 15 Aug 2018 10:19:14 +, Vincent Pelletier wrote: > Fixes a use-after-free reported by KASAN when later > iscsi_target_login_sess_out gets called and it tries to access > conn->sess->se_sess: I could still hit this issue by causing a timeout, and located the guilty kfree: > ret = iscsi_login_set_conn_values(sess, conn, pdu->cid); Here, conn->sess is set. > - if (unlikely(ret)) { > - kfree(sess); This is the guilty kfree. > + ret = -ENOMEM; This is just to be strictly compliant with the hardcoded return value which I'm replacing with "ret". I tend to think this is wrong (hiding a possibly more relevant error code ?), but I do not know the surrounding code nearly enough to make a decision - so status-quo it is. Regards, -- Vincent Pelletier