diff mbox series

Bluetooth: ensure valid channel mode when creating l2cap conn on LE

Message ID 20220112101731.77010-1-gav@thegavinli.com
State New
Headers show
Series Bluetooth: ensure valid channel mode when creating l2cap conn on LE | expand

Commit Message

Gavin Li Jan. 12, 2022, 10:17 a.m. UTC 
From: Gavin Li <git@thegavinli.com>

After creating a socket(AF_INET, SOCK_STREAM, BTPROTO_L2CAP) socket and
connect()'ing to a LE device with default settings (no setsockopt), upon
the first sendmsg, the following BUG occurs because chan->mode==L2CAP_MODE_ERTM,
causing l2cap_ertm_send() -> __set_retrans_timer() -> schedule_delayed_work()
on l2cap_chan.retrans_timer, which was never initialized because
l2cap_ertm_init() was never called to initialize it.

  Call Trace:
   queue_delayed_work_on+0x36/0x40
   l2cap_ertm_send.isra.0+0x14d/0x2d0 [bluetooth]
   l2cap_tx+0x361/0x510 [bluetooth]
   l2cap_chan_send+0xb26/0xb50 [bluetooth]
   l2cap_sock_sendmsg+0xc9/0x100 [bluetooth]
   sock_sendmsg+0x5e/0x60
   sock_write_iter+0x97/0x100
   new_sync_write+0x1d3/0x1f0
   vfs_write+0x1b4/0x270
   ksys_write+0xaf/0xe0
   do_syscall_64+0x33/0x40
   entry_SYSCALL_64_after_hwframe+0x44/0xa9

This patch ensures that when connecting to a LE device, chan->mode will
always be corrected to L2CAP_MODE_LE_FLOWCTL if it is invalid for LE.

Signed-off-by: Gavin Li <git@thegavinli.com>
---
 net/bluetooth/l2cap_sock.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Luiz Augusto von Dentz Jan. 14, 2022, 10:52 p.m. UTC | #1 
Hi,

