Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1644187 > unrolled thread

[PATCH 4.4 49/56] Bluetooth: hci_bcm: add missing tty-device sanity check

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4.4 49/56] Bluetooth: hci_bcm: add missing tty-device sanity check Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-05-18 13:10 +0200
    Re: [PATCH 4.4 49/56] Bluetooth: hci_bcm: add missing tty-device  sanity check Ben Hutchings <ben.hutchings@codethink.co.uk> - 2017-05-24 03:40 +0200
      Re: [PATCH 4.4 49/56] Bluetooth: hci_bcm: add missing tty-device  sanity check Johan Hovold <johan@kernel.org> - 2017-05-24 12:20 +0200

#1644187 — [PATCH 4.4 49/56] Bluetooth: hci_bcm: add missing tty-device sanity check

4.4-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Johan Hovold <johan@kernel.org>

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 <frederic.danis@linux.intel.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

[toc] | [next] | [standalone]

#1649049 — Re: [PATCH 4.4 49/56] Bluetooth: hci_bcm: add missing tty-device sanity check

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 <johan@kernel.org>
> 
> 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 <frederic.danis@linux.intel.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> ---
>  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, 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.

Ben.

-- 
Ben Hutchings
Software Developer, Codethink Ltd.

[toc] | [prev] | [next] | [standalone]

#1649422 — Re: [PATCH 4.4 49/56] Bluetooth: hci_bcm: add missing tty-device sanity check

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 <johan@kernel.org>
> > 
> > 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 <frederic.danis@linux.intel.com>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > ---
> >  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
	...
	[<bf0d5d74>] (bcm_open [hci_uart]) from [<bf0d1284>] (hci_uart_tty_ioctl+0x1b0/0x3c8 [hci_uart])
	[<bf0d1284>] (hci_uart_tty_ioctl [hci_uart]) from [<c0465bcc>] (tty_ioctl+0x5bc/0xbc8)
	[<c0465bcc>] (tty_ioctl) from [<c0258ad4>] (do_vfs_ioctl+0xac/0x9e8)
	[<c0258ad4>] (do_vfs_ioctl) from [<c0259454>] (SyS_ioctl+0x44/0x6c)
	[<c0259454>] (SyS_ioctl) from [<c01092e0>] (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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web