diff mbox series

[Linux-kernel-mentees,net] Bluetooth: Fix NULL pointer dereference in amp_read_loc_assoc_final_data()

Message ID 20200808040440.255578-1-yepeilin.cs@gmail.com (mailing list archive)
State New, archived
Headers show
Series [Linux-kernel-mentees,net] Bluetooth: Fix NULL pointer dereference in amp_read_loc_assoc_final_data() | expand

Commit Message

Peilin Ye Aug. 8, 2020, 4:04 a.m. UTC 
Prevent amp_read_loc_assoc_final_data() from dereferencing `mgr` as NULL.

Reported-and-tested-by: syzbot+f4fb0eaafdb51c32a153@syzkaller.appspotmail.com
Fixes: 9495b2ee757f ("Bluetooth: AMP: Process Chan Selected event")
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
 net/bluetooth/amp.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Dmitry Vyukov March 3, 2021, 8:21 a.m. UTC | #1 
On Sat, Aug 8, 2020 at 6:06 AM Peilin Ye <yepeilin.cs@gmail.com> wrote:
>
> Prevent amp_read_loc_assoc_final_data() from dereferencing `mgr` as NULL.
>
> Reported-and-tested-by: syzbot+f4fb0eaafdb51c32a153@syzkaller.appspotmail.com
> Fixes: 9495b2ee757f ("Bluetooth: AMP: Process Chan Selected event")
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> ---
>  net/bluetooth/amp.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/bluetooth/amp.c b/net/bluetooth/amp.c
> index 9c711f0dfae3..be2d469d6369 100644
> --- a/net/bluetooth/amp.c
> +++ b/net/bluetooth/amp.c
> @@ -297,6 +297,9 @@ void amp_read_loc_assoc_final_data(struct hci_dev *hdev,
>         struct hci_request req;
>         int err;
>
> +       if (!mgr)
> +               return;
> +
>         cp.phy_handle = hcon->handle;
>         cp.len_so_far = cpu_to_le16(0);
>         cp.max_len = cpu_to_le16(hdev->amp_assoc_size);

Not sure what happened here, but the merged patch somehow has a
different author and no Reported-by tag:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e8bd76ede155fd54d8c41d045dda43cd3174d506
so let's tell syzbot what fixed it manually:
#syz fix:
Bluetooth: Fix null pointer dereference in amp_read_loc_assoc_final_data
Gopal Tiwari March 3, 2021, 10:11 a.m. UTC | #2 
Hi, 

I tried to search the patch for one of the bugzilla reported (Internal) https://bugzilla.redhat.com/show_bug.cgi?id=1916057 with the traces 

[  405.938525] Workqueue: hci0 hci_rx_work [bluetooth]
[  405.941360] RIP: 0010:amp_read_loc_assoc_final_data+0xfc/0x1c0 [bluetooth]
[  405.944740] Code: 89 44 24 29 48 b8 00 00 00 00 00 fc ff df 0f b6 04 02 84 c0 74 08 3c 01 0f 8e 9d 00 00 00 0f b7 85 c0 03 00 00 66 89 44 24 2b <f0> 41 80 4c 24 30 04 4c 8d 64 24 68 48 89 ee 4c 89 e7 e8 3d 48 fe
[  405.952396] RSP: 0018:ffff88802ea0f838 EFLAGS: 00010246
[  405.955368] RAX: 0000000000000000 RBX: 1ffff11005d41f08 RCX: dffffc0000000000
[  405.958669] RDX: 1ffff110254cc878 RSI: ffff88802eeee000 RDI: ffff88812a6643c0
[  405.961980] RBP: ffff88812a664000 R08: 0000000000000000 R09: 0000000000000000
[  405.965319] R10: ffff88802ea0fd00 R11: 0000000000000000 R12: 0000000000000000
[  405.968624] R13: 0000000000000041 R14: ffff88802b836800 R15: ffff8881250570c0
[  405.971989] FS:  0000000000000000(0000) GS:ffff888055a00000(0000) knlGS:0000000000000000
[  405.975645] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  405.978755] CR2: 0000000000000030 CR3: 000000002d200000 CR4: 0000000000340ee0
[  405.982150] Call Trace:
[  405.984768]  ? amp_read_loc_assoc+0x170/0x170 [bluetooth]
[  405.987875]  ? rcu_read_unlock+0x50/0x50
[  405.990663]  ? deref_stack_reg+0xf0/0xf0
[  405.993403]  ? __module_address+0x3f/0x370
[  405.996184]  ? hci_cmd_work+0x180/0x330 [bluetooth]
[  405.999170]  ? hci_conn_hash_lookup_handle+0x1a1/0x270 [bluetooth]
[  406.002354]  hci_event_packet+0x1476/0x7e00 [bluetooth]
[  406.005407]  ? arch_stack_walk+0x8f/0xf0
[  406.008206]  ? ret_from_fork+0x27/0x50
[  406.010887]  ? hci_cmd_complete_evt+0xbf70/0xbf70 [bluetooth]
[  406.013933]  ? stack_trace_save+0x8a/0xb0
[  406.016618]  ? do_profile_hits.isra.4.cold.9+0x2d/0x2d
[  406.019483]  ? lock_acquire+0x1a3/0x970
[  406.022092]  ? __wake_up_common_lock+0xaf/0x130


