Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1611609 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] bluetooth: 6lowpan: fix use after free in chan_suspend/resume Michael Scott <michael.scott@linaro.org> - 2017-03-29 08:20 +0200
    Re: [PATCH] bluetooth: 6lowpan: fix use after free in chan_suspend/resume Luiz Augusto von Dentz <luiz.dentz@gmail.com> - 2017-03-31 10:40 +0200
    Re: [PATCH] bluetooth: 6lowpan: fix use after free in  chan_suspend/resume Jukka Rissanen <jukka.rissanen@linux.intel.com> - 2017-03-31 11:40 +0200
    Re: [PATCH] bluetooth: 6lowpan: fix use after free in  chan_suspend/resume Marcel Holtmann <marcel@holtmann.org> - 2017-03-31 12:20 +0200

#1611609 — [PATCH] bluetooth: 6lowpan: fix use after free in chan_suspend/resume

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@linaro.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

[toc] | [next] | [standalone]

#1613740

Hi Michael,

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

[toc] | [prev] | [next] | [standalone]

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

Hi Michael,

On Tue, 2017-03-28 at 23:10 -0700, Michael Scott 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@linaro.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)

Good catch! If we can avoid using the status variable, that is very
good. We could probably also remove the resume and suspend callbacks as
they are now empty functions (unless we need the debug info for
something).

Acked-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>


Cheers,
Jukka

[toc] | [prev] | [next] | [standalone]

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

Hi Michael,

> 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@linaro.org>
> ---
> net/bluetooth/6lowpan.c | 21 +++------------------
> 1 file changed, 3 insertions(+), 18 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web