[PATCH] Bluetooth: L2CAP: fix an illegal state transition from BT_DISCONN

2 views
Skip to first unread message

Sungwoo Kim

unread,
Sep 26, 2022, 4:48:00 PMSep 26
to syzk...@googlegroups.com, Sungwoo Kim, Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-b...@vger.kernel.org, net...@vger.kernel.org, linux-...@vger.kernel.org
Prevent an illegal state transition from BT_DISCONN to BT_CONFIG.
L2CAP_CONN_RSP and L2CAP_CREATE_CHAN_RSP events should be ignored
for BT_DISCONN state according to the Bluetooth Core v5.3 p.1096.
It is found by BTFuzz, a modified version of syzkaller.

Signed-off-by: Sungwoo Kim <i...@sung-woo.kim>
---
net/bluetooth/l2cap_core.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 2c9de67da..a15d64b13 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4307,6 +4307,9 @@ static int l2cap_connect_create_rsp(struct l2cap_conn *conn,
}
}

+ if (chan->state == BT_DISCONN)
+ goto unlock;
+
err = 0;

l2cap_chan_lock(chan);
--
2.25.1

Sungwoo Kim

unread,
Sep 26, 2022, 6:06:39 PMSep 26
to luiz....@gmail.com, da...@davemloft.net, edum...@google.com, i...@sung-woo.kim, johan....@gmail.com, ku...@kernel.org, linux-b...@vger.kernel.org, linux-...@vger.kernel.org, mar...@holtmann.org, net...@vger.kernel.org, pab...@redhat.com, syzk...@googlegroups.com
Signed-off-by: Sungwoo Kim <i...@sung-woo.kim>
---
net/bluetooth/l2cap_core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 2c9de67da..029de9f35 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4294,13 +4294,13 @@ static int l2cap_connect_create_rsp(struct l2cap_conn *conn,
mutex_lock(&conn->chan_lock);

if (scid) {
- chan = __l2cap_get_chan_by_scid(conn, scid);
+ chan = l2cap_get_chan_by_scid(conn, scid);
if (!chan) {
err = -EBADSLT;
goto unlock;
}
} else {
- chan = __l2cap_get_chan_by_ident(conn, cmd->ident);
+ chan = l2cap_get_chan_by_ident(conn, cmd->ident);
if (!chan) {
err = -EBADSLT;
goto unlock;
@@ -4336,6 +4336,7 @@ static int l2cap_connect_create_rsp(struct l2cap_conn *conn,
}

l2cap_chan_unlock(chan);
+ l2cap_chan_put(chan);

unlock:
mutex_unlock(&conn->chan_lock);
--
2.25.1

Luiz Augusto von Dentz

unread,
Sep 27, 2022, 1:37:07 AMSep 27
to Sungwoo Kim, da...@davemloft.net, edum...@google.com, johan....@gmail.com, ku...@kernel.org, linux-b...@vger.kernel.org, linux-...@vger.kernel.org, mar...@holtmann.org, net...@vger.kernel.org, pab...@redhat.com, syzk...@googlegroups.com
Hi Kim,
Not quite right, we cannot lock conn->chan_lock since the likes of
l2cap_get_chan_by_scid will also attempt to lock it:

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 770891f68703..4726d8979276 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4293,26 +4293,18 @@ static int l2cap_connect_create_rsp(struct
l2cap_conn *conn,
BT_DBG("dcid 0x%4.4x scid 0x%4.4x result 0x%2.2x status 0x%2.2x",
dcid, scid, result, status);

- mutex_lock(&conn->chan_lock);
-
if (scid) {
- chan = __l2cap_get_chan_by_scid(conn, scid);
- if (!chan) {
- err = -EBADSLT;
- goto unlock;
- }
+ chan = l2cap_get_chan_by_scid(conn, scid);
+ if (!chan)
+ return -EBADSLT;
} else {
- chan = __l2cap_get_chan_by_ident(conn, cmd->ident);
- if (!chan) {
- err = -EBADSLT;
- goto unlock;
- }
+ chan = l2cap_get_chan_by_ident(conn, cmd->ident);
+ if (!chan)
+ return -EBADSLT;
}

err = 0;

- l2cap_chan_lock(chan);
-
switch (result) {
case L2CAP_CR_SUCCESS:
l2cap_state_change(chan, BT_CONFIG);
@@ -4338,9 +4330,7 @@ static int l2cap_connect_create_rsp(struct
l2cap_conn *conn,
}

l2cap_chan_unlock(chan);
-
-unlock:
- mutex_unlock(&conn->chan_lock);
+ l2cap_chan_put(chan);

return err;
}


--
Luiz Augusto von Dentz

Luiz Augusto von Dentz

unread,
Sep 27, 2022, 1:37:07 AMSep 27
to Sungwoo Kim, syzk...@googlegroups.com, Marcel Holtmann, Johan Hedberg, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-b...@vger.kernel.org, net...@vger.kernel.org, linux-...@vger.kernel.org
Hi Kim,

On Mon, Sep 26, 2022 at 1:47 PM Sungwoo Kim <i...@sung-woo.kim> wrote:
>
> Prevent an illegal state transition from BT_DISCONN to BT_CONFIG.
> L2CAP_CONN_RSP and L2CAP_CREATE_CHAN_RSP events should be ignored
> for BT_DISCONN state according to the Bluetooth Core v5.3 p.1096.
> It is found by BTFuzz, a modified version of syzkaller.
>
> Signed-off-by: Sungwoo Kim <i...@sung-woo.kim>
> ---
> net/bluetooth/l2cap_core.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 2c9de67da..a15d64b13 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -4307,6 +4307,9 @@ static int l2cap_connect_create_rsp(struct l2cap_conn *conn,
> }
> }

Perhaps it would be better to switch to use l2cap_get_chan_by_scid and
l2cap_get_chan_by_ident, since I suspect this is caused by the socket
being terminated while the response is in course so the chan reference
is already 0 thus why l2cap_chan_hold_unless_zero is probably
preferable instead of checking the state directly.

> + if (chan->state == BT_DISCONN)
> + goto unlock;
> +
> err = 0;
>
> l2cap_chan_lock(chan);
> --
> 2.25.1
>


kernel test robot

unread,
Sep 27, 2022, 7:26:44 AMSep 27
to Sungwoo Kim, ll...@lists.linux.dev, kbuil...@lists.01.org, syzk...@googlegroups.com, Sungwoo Kim, Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz, Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-b...@vger.kernel.org, net...@vger.kernel.org, linux-...@vger.kernel.org
Hi Sungwoo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth/master]
[also build test WARNING on bluetooth-next/master linus/master v6.0-rc7 next-20220923]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Sungwoo-Kim/Bluetooth-L2CAP-fix-an-illegal-state-transition-from-BT_DISCONN/20220927-100055
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master
config: i386-randconfig-a011-20220926
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/c033280cb0996b25511763ada18ac95fa544219f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Sungwoo-Kim/Bluetooth-L2CAP-fix-an-illegal-state-transition-from-BT_DISCONN/20220927-100055
git checkout c033280cb0996b25511763ada18ac95fa544219f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/bluetooth/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <l...@intel.com>

All warnings (new ones prefixed by >>):

>> net/bluetooth/l2cap_core.c:4310:6: warning: variable 'err' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (chan->state == BT_DISCONN)
^~~~~~~~~~~~~~~~~~~~~~~~~
net/bluetooth/l2cap_core.c:4346:9: note: uninitialized use occurs here
return err;
^~~
net/bluetooth/l2cap_core.c:4310:2: note: remove the 'if' if its condition is always false
if (chan->state == BT_DISCONN)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/bluetooth/l2cap_core.c:4281:9: note: initialize the variable 'err' to silence this warning
int err;
^
= 0
1 warning generated.


vim +4310 net/bluetooth/l2cap_core.c

4272
4273 static int l2cap_connect_create_rsp(struct l2cap_conn *conn,
4274 struct l2cap_cmd_hdr *cmd, u16 cmd_len,
4275 u8 *data)
4276 {
4277 struct l2cap_conn_rsp *rsp = (struct l2cap_conn_rsp *) data;
4278 u16 scid, dcid, result, status;
4279 struct l2cap_chan *chan;
4280 u8 req[128];
4281 int err;
4282
4283 if (cmd_len < sizeof(*rsp))
4284 return -EPROTO;
4285
4286 scid = __le16_to_cpu(rsp->scid);
4287 dcid = __le16_to_cpu(rsp->dcid);
4288 result = __le16_to_cpu(rsp->result);
4289 status = __le16_to_cpu(rsp->status);
4290
4291 BT_DBG("dcid 0x%4.4x scid 0x%4.4x result 0x%2.2x status 0x%2.2x",
4292 dcid, scid, result, status);
4293
4294 mutex_lock(&conn->chan_lock);
4295
4296 if (scid) {
4297 chan = __l2cap_get_chan_by_scid(conn, scid);
4298 if (!chan) {
4299 err = -EBADSLT;
4300 goto unlock;
4301 }
4302 } else {
4303 chan = __l2cap_get_chan_by_ident(conn, cmd->ident);
4304 if (!chan) {
4305 err = -EBADSLT;
4306 goto unlock;
4307 }
4308 }
4309
> 4310 if (chan->state == BT_DISCONN)
4311 goto unlock;
4312
4313 err = 0;
4314
4315 l2cap_chan_lock(chan);
4316
4317 switch (result) {
4318 case L2CAP_CR_SUCCESS:
4319 l2cap_state_change(chan, BT_CONFIG);
4320 chan->ident = 0;
4321 chan->dcid = dcid;
4322 clear_bit(CONF_CONNECT_PEND, &chan->conf_state);
4323
4324 if (test_and_set_bit(CONF_REQ_SENT, &chan->conf_state))
4325 break;
4326
4327 l2cap_send_cmd(conn, l2cap_get_ident(conn), L2CAP_CONF_REQ,
4328 l2cap_build_conf_req(chan, req, sizeof(req)), req);
4329 chan->num_conf_req++;
4330 break;
4331
4332 case L2CAP_CR_PEND:
4333 set_bit(CONF_CONNECT_PEND, &chan->conf_state);
4334 break;
4335
4336 default:
4337 l2cap_chan_del(chan, ECONNREFUSED);
4338 break;
4339 }
4340
4341 l2cap_chan_unlock(chan);
4342
4343 unlock:
4344 mutex_unlock(&conn->chan_lock);
4345
4346 return err;
4347 }
4348

--
0-DAY CI Kernel Test Service
https://01.org/lkp
config
Reply all
Reply to author
Forward
0 new messages