I didn't found any solution upstream. After the vmcore analysis I found what is wrong. And took reference from the following patch, which seems to be on the similar line 

commit 6dfccd13db2ff2b709ef60a50163925d477549aa
    Author: Anmol Karn <anmol.karan123@gmail.com>
    Date:   Wed Sep 30 19:48:13 2020 +0530
    
        Bluetooth: Fix null pointer dereference in hci_event_packet()
    
        AMP_MGR is getting derefernced in hci_phy_link_complete_evt(), when called
        from hci_event_packet() and there is a possibility, that hcon->amp_mgr may
        not be found when accessing after initialization of hcon.
    
        - net/bluetooth/hci_event.c:4945

How we can avoid this scenario. So I made the chages and tested. It worked or avoided the kernel panic. But I really don't know that some one has already posted the patch. I would have love to backport the patch, I was more of looking for the fix. That's where I didn't applied the reported-by tag as I thought it reported internal only. 

Thanks & regards, 
Gopal Tiwari 



----- Original Message -----
From: "Dmitry Vyukov" <dvyukov@google.com>
To: "Peilin Ye" <yepeilin.cs@gmail.com>
Cc: "Marcel Holtmann" <marcel@holtmann.org>, "Johan Hedberg" <johan.hedberg@gmail.com>, "Andrei Emeltchenko" <andrei.emeltchenko@intel.com>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "David S. Miller" <davem@davemloft.net>, "Jakub Kicinski" <kuba@kernel.org>, linux-kernel-mentees@lists.linuxfoundation.org, "syzkaller-bugs" <syzkaller-bugs@googlegroups.com>, "linux-bluetooth" <linux-bluetooth@vger.kernel.org>, "netdev" <netdev@vger.kernel.org>, "LKML" <linux-kernel@vger.kernel.org>, gtiwari@redhat.com, syzbot+f4fb0eaafdb51c32a153@syzkaller.appspotmail.com
Sent: Wednesday, March 3, 2021 1:51:41 PM
Subject: Re: [Linux-kernel-mentees] [PATCH net] Bluetooth: Fix NULL pointer dereference in amp_read_loc_assoc_final_data()

