Re: [PATCH] bluetooth: 6lowpan: fix use after free in chan_suspend/resume

[Luiz Augusto von Dentz]

Hi Michael,

On Wed, Mar 29, 2017 at 9:10 AM, Michael Scott <michael.scott@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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