linux-kernel - Re: Re: [PATCH] target: Fix a double put in transport_free

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 Mar 2021 15:48:11 +0800 (GMT+08:00)
From:   lyl2019@...l.ustc.edu.cn
To:     michael.christie@...cle.com
Cc:     martin.petersen@...cle.com, linux-scsi@...r.kernel.org,
        target-devel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: Re: [PATCH] target: Fix a double put in transport_free_session




> -----原始邮件-----
> 发件人: michael.christie@...cle.com
> 发送时间: 2021-03-24 00:28:35 (星期三)
> 收件人: "Lv Yunlong" <lyl2019@...l.ustc.edu.cn>, martin.petersen@...cle.com
> 抄送: linux-scsi@...r.kernel.org, target-devel@...r.kernel.org, linux-kernel@...r.kernel.org
> 主题: Re: [PATCH] target: Fix a double put in transport_free_session
> 
> On 3/22/21 9:58 PM, Lv Yunlong wrote:
> > In transport_free_session, se_nacl is got from se_sess
> > with the initial reference. If se_nacl->acl_sess_list is
> > empty, se_nacl->dynamic_stop is set to true. Then the first
> > target_put_nacl(se_nacl) will drop the initial reference
> > and free se_nacl. Later there is a second target_put_nacl()
> > to put se_nacl. It may cause error in race.
> >> My patch sets se_nacl->dynamic_stop to false to avoid the
> > double put.
> > 
> > Signed-off-by: Lv Yunlong <lyl2019@...l.ustc.edu.cn>
> > ---
> >  drivers/target/target_core_transport.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 5ecb9f18a53d..c266defe694f 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -584,8 +584,10 @@ void transport_free_session(struct se_session *se_sess)
> >  		}
> >  		mutex_unlock(&se_tpg->acl_node_mutex);
> >  
> > -		if (se_nacl->dynamic_stop)
> > +		if (se_nacl->dynamic_stop) {
> >  			target_put_nacl(se_nacl);
> > +			se_nacl->dynamic_stop = false;
> > +		}
> >  
> >  		target_put_nacl(se_nacl);
> Could you describe the race a little more?
> 
> Is the race:
> 
> 1. thread1 called core_tpg_check_initiator_node_acl and found the acl.
> sess->se_node_acl is set to the found acl.
> 2. thread2 is running transport_free_session. It now grabs the acl_node_mutex
> and sees se_nacl->acl_sess_list is empty.
> 3. thread2 does the dynamic_stop=true operations in transport_free_session.
> 4. thread1 now calls transport_register_session now adds the sess to acl's
> acl_sess_list.
> 
> Later when the session that thread 1 created is deleted dynamic_stop is still
> set, so we do an extra target_put_nacl?
> 
> I'm not sure your patch will handle this race. When we delete the session thread1
> created dynamic_node_acl is still set, so this:
> 
>                 mutex_lock(&se_tpg->acl_node_mutex);
>                 if (se_nacl->dynamic_node_acl &&
>                     !se_tfo->tpg_check_demo_mode_cache(se_tpg)) {
>                         spin_lock_irqsave(&se_nacl->nacl_sess_lock, flags);
>                         if (list_empty(&se_nacl->acl_sess_list))
>                                 se_nacl->dynamic_stop = true;
> 
> can set dynamic_stop to true again and we can end up doing the extra put still.
> 
> On top of the extra put we also do
> 
> list_del(&se_nacl->acl_list);
> 
> twice so we have to handle that as well.
> 
> Is there also another bug in this code. If someone adds an acl while there is a
> dynamic acl in place core_tpg_add_initiator_node_acl will clear dynamic_node_acl
> but we leave the extra reference, so later when transport_free_session is called
> we will not do the extra put.
> 

Ok, thanks for your answer. According the description above, i think it is a false
positive now.

Thanks.