diff mbox

net: Fix potential NULL pointer dereference in __skb_try_recv_datagram

Message ID 1453304914.1223.325.camel@edumazet-glaptop2.roam.corp.google.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet Jan. 20, 2016, 3:48 p.m. UTC 
On Wed, 2016-01-20 at 16:06 +0100, Jacob Siverskog wrote:
> On Tue, Jan 5, 2016 at 3:39 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Tue, 2016-01-05 at 15:34 +0100, Jacob Siverskog wrote:
> >> On Tue, Jan 5, 2016 at 3:14 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> >> >
> >> > You might build a kernel with KASAN support to get maybe more chances to
> >> > trigger the bug.
> >> >
> >> > ( https://www.kernel.org/doc/Documentation/kasan.txt )
> >> >
> >>
> >> Ah. Doesn't seem to be supported on arm(32) unfortunately.
> >
> > Then you could at least use standard debugging features :
> >
> > CONFIG_SLAB=y
> > CONFIG_SLABINFO=y
> > CONFIG_DEBUG_SLAB=y
> > CONFIG_DEBUG_SLAB_LEAK=y
> >
> > (Or equivalent SLUB options)
> >
> > and
> >
> > CONFIG_DEBUG_PAGEALLOC=y
> >
> > (If arm(32) has CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC=y)
> 
> I tried with those enabled and while toggling power on the Bluetooth
> interface I usually get this after a few iterations:
> kernel: Bluetooth: Unable to push skb to HCI core(-6)

Well, this code seems to be quite buggy.

I do not have time to audit it, but 5 minutes are enough to spot 2
issues.

skb, once given to another queue/layer should not be accessed anymore.

Comments

Jacob Siverskog Jan. 20, 2016, 4:17 p.m. UTC | #1 
On Wed, Jan 20, 2016 at 4:48 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2016-01-20 at 16:06 +0100, Jacob Siverskog wrote:
>> On Tue, Jan 5, 2016 at 3:39 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > On Tue, 2016-01-05 at 15:34 +0100, Jacob Siverskog wrote:
>> >> On Tue, Jan 5, 2016 at 3:14 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> >
>> >> >
>> >> > You might build a kernel with KASAN support to get maybe more chances to
>> >> > trigger the bug.
>> >> >
>> >> > ( https://www.kernel.org/doc/Documentation/kasan.txt )
>> >> >
>> >>
>> >> Ah. Doesn't seem to be supported on arm(32) unfortunately.
>> >
>> > Then you could at least use standard debugging features :
>> >
>> > CONFIG_SLAB=y
>> > CONFIG_SLABINFO=y
>> > CONFIG_DEBUG_SLAB=y
>> > CONFIG_DEBUG_SLAB_LEAK=y
>> >
>> > (Or equivalent SLUB options)
>> >
>> > and
>> >
>> > CONFIG_DEBUG_PAGEALLOC=y
>> >
>> > (If arm(32) has CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC=y)
>>
>> I tried with those enabled and while toggling power on the Bluetooth
>> interface I usually get this after a few iterations:
>> kernel: Bluetooth: Unable to push skb to HCI core(-6)
>
> Well, this code seems to be quite buggy.
>
> I do not have time to audit it, but 5 minutes are enough to spot 2
> issues.
>
> skb, once given to another queue/layer should not be accessed anymore.
>

Ok. Unfortunately I still see the slab corruption even with your changes.
Eric Dumazet Jan. 20, 2016, 5:02 p.m. UTC | #2 
On Wed, 2016-01-20 at 17:17 +0100, Jacob Siverskog wrote:
> On Wed, Jan 20, 2016 at 4:48 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Wed, 2016-01-20 at 16:06 +0100, Jacob Siverskog wrote:
> >> On Tue, Jan 5, 2016 at 3:39 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> > On Tue, 2016-01-05 at 15:34 +0100, Jacob Siverskog wrote:
> >> >> On Tue, Jan 5, 2016 at 3:14 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> >
> >> >> >
> >> >> > You might build a kernel with KASAN support to get maybe more chances to
> >> >> > trigger the bug.
> >> >> >
> >> >> > ( https://www.kernel.org/doc/Documentation/kasan.txt )
> >> >> >
> >> >>
> >> >> Ah. Doesn't seem to be supported on arm(32) unfortunately.
> >> >
> >> > Then you could at least use standard debugging features :
> >> >
> >> > CONFIG_SLAB=y
> >> > CONFIG_SLABINFO=y
> >> > CONFIG_DEBUG_SLAB=y
> >> > CONFIG_DEBUG_SLAB_LEAK=y
> >> >
> >> > (Or equivalent SLUB options)
> >> >
> >> > and
> >> >
> >> > CONFIG_DEBUG_PAGEALLOC=y
> >> >
> >> > (If arm(32) has CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC=y)
> >>
> >> I tried with those enabled and while toggling power on the Bluetooth
> >> interface I usually get this after a few iterations:
> >> kernel: Bluetooth: Unable to push skb to HCI core(-6)
> >
> > Well, this code seems to be quite buggy.
> >
> > I do not have time to audit it, but 5 minutes are enough to spot 2
> > issues.
> >
> > skb, once given to another queue/layer should not be accessed anymore.
> >
> 
> Ok. Unfortunately I still see the slab corruption even with your changes.

Patch was only showing potential _reads_ after free, which do not
generally corrupt memory.

As I said, a full audit is needed, and I don't have time for this.
diff mbox

Patch

diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
index 24a652f9252b..2d3092aa6cfe 100644
--- a/drivers/bluetooth/btwilink.c
+++ b/drivers/bluetooth/btwilink.c
@@ -98,6 +98,7 @@  static void st_reg_completion_cb(void *priv_data, char data)
 static long st_receive(void *priv_data, struct sk_buff *skb)
 {
 	struct ti_st *lhst = priv_data;
+	unsigned int len;
 	int err;
 
 	if (!skb)
@@ -109,13 +110,14 @@  static long st_receive(void *priv_data, struct sk_buff *skb)
 	}
 
 	/* Forward skb to HCI core layer */
+	len = skb->len;
 	err = hci_recv_frame(lhst->hdev, skb);
 	if (err < 0) {
 		BT_ERR("Unable to push skb to HCI core(%d)", err);
 		return err;
 	}
 
-	lhst->hdev->stat.byte_rx += skb->len;
+	lhst->hdev->stat.byte_rx += len;
 
 	return 0;
 }
@@ -245,6 +247,7 @@  static int ti_st_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct ti_st *hst;
 	long len;
+	u8 pkt_type;
 
 	hst = hci_get_drvdata(hdev);
 
@@ -258,6 +261,7 @@  static int ti_st_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 	 * Freeing skb memory is taken care in shared transport layer,
 	 * so don't free skb memory here.
 	 */
+	pkt_type = hci_skb_pkt_type(skb);
 	len = hst->st_write(skb);
 	if (len < 0) {
 		kfree_skb(skb);
@@ -268,7 +272,7 @@  static int ti_st_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 
 	/* ST accepted our skb. So, Go ahead and do rest */
 	hdev->stat.byte_tx += len;
-	ti_st_tx_complete(hst, hci_skb_pkt_type(skb));
+	ti_st_tx_complete(hst, pkt_type);
 
 	return 0;
 }