lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Mon, 31 Aug 2020 02:12:45 +0530
From:   Anmol Karn <anmol.karan123@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     syzbot+0bef568258653cff272f@...kaller.appspotmail.com,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com,
        netdev@...r.kernel.org, linux-bluetooth@...r.kernel.org,
        kuba@...nel.org, davem@...emloft.net
Subject: Re: [Linux-kernel-mentees] [PATCH] net: bluetooth: Fix null pointer
 deref in hci_phy_link_complete_evt

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@...kaller.appspotmail.com
> > > > Link: https://syzkaller.appspot.com/bug?id=0d93140da5a82305a66a136af99b088b75177b99
> > > > Signed-off-by: Anmol Karn <anmol.karan123@...il.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

Powered by blists - more mailing lists