From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-f66.google.com ([209.85.215.66]:36243 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933569AbdEXKTU (ORCPT ); Wed, 24 May 2017 06:19:20 -0400 Date: Wed, 24 May 2017 12:19:13 +0200 From: Johan Hovold To: Ben Hutchings Cc: Johan Hovold , Marcel Holtmann , linux-kernel@vger.kernel.org, stable@vger.kernel.org, Frederic Danis , Greg Kroah-Hartman Subject: Re: [PATCH 4.4 49/56] Bluetooth: hci_bcm: add missing tty-device sanity check Message-ID: <20170524101913.GH3657@localhost> References: <20170518104840.395932131@linuxfoundation.org> <20170518104842.471187557@linuxfoundation.org> <1495589815.2083.7.camel@codethink.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1495589815.2083.7.camel@codethink.co.uk> Sender: stable-owner@vger.kernel.org List-ID: On Wed, May 24, 2017 at 02:36:55AM +0100, Ben Hutchings wrote: > On Thu, 2017-05-18 at 12:49 +0200, Greg Kroah-Hartman wrote: > > 4.4-stable review patch. If anyone has any objections, please let me know. > > > > ------------------ > > > > From: Johan Hovold > > > > commit 95065a61e9bf25fb85295127fba893200c2bbbd8 upstream. > > > > Make sure to check the tty-device pointer before looking up the sibling > > platform device to avoid dereferencing a NULL-pointer when the tty is > > one end of a Unix98 pty. > > > > Fixes: 0395ffc1ee05 ("Bluetooth: hci_bcm: Add PM for BCM devices") > > Cc: Frederic Danis > > Signed-off-by: Johan Hovold > > Signed-off-by: Marcel Holtmann > > Signed-off-by: Greg Kroah-Hartman > > > > --- > > drivers/bluetooth/hci_bcm.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > --- a/drivers/bluetooth/hci_bcm.c > > +++ b/drivers/bluetooth/hci_bcm.c > > @@ -287,6 +287,9 @@ static int bcm_open(struct hci_uart *hu) > > > > hu->priv = bcm; > > > > + if (!hu->tty->dev) > > + goto out; > > + > > mutex_lock(&bcm_device_lock); > > list_for_each(p, &bcm_device_list) { > > struct bcm_device *dev = list_entry(p, struct bcm_device, list); > > @@ -307,7 +310,7 @@ static int bcm_open(struct hci_uart *hu) > > } > > > > mutex_unlock(&bcm_device_lock); > > - > > +out: > > return 0; > > } > > I'm a bit sceptical that this is fixing a real bug, Please take a look at bcm_open() where the tty device is being dereferenced whenever there is bcm platform device registered: list_for_each(p, &bcm_device_list) { struct bcm_device *dev = list_entry(p, struct bcm_device, list); ... if (hu->tty->dev->parent == dev->pdev->dev.parent) { This can be used by an unprivileged user to trigger a NULL-dereference: Unable to handle kernel NULL pointer dereference at virtual address 00000000 ... [] (bcm_open [hci_uart]) from [] (hci_uart_tty_ioctl+0x1b0/0x3c8 [hci_uart]) [] (hci_uart_tty_ioctl [hci_uart]) from [] (tty_ioctl+0x5bc/0xbc8) [] (tty_ioctl) from [] (do_vfs_ioctl+0xac/0x9e8) [] (do_vfs_ioctl) from [] (SyS_ioctl+0x44/0x6c) [] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x1c) > but if it is - > surely bcm_open() should fail if the tty device is not the right type? > bcm_setup() would certainly fail after this. The driver is written to be able to handle a non-existing platform device (e.g. see use of bcm_device_exists() in the commit introducing this issue), although since commit 6cc4396c8829 ("Bluetooth: hci_bcm: Add wake-up capability") setup would indeed fail in this case (well at least when firmware *is* available...). Perhaps that is a separate issue that should be fixed (by failing early in open as you suggest or by again allowing the driver to work without a platform device), but I believe this patch is correct nonetheless. And it does specifically fix the local DoS-attack. Thanks, Johan