On Sat, Aug 8, 2020 at 6:06 AM Peilin Ye <yepeilin.cs@gmail.com> wrote:
>
> Prevent amp_read_loc_assoc_final_data() from dereferencing `mgr` as NULL.
>
> Reported-and-tested-by: syzbot+f4fb0eaafdb51c32a153@syzkaller.appspotmail.com
> Fixes: 9495b2ee757f ("Bluetooth: AMP: Process Chan Selected event")
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> ---
>  net/bluetooth/amp.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/bluetooth/amp.c b/net/bluetooth/amp.c
> index 9c711f0dfae3..be2d469d6369 100644
> --- a/net/bluetooth/amp.c
> +++ b/net/bluetooth/amp.c
> @@ -297,6 +297,9 @@ void amp_read_loc_assoc_final_data(struct hci_dev *hdev,
>         struct hci_request req;
>         int err;
>
> +       if (!mgr)
> +               return;
> +
>         cp.phy_handle = hcon->handle;
>         cp.len_so_far = cpu_to_le16(0);
>         cp.max_len = cpu_to_le16(hdev->amp_assoc_size);

Not sure what happened here, but the merged patch somehow has a
different author and no Reported-by tag:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e8bd76ede155fd54d8c41d045dda43cd3174d506
so let's tell syzbot what fixed it manually:
#syz fix:
Bluetooth: Fix null pointer dereference in amp_read_loc_assoc_final_data
Dmitry Vyukov March 3, 2021, 10:27 a.m. UTC | #3 
On Wed, Mar 3, 2021 at 11:11 AM Gopal Tiwari <gtiwari@redhat.com> wrote:
>
> Hi,
>
> I tried to search the patch for one of the bugzilla reported (Internal) https://bugzilla.redhat.com/show_bug.cgi?id=1916057 with the traces
>
> [  405.938525] Workqueue: hci0 hci_rx_work [bluetooth]
> [  405.941360] RIP: 0010:amp_read_loc_assoc_final_data+0xfc/0x1c0 [bluetooth]
> [  405.944740] Code: 89 44 24 29 48 b8 00 00 00 00 00 fc ff df 0f b6 04 02 84 c0 74 08 3c 01 0f 8e 9d 00 00 00 0f b7 85 c0 03 00 00 66 89 44 24 2b <f0> 41 80 4c 24 30 04 4c 8d 64 24 68 48 89 ee 4c 89 e7 e8 3d 48 fe
> [  405.952396] RSP: 0018:ffff88802ea0f838 EFLAGS: 00010246
> [  405.955368] RAX: 0000000000000000 RBX: 1ffff11005d41f08 RCX: dffffc0000000000
> [  405.958669] RDX: 1ffff110254cc878 RSI: ffff88802eeee000 RDI: ffff88812a6643c0
> [  405.961980] RBP: ffff88812a664000 R08: 0000000000000000 R09: 0000000000000000
> [  405.965319] R10: ffff88802ea0fd00 R11: 0000000000000000 R12: 0000000000000000
> [  405.968624] R13: 0000000000000041 R14: ffff88802b836800 R15: ffff8881250570c0
> [  405.971989] FS:  0000000000000000(0000) GS:ffff888055a00000(0000) knlGS:0000000000000000
> [  405.975645] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  405.978755] CR2: 0000000000000030 CR3: 000000002d200000 CR4: 0000000000340ee0
> [  405.982150] Call Trace:
> [  405.984768]  ? amp_read_loc_assoc+0x170/0x170 [bluetooth]
> [  405.987875]  ? rcu_read_unlock+0x50/0x50
> [  405.990663]  ? deref_stack_reg+0xf0/0xf0
> [  405.993403]  ? __module_address+0x3f/0x370
> [  405.996184]  ? hci_cmd_work+0x180/0x330 [bluetooth]
> [  405.999170]  ? hci_conn_hash_lookup_handle+0x1a1/0x270 [bluetooth]
> [  406.002354]  hci_event_packet+0x1476/0x7e00 [bluetooth]
> [  406.005407]  ? arch_stack_walk+0x8f/0xf0
> [  406.008206]  ? ret_from_fork+0x27/0x50
> [  406.010887]  ? hci_cmd_complete_evt+0xbf70/0xbf70 [bluetooth]
> [  406.013933]  ? stack_trace_save+0x8a/0xb0
> [  406.016618]  ? do_profile_hits.isra.4.cold.9+0x2d/0x2d
> [  406.019483]  ? lock_acquire+0x1a3/0x970
> [  406.022092]  ? __wake_up_common_lock+0xaf/0x130
>
>
> I didn't found any solution upstream. After the vmcore analysis I found what is wrong. And took reference from the following patch, which seems to be on the similar line
>
> commit 6dfccd13db2ff2b709ef60a50163925d477549aa
>     Author: Anmol Karn <anmol.karan123@gmail.com>
>     Date:   Wed Sep 30 19:48:13 2020 +0530
>
>         Bluetooth: Fix null pointer dereference in hci_event_packet()
>
>         AMP_MGR is getting derefernced in hci_phy_link_complete_evt(), when called
>         from hci_event_packet() and there is a possibility, that hcon->amp_mgr may
>         not be found when accessing after initialization of hcon.
>
>         - net/bluetooth/hci_event.c:4945
>
> How we can avoid this scenario. So I made the chages and tested. It worked or avoided the kernel panic. But I really don't know that some one has already posted the patch. I would have love to backport the patch, I was more of looking for the fix. That's where I didn't applied the reported-by tag as I thought it reported internal only.

