lkml.org 
[lkml]   [2021]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: Re: [PATCH] net/mlx5: Fix a potential use after free in mlx5e_ktls_del_rx



    > -----原始邮件-----
    > 发件人: "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 !
    \
     
     \ /
      Last update: 2021-03-23 14:41    [W:4.300 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site