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

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

[Lv Yunlong]

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?

Fixes: b850bbff96512 ("net/mlx5e: kTLS, Use refcounts to free kTLS RX priv context")
Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
---
 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)
-- 
2.25.1

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

[Maxim Mikityanskiy]

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@mail.ustc.edu.cn>
> ---
>   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)
> 

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

[lyl2019]

> -----原始邮件-----
> 发件人: "Maxim Mikityanskiy" <maximmi@nvidia.com>
> 发送时间: 2021-03-23 16:52:07 (星期二)
> 收件人: "Lv Yunlong" <lyl2019@mail.ustc.edu.cn>, borisp@nvidia.com, saeedm@nvidia.com, leon@kernel.org, davem@davemloft.net, kuba@kernel.org, maximmi@mellanox.com
> 抄送: netdev@vger.kernel.org, linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
> 主题: 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@mail.ustc.edu.cn>
> > ---
> >   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 !