| Message ID | 20200829165712.229437-1-anmol.karan123@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Linux-kernel-mentees] net: bluetooth: Fix null pointer deref in hci_phy_link_complete_evt | expand |
On Sat, Aug 29, 2020 at 10:27:12PM +0530, Anmol Karn wrote: > Fix null pointer deref in hci_phy_link_complete_evt, there was no > checking there for the hcon->amp_mgr->l2cap_conn->hconn, and also > in hci_cmd_work, for hdev->sent_cmd. > > To fix this issue Add pointer checking in hci_cmd_work and > hci_phy_link_complete_evt. > [Linux-next-20200827] > > This patch corrected some mistakes from previous patch. > > Reported-by: syzbot+0bef568258653cff272f@syzkaller.appspotmail.com > Link: https://syzkaller.appspot.com/bug?id=0d93140da5a82305a66a136af99b088b75177b99 > Signed-off-by: Anmol Karn <anmol.karan123@gmail.com> > --- > net/bluetooth/hci_core.c | 5 ++++- > net/bluetooth/hci_event.c | 4 ++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index 68bfe57b6625..996efd654e7a 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -4922,7 +4922,10 @@ static void hci_cmd_work(struct work_struct *work) > > kfree_skb(hdev->sent_cmd); > > - hdev->sent_cmd = skb_clone(skb, GFP_KERNEL); > + if (hdev->sent_cmd) { > + hdev->sent_cmd = skb_clone(skb, GFP_KERNEL); > + } How can sent_cmd be NULL here? Are you sure something previous to this shouldn't be fixed instead? > + > if (hdev->sent_cmd) { > if (hci_req_status_pend(hdev)) > hci_dev_set_flag(hdev, HCI_CMD_PENDING); > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index 4b7fc430793c..1e7d9bee9111 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -4941,6 +4941,10 @@ static void hci_phy_link_complete_evt(struct hci_dev *hdev, > hci_dev_unlock(hdev); > return; > } > + if (!(hcon->amp_mgr->l2cap_conn->hcon)) { > + hci_dev_unlock(hdev); > + return; > + } How can this be triggered? thanks, greg k-h
On Sun, Aug 30, 2020 at 11:19:17AM +0200, Greg KH wrote: > On Sat, Aug 29, 2020 at 10:27:12PM +0530, Anmol Karn wrote: > > Fix null pointer deref in hci_phy_link_complete_evt, there was no > > checking there for the hcon->amp_mgr->l2cap_conn->hconn, and also > > in hci_cmd_work, for hdev->sent_cmd. > > > > To fix this issue Add pointer checking in hci_cmd_work and > > hci_phy_link_complete_evt. > > [Linux-next-20200827] > > > > This patch corrected some mistakes from previous patch. > > > > Reported-by: syzbot+0bef568258653cff272f@syzkaller.appspotmail.com > > Link: https://syzkaller.appspot.com/bug?id=0d93140da5a82305a66a136af99b088b75177b99 > > Signed-off-by: Anmol Karn <anmol.karan123@gmail.com> > > --- > > net/bluetooth/hci_core.c | 5 ++++- > > net/bluetooth/hci_event.c | 4 ++++ > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > > index 68bfe57b6625..996efd654e7a 100644 > > --- a/net/bluetooth/hci_core.c > > +++ b/net/bluetooth/hci_core.c > > @@ -4922,7 +4922,10 @@ static void hci_cmd_work(struct work_struct *work) > > > > kfree_skb(hdev->sent_cmd); > > > > - hdev->sent_cmd = skb_clone(skb, GFP_KERNEL); > > + if (hdev->sent_cmd) { > > + hdev->sent_cmd = skb_clone(skb, GFP_KERNEL); > > + } > > How can sent_cmd be NULL here? Are you sure something previous to this > shouldn't be fixed instead? Sir, sent_cmd was freed before this condition check, thats why i checked it, i think i should check it before the free of hdev->sent_cmd like, if (hdev->sent_cmd) kfree_skb(hdev->sent_cmd); what's your opininon on this. > > > > + > > if (hdev->sent_cmd) { > > if (hci_req_status_pend(hdev)) > > hci_dev_set_flag(hdev, HCI_CMD_PENDING); > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > > index 4b7fc430793c..1e7d9bee9111 100644 > > --- a/net/bluetooth/hci_event.c > > +++ b/net/bluetooth/hci_event.c > > @@ -4941,6 +4941,10 @@ static void hci_phy_link_complete_evt(struct hci_dev *hdev, > > hci_dev_unlock(hdev); > > return; > > } > > + if (!(hcon->amp_mgr->l2cap_conn->hcon)) { > > + hci_dev_unlock(hdev); > > + return; > > + } > > How can this be triggered? syzbot showed that this line is accessed irrespective of the null value it contains, so added a pointer check for that. please give some opinions on this, if (!bredr_hcon) { hci_dev_unlock(hdev); return; } Thanks, Anmol Karn
On Sun, Aug 30, 2020 at 05:56:23PM +0530, Anmol Karn wrote: > On Sun, Aug 30, 2020 at 11:19:17AM +0200, Greg KH wrote: > > On Sat, Aug 29, 2020 at 10:27:12PM +0530, Anmol Karn wrote: > > > Fix null pointer deref in hci_phy_link_complete_evt, there was no > > > checking there for the hcon->amp_mgr->l2cap_conn->hconn, and also > > > in hci_cmd_work, for hdev->sent_cmd. > > > > > > To fix this issue Add pointer checking in hci_cmd_work and > > > hci_phy_link_complete_evt. > > > [Linux-next-20200827] > > > > > > This patch corrected some mistakes from previous patch. > > > > > > Reported-by: syzbot+0bef568258653cff272f@syzkaller.appspotmail.com > > > Link: https://syzkaller.appspot.com/bug?id=0d93140da5a82305a66a136af99b088b75177b99 > > > Signed-off-by: Anmol Karn <anmol.karan123@gmail.com> > > > --- > > > net/bluetooth/hci_core.c | 5 ++++- > > > net/bluetooth/hci_event.c | 4 ++++ > > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > > > index 68bfe57b6625..996efd654e7a 100644 > > > --- a/net/bluetooth/hci_core.c > > > +++ b/net/bluetooth/hci_core.c > > > @@ -4922,7 +4922,10 @@ static void hci_cmd_work(struct work_struct *work) > > > > > > kfree_skb(hdev->sent_cmd); > > > > > > - hdev->sent_cmd = skb_clone(skb, GFP_KERNEL); > > > + if (hdev->sent_cmd) { > > > + hdev->sent_cmd = skb_clone(skb, GFP_KERNEL); > > > + } > > > > How can sent_cmd be NULL here? Are you sure something previous to this > > shouldn't be fixed instead? > > Sir, sent_cmd was freed before this condition check, thats why i checked it, But it can not be NULL at that point in time, as nothing set it to NULL, correct? > i think i should check it before the free of hdev->sent_cmd like, > > if (hdev->sent_cmd) > kfree_skb(hdev->sent_cmd); No, that's not needed. What is the problem with these lines that you are trying to solve? > > > + > > > if (hdev->sent_cmd) { > > > if (hci_req_status_pend(hdev)) > > > hci_dev_set_flag(hdev, HCI_CMD_PENDING); > > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > > > index 4b7fc430793c..1e7d9bee9111 100644 > > > --- a/net/bluetooth/hci_event.c > > > +++ b/net/bluetooth/hci_event.c > > > @@ -4941,6 +4941,10 @@ static void hci_phy_link_complete_evt(struct hci_dev *hdev, > > > hci_dev_unlock(hdev); > > > return; > > > } > > > + if (!(hcon->amp_mgr->l2cap_conn->hcon)) { > > > + hci_dev_unlock(hdev); > > > + return; > > > + } > > > > How can this be triggered? > > syzbot showed that this line is accessed irrespective of the null value it contains, so added a > pointer check for that. But does hcon->amp_mgr->l2cap_conn->hcon become NULL here? thanks, greg k-h
On Sun, Aug 30, 2020 at 07:30:10PM +0200, Greg KH wrote: > On Sun, Aug 30, 2020 at 05:56:23PM +0530, Anmol Karn wrote: > > On Sun, Aug 30, 2020 at 11:19:17AM +0200, Greg KH wrote: > > > On Sat, Aug 29, 2020 at 10:27:12PM +0530, Anmol Karn wrote: > > > > Fix null pointer deref in hci_phy_link_complete_evt, there was no > > > > checking there for the hcon->amp_mgr->l2cap_conn->hconn, and also > > > > in hci_cmd_work, for hdev->sent_cmd. > > > > > > > > To fix this issue Add pointer checking in hci_cmd_work and > > > > hci_phy_link_complete_evt. > > > > [Linux-next-20200827] > > > > > > > > This patch corrected some mistakes from previous patch. > > > > > > > > Reported-by: syzbot+0bef568258653cff272f@syzkaller.appspotmail.com > > > > Link: https://syzkaller.appspot.com/bug?id=0d93140da5a82305a66a136af99b088b75177b99 > > > > Signed-off-by: Anmol Karn <anmol.karan123@gmail.com> > > > > --- > > > > net/bluetooth/hci_core.c | 5 ++++- > > > > net/bluetooth/hci_event.c | 4 ++++ > > > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > > > > index 68bfe57b6625..996efd654e7a 100644 > > > > --- a/net/bluetooth/hci_core.c > > > > +++ b/net/bluetooth/hci_core.c > > > > @@ -4922,7 +4922,10 @@ static void hci_cmd_work(struct work_struct *work) > > > > > > > > kfree_skb(hdev->sent_cmd); > > > > > > > > - hdev->sent_cmd = skb_clone(skb, GFP_KERNEL); > > > > + if (hdev->sent_cmd) { > > > > + hdev->sent_cmd = skb_clone(skb, GFP_KERNEL); > > > > + } > > > > > > How can sent_cmd be NULL here? Are you sure something previous to this > > > shouldn't be fixed instead? > > > > Sir, sent_cmd was freed before this condition check, thats why i checked it, > > But it can not be NULL at that point in time, as nothing set it to NULL, > correct? > > > i think i should check it before the free of hdev->sent_cmd like, > > > > if (hdev->sent_cmd) > > kfree_skb(hdev->sent_cmd); > > No, that's not needed. > > What is the problem with these lines that you are trying to solve? > > > > > + > > > > if (hdev->sent_cmd) { > > > > if (hci_req_status_pend(hdev)) > > > > hci_dev_set_flag(hdev, HCI_CMD_PENDING); > > > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > > > > index 4b7fc430793c..1e7d9bee9111 100644 > > > > --- a/net/bluetooth/hci_event.c > > > > +++ b/net/bluetooth/hci_event.c > > > > @@ -4941,6 +4941,10 @@ static void hci_phy_link_complete_evt(struct hci_dev *hdev, > > > > hci_dev_unlock(hdev); > > > > return; > > > > } > > > > + if (!(hcon->amp_mgr->l2cap_conn->hcon)) { > > > > + hci_dev_unlock(hdev); > > > > + return; > > > > + } > > > > > > How can this be triggered? > > > > syzbot showed that this line is accessed irrespective of the null value it contains, so added a > > pointer check for that. > > But does hcon->amp_mgr->l2cap_conn->hcon become NULL here? Sir, according to the report obtained by running decode_stacktrace on logs there is something getting null at this line, after verifying the buggy address i thought it would be better to check this whole line. will dig more deeper into this and will make appropriate changes in the next version, thanks for review. Anmol Karn
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 68bfe57b6625..996efd654e7a 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -4922,7 +4922,10 @@ static void hci_cmd_work(struct work_struct *work) kfree_skb(hdev->sent_cmd); - hdev->sent_cmd = skb_clone(skb, GFP_KERNEL); + if (hdev->sent_cmd) { + hdev->sent_cmd = skb_clone(skb, GFP_KERNEL); + } + if (hdev->sent_cmd) { if (hci_req_status_pend(hdev)) hci_dev_set_flag(hdev, HCI_CMD_PENDING); diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 4b7fc430793c..1e7d9bee9111 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -4941,6 +4941,10 @@ static void hci_phy_link_complete_evt(struct hci_dev *hdev, hci_dev_unlock(hdev); return; } + if (!(hcon->amp_mgr->l2cap_conn->hcon)) { + hci_dev_unlock(hdev); + return; + } bredr_hcon = hcon->amp_mgr->l2cap_conn->hcon;
Fix null pointer deref in hci_phy_link_complete_evt, there was no checking there for the hcon->amp_mgr->l2cap_conn->hconn, and also in hci_cmd_work, for hdev->sent_cmd. To fix this issue Add pointer checking in hci_cmd_work and hci_phy_link_complete_evt. [Linux-next-20200827] This patch corrected some mistakes from previous patch. Reported-by: syzbot+0bef568258653cff272f@syzkaller.appspotmail.com Link: https://syzkaller.appspot.com/bug?id=0d93140da5a82305a66a136af99b088b75177b99 Signed-off-by: Anmol Karn <anmol.karan123@gmail.com> --- net/bluetooth/hci_core.c | 5 ++++- net/bluetooth/hci_event.c | 4 ++++ 2 files changed, 8 insertions(+), 1 deletion(-)