* [PATCH] Bluetooth: L2CAP: Fix not checking for maximum number of DCID
@ 2021-03-12 18:19 Luiz Augusto von Dentz
2021-03-12 19:48 ` bluez.test.bot
2021-03-13 11:02 ` [PATCH] " Marcel Holtmann
0 siblings, 2 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2021-03-12 18:19 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
When receiving L2CAP_CREDIT_BASED_CONNECTION_REQ the remote may request
more channels than allowed by the spec (10 octecs = 5 CIDs) so this
truncates the response allowing it to create only the maximum allowed.
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
include/net/bluetooth/l2cap.h | 1 +
net/bluetooth/l2cap_core.c | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 61800a7b6192..3c4f550e5a8b 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -494,6 +494,7 @@ struct l2cap_le_credits {
#define L2CAP_ECRED_MIN_MTU 64
#define L2CAP_ECRED_MIN_MPS 64
+#define L2CAP_ECRED_MAX_CID 5
struct l2cap_ecred_conn_req {
__le16 psm;
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 72c2f5226d67..6325d4a89b31 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5921,7 +5921,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
struct l2cap_ecred_conn_req *req = (void *) data;
struct {
struct l2cap_ecred_conn_rsp rsp;
- __le16 dcid[5];
+ __le16 dcid[L2CAP_ECRED_MAX_CID];
} __packed pdu;
struct l2cap_chan *chan, *pchan;
u16 mtu, mps;
@@ -5973,7 +5973,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
cmd_len -= sizeof(*req);
num_scid = cmd_len / sizeof(u16);
- for (i = 0; i < num_scid; i++) {
+ for (i = 0; i < num_scid && i < ARRAY_SIZE(pdu.dcid); i++) {
u16 scid = __le16_to_cpu(req->scid[i]);
BT_DBG("scid[%d] 0x%4.4x", i, scid);
--
2.29.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: Bluetooth: L2CAP: Fix not checking for maximum number of DCID
2021-03-12 18:19 [PATCH] Bluetooth: L2CAP: Fix not checking for maximum number of DCID Luiz Augusto von Dentz
@ 2021-03-12 19:48 ` bluez.test.bot
2021-03-13 11:02 ` [PATCH] " Marcel Holtmann
1 sibling, 0 replies; 4+ messages in thread
From: bluez.test.bot @ 2021-03-12 19:48 UTC (permalink / raw)
To: linux-bluetooth, luiz.dentz
[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]
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=447251
---Test result---
##############################
Test: CheckPatch - PASS
##############################
Test: CheckGitLint - PASS
##############################
Test: CheckBuildK - PASS
##############################
Test: CheckTestRunner: Setup - PASS
##############################
Test: CheckTestRunner: l2cap-tester - PASS
Total: 40, Passed: 34 (85.0%), Failed: 0, Not Run: 6
##############################
Test: CheckTestRunner: bnep-tester - PASS
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
##############################
Test: CheckTestRunner: mgmt-tester - FAIL
Total: 416, Passed: 399 (95.9%), Failed: 3, Not Run: 14
##############################
Test: CheckTestRunner: rfcomm-tester - PASS
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0
##############################
Test: CheckTestRunner: sco-tester - PASS
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
##############################
Test: CheckTestRunner: smp-tester - PASS
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
##############################
Test: CheckTestRunner: userchan-tester - PASS
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0
---
Regards,
Linux Bluetooth
[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 43342 bytes --]
[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3532 bytes --]
[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 547124 bytes --]
[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11653 bytes --]
[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9888 bytes --]
[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11799 bytes --]
[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5429 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Bluetooth: L2CAP: Fix not checking for maximum number of DCID
2021-03-12 18:19 [PATCH] Bluetooth: L2CAP: Fix not checking for maximum number of DCID Luiz Augusto von Dentz
2021-03-12 19:48 ` bluez.test.bot
@ 2021-03-13 11:02 ` Marcel Holtmann
2021-03-15 20:01 ` Luiz Augusto von Dentz
1 sibling, 1 reply; 4+ messages in thread
From: Marcel Holtmann @ 2021-03-13 11:02 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
> When receiving L2CAP_CREDIT_BASED_CONNECTION_REQ the remote may request
> more channels than allowed by the spec (10 octecs = 5 CIDs) so this
> truncates the response allowing it to create only the maximum allowed.
so what does the spec say the behavior should be? Truncate or actually respond with an error?
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> include/net/bluetooth/l2cap.h | 1 +
> net/bluetooth/l2cap_core.c | 4 ++--
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 61800a7b6192..3c4f550e5a8b 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -494,6 +494,7 @@ struct l2cap_le_credits {
>
> #define L2CAP_ECRED_MIN_MTU 64
> #define L2CAP_ECRED_MIN_MPS 64
> +#define L2CAP_ECRED_MAX_CID 5
>
> struct l2cap_ecred_conn_req {
> __le16 psm;
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 72c2f5226d67..6325d4a89b31 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -5921,7 +5921,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
> struct l2cap_ecred_conn_req *req = (void *) data;
> struct {
> struct l2cap_ecred_conn_rsp rsp;
> - __le16 dcid[5];
> + __le16 dcid[L2CAP_ECRED_MAX_CID];
> } __packed pdu;
> struct l2cap_chan *chan, *pchan;
> u16 mtu, mps;
> @@ -5973,7 +5973,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
> cmd_len -= sizeof(*req);
> num_scid = cmd_len / sizeof(u16);
>
> - for (i = 0; i < num_scid; i++) {
> + for (i = 0; i < num_scid && i < ARRAY_SIZE(pdu.dcid); i++) {
> u16 scid = __le16_to_cpu(req->scid[i]);
>
> BT_DBG("scid[%d] 0x%4.4x", i, scid);
Is this really a good idea? I would prefer if we check the size first and then just respond with an error.
Regards
Marcel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Bluetooth: L2CAP: Fix not checking for maximum number of DCID
2021-03-13 11:02 ` [PATCH] " Marcel Holtmann
@ 2021-03-15 20:01 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2021-03-15 20:01 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Sat, Mar 13, 2021 at 3:02 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > When receiving L2CAP_CREDIT_BASED_CONNECTION_REQ the remote may request
> > more channels than allowed by the spec (10 octecs = 5 CIDs) so this
> > truncates the response allowing it to create only the maximum allowed.
>
> so what does the spec say the behavior should be? Truncate or actually respond with an error?
The spec is not very clear about this, well except by saying:
'The Source CID is an array of up to 5 two-octet values and represents the
channel endpoints on the device sending the request.'
So I guess responding with an error would still conform to the above
statement so we would just strictly follow the maximum number of
channels allowed.
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > include/net/bluetooth/l2cap.h | 1 +
> > net/bluetooth/l2cap_core.c | 4 ++--
> > 2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> > index 61800a7b6192..3c4f550e5a8b 100644
> > --- a/include/net/bluetooth/l2cap.h
> > +++ b/include/net/bluetooth/l2cap.h
> > @@ -494,6 +494,7 @@ struct l2cap_le_credits {
> >
> > #define L2CAP_ECRED_MIN_MTU 64
> > #define L2CAP_ECRED_MIN_MPS 64
> > +#define L2CAP_ECRED_MAX_CID 5
> >
> > struct l2cap_ecred_conn_req {
> > __le16 psm;
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 72c2f5226d67..6325d4a89b31 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -5921,7 +5921,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
> > struct l2cap_ecred_conn_req *req = (void *) data;
> > struct {
> > struct l2cap_ecred_conn_rsp rsp;
> > - __le16 dcid[5];
> > + __le16 dcid[L2CAP_ECRED_MAX_CID];
> > } __packed pdu;
> > struct l2cap_chan *chan, *pchan;
> > u16 mtu, mps;
> > @@ -5973,7 +5973,7 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
> > cmd_len -= sizeof(*req);
> > num_scid = cmd_len / sizeof(u16);
> >
> > - for (i = 0; i < num_scid; i++) {
> > + for (i = 0; i < num_scid && i < ARRAY_SIZE(pdu.dcid); i++) {
> > u16 scid = __le16_to_cpu(req->scid[i]);
> >
> > BT_DBG("scid[%d] 0x%4.4x", i, scid);
>
> Is this really a good idea? I would prefer if we check the size first and then just respond with an error.
Right, I will change it to just fail with L2CAP_CR_LE_INVALID_PARAMS instead.
> Regards
>
> Marcel
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-15 20:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-12 18:19 [PATCH] Bluetooth: L2CAP: Fix not checking for maximum number of DCID Luiz Augusto von Dentz
2021-03-12 19:48 ` bluez.test.bot
2021-03-13 11:02 ` [PATCH] " Marcel Holtmann
2021-03-15 20:01 ` Luiz Augusto von Dentz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.