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 <[email protected]>
---
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
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=680700
---Test result---
Test Summary:
CheckPatch PASS 1.64 seconds
GitLint PASS 0.73 seconds
SubjectPrefix PASS 0.61 seconds
BuildKernel PASS 35.46 seconds
BuildKernel32 PASS 31.10 seconds
Incremental Build with patchesPASS 44.66 seconds
TestRunner: Setup PASS 521.84 seconds
TestRunner: l2cap-tester PASS 17.49 seconds
TestRunner: iso-tester PASS 16.58 seconds
TestRunner: bnep-tester PASS 6.52 seconds
TestRunner: mgmt-tester PASS 106.05 seconds
TestRunner: rfcomm-tester PASS 10.22 seconds
TestRunner: sco-tester PASS 9.72 seconds
TestRunner: ioctl-tester PASS 11.05 seconds
TestRunner: smp-tester PASS 9.76 seconds
TestRunner: userchan-tester PASS 6.77 seconds
---
Regards,
Linux Bluetooth
Hi Kim,
On Mon, Sep 26, 2022 at 1:47 PM Sungwoo Kim <[email protected]> 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 <[email protected]>
> ---
> 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
>
--
Luiz Augusto von Dentz
Signed-off-by: Sungwoo Kim <[email protected]>
---
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
Hi Kim,
On Mon, Sep 26, 2022 at 3:06 PM Sungwoo Kim <[email protected]> wrote:
>
> Signed-off-by: Sungwoo Kim <[email protected]>
> ---
> 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
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
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 <[email protected]>
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