Hi Gopal,

I think it's somewhat inherent to the current kernel unstructured
processes with bugs being reported on mailing lists, bugzilla,
distro-specific trackers.
One useful thing, though, is searching Lore, e.g. searching for just
the crashing function:
https://lore.kernel.org/lkml/?q=amp_read_loc_assoc_final_data
gives the report and the patch (if we filter out all entries produced
by your patch, which obviously wasn't yet there before you wrote it
:)):

12. [Linux-kernel-mentees] [PATCH net] Bluetooth: Fix NULL pointer
dereference in amp_read_loc_assoc_final_data()
    - by Peilin Ye @ 2020-08-08  4:04 UTC [21%]

13. KASAN: null-ptr-deref Write in amp_read_loc_assoc_final_data
    - by syzbot @ 2020-07-31 17:04 UTC [13%]


> Thanks & regards,
> Gopal Tiwari
>
>
>
> ----- Original Message -----
> From: "Dmitry Vyukov" <dvyukov@google.com>
> To: "Peilin Ye" <yepeilin.cs@gmail.com>
> Cc: "Marcel Holtmann" <marcel@holtmann.org>, "Johan Hedberg" <johan.hedberg@gmail.com>, "Andrei Emeltchenko" <andrei.emeltchenko@intel.com>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "David S. Miller" <davem@davemloft.net>, "Jakub Kicinski" <kuba@kernel.org>, linux-kernel-mentees@lists.linuxfoundation.org, "syzkaller-bugs" <syzkaller-bugs@googlegroups.com>, "linux-bluetooth" <linux-bluetooth@vger.kernel.org>, "netdev" <netdev@vger.kernel.org>, "LKML" <linux-kernel@vger.kernel.org>, gtiwari@redhat.com, syzbot+f4fb0eaafdb51c32a153@syzkaller.appspotmail.com
> Sent: Wednesday, March 3, 2021 1:51:41 PM
> Subject: Re: [Linux-kernel-mentees] [PATCH net] Bluetooth: Fix NULL pointer dereference in amp_read_loc_assoc_final_data()
>
> On Sat, Aug 8, 2020 at 6:06 AM Peilin Ye <yepeilin.cs@gmail.com> wrote:
> >
> > Prevent amp_read_loc_assoc_final_data() from dereferencing `mgr` as NULL.
> >
> > Reported-and-tested-by: syzbot+f4fb0eaafdb51c32a153@syzkaller.appspotmail.com
> > Fixes: 9495b2ee757f ("Bluetooth: AMP: Process Chan Selected event")
> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> > ---
> >  net/bluetooth/amp.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/net/bluetooth/amp.c b/net/bluetooth/amp.c
> > index 9c711f0dfae3..be2d469d6369 100644
> > --- a/net/bluetooth/amp.c
> > +++ b/net/bluetooth/amp.c
> > @@ -297,6 +297,9 @@ void amp_read_loc_assoc_final_data(struct hci_dev *hdev,
> >         struct hci_request req;
> >         int err;
> >
> > +       if (!mgr)
> > +               return;
> > +
> >         cp.phy_handle = hcon->handle;
> >         cp.len_so_far = cpu_to_le16(0);
> >         cp.max_len = cpu_to_le16(hdev->amp_assoc_size);
>
> Not sure what happened here, but the merged patch somehow has a
> different author and no Reported-by tag:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e8bd76ede155fd54d8c41d045dda43cd3174d506
> so let's tell syzbot what fixed it manually:
> #syz fix:
> Bluetooth: Fix null pointer dereference in amp_read_loc_assoc_final_data
>
diff mbox series

Patch

diff --git a/net/bluetooth/amp.c b/net/bluetooth/amp.c
index 9c711f0dfae3..be2d469d6369 100644
--- a/net/bluetooth/amp.c
+++ b/net/bluetooth/amp.c
@@ -297,6 +297,9 @@  void amp_read_loc_assoc_final_data(struct hci_dev *hdev,
 	struct hci_request req;
 	int err;
 
+	if (!mgr)
+		return;
+
 	cp.phy_handle = hcon->handle;
 	cp.len_so_far = cpu_to_le16(0);
 	cp.max_len = cpu_to_le16(hdev->amp_assoc_size);