Re: [PATCH] net: Fix potential NULL pointer dereference in __skb_try_recv_datagram

[Eric Dumazet]

On Wed, 2016-01-20 at 16:06 +0100, Jacob Siverskog wrote:
> On Tue, Jan 5, 2016 at 3:39 PM, Eric Dumazet <eric.duma...@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.duma...@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.

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;
 }