| Message ID | 20220926204657.3147968-1-iam@sung-woo.kim (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bluetooth: L2CAP: fix an illegal state transition from BT_DISCONN | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/checkpatch | success | Checkpatch PASS |
| tedd_an/gitlint | success | Gitlint PASS |
| tedd_an/subjectprefix | success | PASS |
| tedd_an/buildkernel | success | Build Kernel PASS |
| tedd_an/buildkernel32 | success | Build Kernel32 PASS |
| tedd_an/incremental_build | success | Pass |
| tedd_an/testrunnersetup | success | Test Runner Setup PASS |
| tedd_an/testrunnerl2cap-tester | success | Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunneriso-tester | success | Total: 55, Passed: 55 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnerbnep-tester | success | Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnermgmt-tester | success | Total: 494, Passed: 494 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnerrfcomm-tester | success | Total: 11, Passed: 11 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnersco-tester | success | Total: 12, Passed: 12 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnerioctl-tester | success | Total: 28, Passed: 28 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnersmp-tester | success | Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunneruserchan-tester | success | Total: 4, Passed: 4 (100.0%), Failed: 0, Not Run: 0 |
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 <iam@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 <iam@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 >
Signed-off-by: Sungwoo Kim <iam@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);
Hi Kim, On Mon, Sep 26, 2022 at 3:06 PM Sungwoo Kim <iam@sung-woo.kim> wrote: > > Signed-off-by: Sungwoo Kim <iam@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 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; }
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 <lkp@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
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);
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 <iam@sung-woo.kim> --- net/bluetooth/l2cap_core.c | 3 +++ 1 file changed, 3 insertions(+)