diff mbox series

Bluetooth: L2CAP: Fix not checking for maximum number of DCID

Message ID 20210312181948.1225833-1-luiz.dentz@gmail.com (mailing list archive)
State New, archived
Headers show
Series Bluetooth: L2CAP: Fix not checking for maximum number of DCID | expand

Commit Message

Luiz Augusto von Dentz March 12, 2021, 6:19 p.m. UTC 
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(-)

Comments

bluez.test.bot@gmail.com March 12, 2021, 7:48 p.m. UTC | #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=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
Marcel Holtmann March 13, 2021, 11:02 a.m. UTC | #2 
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
Luiz Augusto von Dentz March 15, 2021, 8:01 p.m. UTC | #3 
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
>
diff mbox series

Patch

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);