On Wed, Jan 12, 2022 at 2:17 AM <gav@thegavinli.com> wrote:
>
> From: Gavin Li <git@thegavinli.com>
>
> After creating a socket(AF_INET, SOCK_STREAM, BTPROTO_L2CAP) socket and
> connect()'ing to a LE device with default settings (no setsockopt), upon
> the first sendmsg, the following BUG occurs because chan->mode==L2CAP_MODE_ERTM,
> causing l2cap_ertm_send() -> __set_retrans_timer() -> schedule_delayed_work()
> on l2cap_chan.retrans_timer, which was never initialized because
> l2cap_ertm_init() was never called to initialize it.
>
>   Call Trace:
>    queue_delayed_work_on+0x36/0x40
>    l2cap_ertm_send.isra.0+0x14d/0x2d0 [bluetooth]
>    l2cap_tx+0x361/0x510 [bluetooth]
>    l2cap_chan_send+0xb26/0xb50 [bluetooth]
>    l2cap_sock_sendmsg+0xc9/0x100 [bluetooth]
>    sock_sendmsg+0x5e/0x60
>    sock_write_iter+0x97/0x100
>    new_sync_write+0x1d3/0x1f0
>    vfs_write+0x1b4/0x270
>    ksys_write+0xaf/0xe0
>    do_syscall_64+0x33/0x40
>    entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> This patch ensures that when connecting to a LE device, chan->mode will
> always be corrected to L2CAP_MODE_LE_FLOWCTL if it is invalid for LE.
>
> Signed-off-by: Gavin Li <git@thegavinli.com>
> ---
>  net/bluetooth/l2cap_sock.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 160c016a5dfb9..58c06ef32656c 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -78,6 +78,17 @@ static int l2cap_validate_le_psm(u16 psm)
>         return 0;
>  }
>
> +static bool l2cap_mode_supports_le(u8 mode)
> +{
> +       switch (mode) {
> +               case L2CAP_MODE_LE_FLOWCTL:
> +               case L2CAP_MODE_EXT_FLOWCTL:
> +                       return true;
> +               default:
> +                       return false;
> +       }
> +}
> +
>  static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
>  {
>         struct sock *sk = sock->sk;
> @@ -161,7 +172,7 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
>                 break;
>         }
>
> -       if (chan->psm && bdaddr_type_is_le(chan->src_type))
> +       if (chan->psm && bdaddr_type_is_le(la.l2_bdaddr_type) && !l2cap_mode_supports_le(chan->mode))
>                 chan->mode = L2CAP_MODE_LE_FLOWCTL;
>
>         chan->state = BT_BOUND;
> @@ -240,7 +251,7 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr,
>                         return -EINVAL;
>         }
>
> -       if (chan->psm && bdaddr_type_is_le(chan->src_type) && !chan->mode)
> +       if (chan->psm && bdaddr_type_is_le(la.l2_bdaddr_type) && !l2cap_mode_supports_le(chan->mode))
>                 chan->mode = L2CAP_MODE_LE_FLOWCTL;
>
>         err = l2cap_chan_connect(chan, la.l2_psm, __le16_to_cpu(la.l2_cid),
> --
> 2.34.1

Doesn't apply to bluetooth-next:

https://github.com/bluez/bluez/issues/250
Luiz Augusto von Dentz Jan. 14, 2022, 11 p.m. UTC | #2 
Hi,

On Fri, Jan 14, 2022 at 2:52 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi,
>
> On Wed, Jan 12, 2022 at 2:17 AM <gav@thegavinli.com> wrote:
> >
> > From: Gavin Li <git@thegavinli.com>
> >
> > After creating a socket(AF_INET, SOCK_STREAM, BTPROTO_L2CAP) socket and
> > connect()'ing to a LE device with default settings (no setsockopt), upon
> > the first sendmsg, the following BUG occurs because chan->mode==L2CAP_MODE_ERTM,
> > causing l2cap_ertm_send() -> __set_retrans_timer() -> schedule_delayed_work()
> > on l2cap_chan.retrans_timer, which was never initialized because
> > l2cap_ertm_init() was never called to initialize it.
> >
> >   Call Trace:
> >    queue_delayed_work_on+0x36/0x40
> >    l2cap_ertm_send.isra.0+0x14d/0x2d0 [bluetooth]
> >    l2cap_tx+0x361/0x510 [bluetooth]
> >    l2cap_chan_send+0xb26/0xb50 [bluetooth]
> >    l2cap_sock_sendmsg+0xc9/0x100 [bluetooth]
> >    sock_sendmsg+0x5e/0x60
> >    sock_write_iter+0x97/0x100
> >    new_sync_write+0x1d3/0x1f0
> >    vfs_write+0x1b4/0x270
> >    ksys_write+0xaf/0xe0
> >    do_syscall_64+0x33/0x40
> >    entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >
> > This patch ensures that when connecting to a LE device, chan->mode will
> > always be corrected to L2CAP_MODE_LE_FLOWCTL if it is invalid for LE.
> >
> > Signed-off-by: Gavin Li <git@thegavinli.com>
> > ---
> >  net/bluetooth/l2cap_sock.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index 160c016a5dfb9..58c06ef32656c 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -78,6 +78,17 @@ static int l2cap_validate_le_psm(u16 psm)
> >         return 0;
> >  }
> >
> > +static bool l2cap_mode_supports_le(u8 mode)
> > +{
> > +       switch (mode) {
> > +               case L2CAP_MODE_LE_FLOWCTL:
> > +               case L2CAP_MODE_EXT_FLOWCTL:
> > +                       return true;
> > +               default:
> > +                       return false;
> > +       }
> > +}
> > +
> >  static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
> >  {
> >         struct sock *sk = sock->sk;
> > @@ -161,7 +172,7 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
> >                 break;
> >         }
> >
> > -       if (chan->psm && bdaddr_type_is_le(chan->src_type))
> > +       if (chan->psm && bdaddr_type_is_le(la.l2_bdaddr_type) && !l2cap_mode_supports_le(chan->mode))
> >                 chan->mode = L2CAP_MODE_LE_FLOWCTL;
> >
> >         chan->state = BT_BOUND;
> > @@ -240,7 +251,7 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr,
> >                         return -EINVAL;
> >         }
> >
> > -       if (chan->psm && bdaddr_type_is_le(chan->src_type) && !chan->mode)
> > +       if (chan->psm && bdaddr_type_is_le(la.l2_bdaddr_type) && !l2cap_mode_supports_le(chan->mode))
> >                 chan->mode = L2CAP_MODE_LE_FLOWCTL;
> >
> >         err = l2cap_chan_connect(chan, la.l2_psm, __le16_to_cpu(la.l2_cid),
> > --
> > 2.34.1
>
> Doesn't apply to bluetooth-next:
>
> https://github.com/bluez/bluez/issues/250

Please disregard the link above, Ive meant to paste:

Applying: Bluetooth: ensure valid channel mode when creating l2cap conn on LE
error: patch failed: net/bluetooth/l2cap_sock.c:161
error: net/bluetooth/l2cap_sock.c: patch does not apply

I did fix something similar:

https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=30d57722732d9736554f85f75f9d7ad5402d192e
Gavin Li Jan. 17, 2022, 7:48 a.m. UTC | #3 
This looks good; thanks for catching that.

Gavin


On Fri, Jan 14, 2022 at 3:00 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi,
>
> On Fri, Jan 14, 2022 at 2:52 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi,
> >
> > On Wed, Jan 12, 2022 at 2:17 AM <gav@thegavinli.com> wrote:
> > >
> > > From: Gavin Li <git@thegavinli.com>
> > >
> > > After creating a socket(AF_INET, SOCK_STREAM, BTPROTO_L2CAP) socket and
> > > connect()'ing to a LE device with default settings (no setsockopt), upon
> > > the first sendmsg, the following BUG occurs because chan->mode==L2CAP_MODE_ERTM,
> > > causing l2cap_ertm_send() -> __set_retrans_timer() -> schedule_delayed_work()
> > > on l2cap_chan.retrans_timer, which was never initialized because
> > > l2cap_ertm_init() was never called to initialize it.
> > >
> > >   Call Trace:
> > >    queue_delayed_work_on+0x36/0x40
> > >    l2cap_ertm_send.isra.0+0x14d/0x2d0 [bluetooth]
> > >    l2cap_tx+0x361/0x510 [bluetooth]
> > >    l2cap_chan_send+0xb26/0xb50 [bluetooth]
> > >    l2cap_sock_sendmsg+0xc9/0x100 [bluetooth]
> > >    sock_sendmsg+0x5e/0x60
> > >    sock_write_iter+0x97/0x100
> > >    new_sync_write+0x1d3/0x1f0
> > >    vfs_write+0x1b4/0x270
> > >    ksys_write+0xaf/0xe0
> > >    do_syscall_64+0x33/0x40
> > >    entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > >
> > > This patch ensures that when connecting to a LE device, chan->mode will
> > > always be corrected to L2CAP_MODE_LE_FLOWCTL if it is invalid for LE.
> > >
> > > Signed-off-by: Gavin Li <git@thegavinli.com>
> > > ---
> > >  net/bluetooth/l2cap_sock.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > > index 160c016a5dfb9..58c06ef32656c 100644
> > > --- a/net/bluetooth/l2cap_sock.c
> > > +++ b/net/bluetooth/l2cap_sock.c
> > > @@ -78,6 +78,17 @@ static int l2cap_validate_le_psm(u16 psm)
> > >         return 0;
> > >  }
> > >
> > > +static bool l2cap_mode_supports_le(u8 mode)
> > > +{
> > > +       switch (mode) {
> > > +               case L2CAP_MODE_LE_FLOWCTL:
> > > +               case L2CAP_MODE_EXT_FLOWCTL:
> > > +                       return true;
> > > +               default:
> > > +                       return false;
> > > +       }
> > > +}
> > > +
> > >  static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
> > >  {
> > >         struct sock *sk = sock->sk;
> > > @@ -161,7 +172,7 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
> > >                 break;
> > >         }
> > >
> > > -       if (chan->psm && bdaddr_type_is_le(chan->src_type))
> > > +       if (chan->psm && bdaddr_type_is_le(la.l2_bdaddr_type) && !l2cap_mode_supports_le(chan->mode))
> > >                 chan->mode = L2CAP_MODE_LE_FLOWCTL;
> > >
> > >         chan->state = BT_BOUND;
> > > @@ -240,7 +251,7 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr,
> > >                         return -EINVAL;
> > >         }
> > >
> > > -       if (chan->psm && bdaddr_type_is_le(chan->src_type) && !chan->mode)
> > > +       if (chan->psm && bdaddr_type_is_le(la.l2_bdaddr_type) && !l2cap_mode_supports_le(chan->mode))
> > >                 chan->mode = L2CAP_MODE_LE_FLOWCTL;
> > >
> > >         err = l2cap_chan_connect(chan, la.l2_psm, __le16_to_cpu(la.l2_cid),
> > > --
> > > 2.34.1
> >
> > Doesn't apply to bluetooth-next:
> >
> > https://github.com/bluez/bluez/issues/250
>
> Please disregard the link above, Ive meant to paste:
>
> Applying: Bluetooth: ensure valid channel mode when creating l2cap conn on LE
> error: patch failed: net/bluetooth/l2cap_sock.c:161
> error: net/bluetooth/l2cap_sock.c: patch does not apply
>
> I did fix something similar:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=30d57722732d9736554f85f75f9d7ad5402d192e
>
> --
> Luiz Augusto von Dentz
Marcel Holtmann Jan. 19, 2022, 5:06 p.m. UTC | #4 
Hi Gavin,

> After creating a socket(AF_INET, SOCK_STREAM, BTPROTO_L2CAP) socket and
> connect()'ing to a LE device with default settings (no setsockopt), upon
> the first sendmsg, the following BUG occurs because chan->mode==L2CAP_MODE_ERTM,
> causing l2cap_ertm_send() -> __set_retrans_timer() -> schedule_delayed_work()
> on l2cap_chan.retrans_timer, which was never initialized because
> l2cap_ertm_init() was never called to initialize it.
> 
>  Call Trace:
>   queue_delayed_work_on+0x36/0x40
>   l2cap_ertm_send.isra.0+0x14d/0x2d0 [bluetooth]
>   l2cap_tx+0x361/0x510 [bluetooth]
>   l2cap_chan_send+0xb26/0xb50 [bluetooth]
>   l2cap_sock_sendmsg+0xc9/0x100 [bluetooth]
>   sock_sendmsg+0x5e/0x60
>   sock_write_iter+0x97/0x100
>   new_sync_write+0x1d3/0x1f0
>   vfs_write+0x1b4/0x270
>   ksys_write+0xaf/0xe0
>   do_syscall_64+0x33/0x40
>   entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> This patch ensures that when connecting to a LE device, chan->mode will
> always be corrected to L2CAP_MODE_LE_FLOWCTL if it is invalid for LE.
> 
> Signed-off-by: Gavin Li <git@thegavinli.com>
> ---
> net/bluetooth/l2cap_sock.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 160c016a5dfb9..58c06ef32656c 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -78,6 +78,17 @@ static int l2cap_validate_le_psm(u16 psm)
> 	return 0;
> }
> 
> +static bool l2cap_mode_supports_le(u8 mode)
> +{
> +	switch (mode) {
> +		case L2CAP_MODE_LE_FLOWCTL:
> +		case L2CAP_MODE_EXT_FLOWCTL:
> +			return true;
> +		default:
> +			return false;
> +	}
> +}
> +

