linux-kernel - Re: [PATCH] bluetooth: 6lowpan: fix use after free in chan

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABBYNZ+=W6kvxrmaTWktNTtc76hoDgYuZOK9QF5QNd+5+MSjNA@mail.gmail.com>
Date:   Fri, 31 Mar 2017 11:33:37 +0300
From:   Luiz Augusto von Dentz <luiz.dentz@...il.com>
To:     Michael Scott <michael.scott@...aro.org>
Cc:     Marcel Holtmann <marcel@...tmann.org>,
        Gustavo Padovan <gustavo@...ovan.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jukka Rissanen <jukka.rissanen@...ux.intel.com>,
        "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
        linux-wpan@...r.kernel.org,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] bluetooth: 6lowpan: fix use after free in chan_suspend/resume

Hi Michael,

On Wed, Mar 29, 2017 at 9:10 AM, Michael Scott <michael.scott@...aro.org> wrote:
> A status field in the skb_cb struct was storing a channel status
> based on channel suspend/resume events.  This stored status was
> then used to return EAGAIN if there were packet sending issues
> in snd_pkt().
>
> The issue is that the skb has been freed by the time the callback
> to 6lowpan's suspend/resume was called.  So, this generates a
> "use after free" issue that was noticed while running kernel tests
> with KASAN debug enabled.
>
> Let's eliminate the status field entirely as we can use the channel
> tx_credits to indicate whether we should return EAGAIN when handling
> packets.
>
> Signed-off-by: Michael Scott <michael.scott@...aro.org>
> ---
>  net/bluetooth/6lowpan.c | 21 +++------------------
>  1 file changed, 3 insertions(+), 18 deletions(-)
>
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index d491529332f4..e27be3ca0a0c 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -38,7 +38,6 @@ struct skb_cb {
>         struct in6_addr addr;
>         struct in6_addr gw;
>         struct l2cap_chan *chan;
> -       int status;
>  };
>  #define lowpan_cb(skb) ((struct skb_cb *)((skb)->cb))
>
> @@ -528,7 +527,7 @@ static int send_pkt(struct l2cap_chan *chan, struct sk_buff *skb,
>         }
>
>         if (!err)
> -               err = lowpan_cb(skb)->status;
> +               err = (!chan->tx_credits ? -EAGAIN : 0);
>
>         if (err < 0) {
>                 if (err == -EAGAIN)
> @@ -964,26 +963,12 @@ static struct sk_buff *chan_alloc_skb_cb(struct l2cap_chan *chan,
>
>  static void chan_suspend_cb(struct l2cap_chan *chan)
>  {
> -       struct sk_buff *skb = chan->data;
> -
> -       BT_DBG("chan %p conn %p skb %p", chan, chan->conn, skb);
> -
> -       if (!skb)
> -               return;
> -
> -       lowpan_cb(skb)->status = -EAGAIN;
> +       BT_DBG("chan %p suspend", chan);
>  }
>
>  static void chan_resume_cb(struct l2cap_chan *chan)
>  {
> -       struct sk_buff *skb = chan->data;
> -
> -       BT_DBG("chan %p conn %p skb %p", chan, chan->conn, skb);
> -
> -       if (!skb)
> -               return;
> -
> -       lowpan_cb(skb)->status = 0;
> +       BT_DBG("chan %p resume", chan);
>  }
>
>  static long chan_get_sndtimeo_cb(struct l2cap_chan *chan)
> --
> 2.11.0

It should be possible to queue the packets on l2cap_chan_send, Im not
sure why we have this suspend logic in the first place so perhaps
Jukka can shed some light here.



-- 
Luiz Augusto von Dentz