lkml.org 
[lkml]   [2021]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: Re: Re: [Drbd-dev] [PATCH] drbd: Fix a use after free in get_initial_state



    > -----原始邮件-----
    > 发件人: lyl2019@mail.ustc.edu.cn
    > 发送时间: 2021-04-01 23:13:58 (星期四)
    > 收件人: "Christoph Böhmwalder" <christoph.boehmwalder@linbit.com>
    > 抄送: philipp.reisner@linbit.com, lars.ellenberg@linbit.com, axboe@kernel.dk, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, drbd-dev@lists.linbit.com
    > 主题: Re: Re: [Drbd-dev] [PATCH] drbd: Fix a use after free in get_initial_state
    >
    >
    >
    >
    > > -----原始邮件-----
    > > 发件人: "Christoph Böhmwalder" <christoph.boehmwalder@linbit.com>
    > > 发送时间: 2021-04-01 21:01:20 (星期四)
    > > 收件人: "Lv Yunlong" <lyl2019@mail.ustc.edu.cn>
    > > 抄送: philipp.reisner@linbit.com, lars.ellenberg@linbit.com, axboe@kernel.dk, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, drbd-dev@lists.linbit.com
    > > 主题: Re: [Drbd-dev] [PATCH] drbd: Fix a use after free in get_initial_state
    > >
    > > On 4/1/21 1:57 PM, Lv Yunlong wrote:
    > > > In get_initial_state, it calls notify_initial_state_done(skb,..) if
    > > > cb->args[5]==1. I see that if genlmsg_put() failed in
    > > > notify_initial_state_done(), the skb will be freed by nlmsg_free(skb).
    > > > Then get_initial_state will goto out and the freed skb will be used by
    > > > return value skb->len.
    > > >
    > > > My patch lets skb_len = skb->len and return the skb_len to avoid the uaf.
    > > >
    > > > Fixes: a29728463b254 ("drbd: Backport the "events2" command")
    > > > Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
    > > > ---
    > > > drivers/block/drbd/drbd_nl.c | 3 ++-
    > > > 1 file changed, 2 insertions(+), 1 deletion(-)
    > > >
    > > > diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
    > > > index bf7de4c7b96c..474f84675d0a 100644
    > > > --- a/drivers/block/drbd/drbd_nl.c
    > > > +++ b/drivers/block/drbd/drbd_nl.c
    > > > @@ -4905,6 +4905,7 @@ static int get_initial_state(struct sk_buff *skb, struct netlink_callback *cb)
    > > > struct drbd_state_change *state_change = (struct drbd_state_change *)cb->args[0];
    > > > unsigned int seq = cb->args[2];
    > > > unsigned int n;
    > > > + unsigned int skb_len = skb->len;
    > > > enum drbd_notification_type flags = 0;
    > > >
    > > > /* There is no need for taking notification_mutex here: it doesn't
    > > > @@ -4915,7 +4916,7 @@ static int get_initial_state(struct sk_buff *skb, struct netlink_callback *cb)
    > > > cb->args[5]--;
    > > > if (cb->args[5] == 1) {
    > > > notify_initial_state_done(skb, seq);
    > > > - goto out;
    > > > + return skb_len;
    > > > }
    > > > n = cb->args[4]++;
    > > > if (cb->args[4] < cb->args[3])
    > > >
    > >
    > > Thanks for the patch!
    > >
    > > I think the problem goes even further: skb can also be freed in the
    > > notify_*_state_change -> notify_*_state calls below.
    > >
    > > Also, at the point where we save skb->len into skb_len, skb is not
    > > initialized yet. Maybe it makes more sense to not return a length in the
    > > first place here, but an error code instead.
    > >
    > > --
    > > Christoph Böhmwalder
    > > LINBIT | Keeping the Digital World Running
    > > DRBD HA — Disaster Recovery — Software defined Storage
    >
    > Ok, I see.
    > I found that drbd_adm_get_initial_state() has called the get_initial_state(),
    > and return -ENOMEM if it calls remember_old_state() failed.
    >
    > So, i think that means if get_initial_state() failed on the notify_initial_state_done(),
    > it should return -ENOMEM too.
    >
    > I will submit the PATCH v2 to fix the first place. The fixes of the further problem is
    > hard for me.
    >
    > Thanks.

    I found that notify_initial_state_done() uses err = -EMSGSIZE, so the first place should
    return -EMSGSIZE not -ENOMEM. Sorry.
    \
     
     \ /
      Last update: 2021-04-01 19:54    [W:3.241 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site