> -----原始邮件-----
> 发件人: "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 !
