Re: Re: [PATCH] net/mlx5: Fix a potential use after free in mlx5e_ktls_del_rx

[lyl2019]

> -----原始邮件-----
> 发件人: "Maxim Mikityanskiy" <maximmi@xxxxxxxxxx>
> 发送时间: 2021-03-23 16:52:07 (星期二)
> 收件人: "Lv Yunlong" <lyl2019@xxxxxxxxxxxxxxxx>, borisp@xxxxxxxxxx, saeedm@xxxxxxxxxx, leon@xxxxxxxxxx, davem@xxxxxxxxxxxxx, kuba@xxxxxxxxxx, maximmi@xxxxxxxxxxxx
> 抄送: netdev@xxxxxxxxxxxxxxx, linux-rdma@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx
> 主题: Re: [PATCH] net/mlx5: Fix a potential use after free in mlx5e_ktls_del_rx
> 
> On 2021-03-22 16:21, Lv Yunlong wrote:
> > My static analyzer tool reported a potential uaf in
> > mlx5e_ktls_del_rx. In this function, if the condition
> > cancel_work_sync(&resync->work) is true, and then
> > priv_rx could be freed. But priv_rx is used later.
> > 
> > I'm unfamiliar with how this function works. Maybe the
> > maintainer forgot to add return after freeing priv_rx?
> 
> Thanks for running a static analyzer over our code! Sadly, the fix is 
> not correct and breaks stuff, and there is no problem with this code.
> 
> First of all, mlx5e_ktls_priv_rx_put doesn't necessarily free priv_rx. 
> It decrements the refcount and frees the object only when the refcount 
> goes to zero. Unless there are other bugs, the refcount in this branch 
> is not expected to go to zero, so there is no use-after-free in the code 
> below. The corresponding elevation of the refcount happens before 
> queue_work of resync->work. So, no, we haven't forgot to add a return, 
> we just expect priv_rx to stay alive after this call, and we want to run 
> the cleanup code below this `if`, while your fix skips the cleanup and 
> skips the second mlx5e_ktls_priv_rx_put in the end of this function, 
> leading to a memory leak.
> 
> If you'd like to calm down the static analyzer, you could try to add a 
> WARN_ON assertion to check that mlx5e_ktls_priv_rx_put returns false in 
> that `if` (meaning that the object hasn't been freed). If would be nice 
> to have this WARN_ON regardless of static analyzers.
> 
> > Fixes: b850bbff96512 ("net/mlx5e: kTLS, Use refcounts to free kTLS RX priv context")
> > Signed-off-by: Lv Yunlong <lyl2019@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c
> > index d06532d0baa4..54a77df42316 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c
> > @@ -663,8 +663,10 @@ void mlx5e_ktls_del_rx(struct net_device *netdev, struct tls_context *tls_ctx)
> >   		 */
> >   		wait_for_completion(&priv_rx->add_ctx);
> >   	resync = &priv_rx->resync;
> > -	if (cancel_work_sync(&resync->work))
> > +	if (cancel_work_sync(&resync->work)) {
> >   		mlx5e_ktls_priv_rx_put(priv_rx);
> > +		return;
> > +	}
> >   
> >   	priv_rx->stats->tls_del++;
> >   	if (priv_rx->rule.rule)
> > 
> 

Ok, it is a good idea. 

Thank you for your generous advice !