please use correct coding style.


> static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
> {
> 	struct sock *sk = sock->sk;
> @@ -161,7 +172,7 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
> 		break;
> 	}
> 
> -	if (chan->psm && bdaddr_type_is_le(chan->src_type))
> +	if (chan->psm && bdaddr_type_is_le(la.l2_bdaddr_type) && !l2cap_mode_supports_le(chan->mode))
> 		chan->mode = L2CAP_MODE_LE_FLOWCTL;
> 
> 	chan->state = BT_BOUND;
> @@ -240,7 +251,7 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr,
> 			return -EINVAL;
> 	}
> 
> -	if (chan->psm && bdaddr_type_is_le(chan->src_type) && !chan->mode)
> +	if (chan->psm && bdaddr_type_is_le(la.l2_bdaddr_type) && !l2cap_mode_supports_le(chan->mode))
> 		chan->mode = L2CAP_MODE_LE_FLOWCTL;

And you need to add a proper multi-line if clause here.

Regards

Marcel
diff mbox series

Patch

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 160c016a5dfb9..58c06ef32656c 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -78,6 +78,17 @@  static int l2cap_validate_le_psm(u16 psm)
 	return 0;
 }
 
+static bool l2cap_mode_supports_le(u8 mode)
+{
+	switch (mode) {
+		case L2CAP_MODE_LE_FLOWCTL:
+		case L2CAP_MODE_EXT_FLOWCTL:
+			return true;
+		default:
+			return false;
+	}
+}
+
 static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
 {
 	struct sock *sk = sock->sk;
@@ -161,7 +172,7 @@  static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
 		break;
 	}
 
-	if (chan->psm && bdaddr_type_is_le(chan->src_type))
+	if (chan->psm && bdaddr_type_is_le(la.l2_bdaddr_type) && !l2cap_mode_supports_le(chan->mode))
 		chan->mode = L2CAP_MODE_LE_FLOWCTL;
 
 	chan->state = BT_BOUND;
@@ -240,7 +251,7 @@  static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr,
 			return -EINVAL;
 	}
 
-	if (chan->psm && bdaddr_type_is_le(chan->src_type) && !chan->mode)
+	if (chan->psm && bdaddr_type_is_le(la.l2_bdaddr_type) && !l2cap_mode_supports_le(chan->mode))
 		chan->mode = L2CAP_MODE_LE_FLOWCTL;
 
 	err = l2cap_chan_connect(chan, la.l2_psm, __le16_to_cpu(la.l2_cid),