Hi Marcel, Archie,
On Mon, Jun 29, 2020 at 12:00 PM Marcel Holtmann <mar...@holtmann.org> wrote:
>
> Hi Archie,
>
> > It is possible to receive an L2CAP conn req for an encrypted
> > connection, before actually receiving the HCI change encryption
> > event. If this happened, the received L2CAP packet will be ignored.
How is this possible? Or you are referring to a race between the ACL
and Event endpoint where the Encryption Change is actually pending to
be processed but we end up processing the ACL data first.
> > This patch queues the L2CAP packet and process them after the
> > expected HCI event is received. If after 2 seconds we still don't
> > receive it, then we assume something bad happened and discard the
> > queued packets.
>
> as with the other patch, this should be behind the same quirk and
> experimental setting for exactly the same reasons.
>
> >
> > Signed-off-by: Archie Pusaka <apus...@chromium.org>
> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpan...@chromium.org>
> >
> > ---
> >
> > include/net/bluetooth/bluetooth.h | 6 +++
> > include/net/bluetooth/l2cap.h | 6 +++
> > net/bluetooth/hci_event.c | 3 ++
> > net/bluetooth/l2cap_core.c | 87 +++++++++++++++++++++++++++----
> > 4 files changed, 91 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/net/bluetooth/bluetooth.h
> > b/include/net/bluetooth/bluetooth.h
> > index 7ee8041af803..e64278401084 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -335,7 +335,11 @@ struct l2cap_ctrl {
> > u16 reqseq;
> > u16 txseq;
> > u8 retries;
> > + u8 rsp_code;
> > + u8 amp_id;
> > + __u8 ident;
> > __le16 psm;
> > + __le16 scid;
> > bdaddr_t bdaddr;
> > struct l2cap_chan *chan;
> > };
>
> I would not bother trying to make this work with CREATE_CHAN_REQ. That is if
> you want to setup a L2CAP channel that can be moved between BR/EDR and AMP
> controllers and in that case you have to read the L2CAP information and
> features first. Meaning there will have been unencrypted ACL packets. This
> problem only exists if the remote side doesn’t request any version
> information first.
>
> > @@ -374,6 +378,8 @@ struct bt_skb_cb {
> > struct hci_ctrl hci;
> > };
> > };
> > +static_assert(sizeof(struct bt_skb_cb) <= sizeof(((struct sk_buff
> > *)0)->cb));
> > +
> > #define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
> >
> > #define hci_skb_pkt_type(skb) bt_cb((skb))->pkt_type
> > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> > index 8f1e6a7a2df8..f8f6dec96f12 100644
> > --- a/include/net/bluetooth/l2cap.h
> > +++ b/include/net/bluetooth/l2cap.h
> > @@ -58,6 +58,7 @@
> > #define L2CAP_MOVE_ERTX_TIMEOUT msecs_to_jiffies(60000)
> > #define L2CAP_WAIT_ACK_POLL_PERIOD msecs_to_jiffies(200)
> > #define L2CAP_WAIT_ACK_TIMEOUT msecs_to_jiffies(10000)
> > +#define L2CAP_PEND_ENC_CONN_TIMEOUT msecs_to_jiffies(2000)
> >
> > #define L2CAP_A2MP_DEFAULT_MTU 670
> >
> > @@ -700,6 +701,9 @@ struct l2cap_conn {
> > struct mutex chan_lock;
> > struct kref ref;
> > struct list_head users;
> > +
> > + struct delayed_work remove_pending_encrypt_conn;
> > + struct sk_buff_head pending_conn_q;
> > };
> >
> > struct l2cap_user {
> > @@ -1001,4 +1005,6 @@ void l2cap_conn_put(struct l2cap_conn *conn);
> > int l2cap_register_user(struct l2cap_conn *conn, struct l2cap_user *user);
> > void l2cap_unregister_user(struct l2cap_conn *conn, struct l2cap_user
> > *user);
> >
> > +void l2cap_process_pending_encrypt_conn(struct hci_conn *hcon);
> > +
> > #endif /* __L2CAP_H */
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 108c6c102a6a..8cefc51a5ca4 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -3136,6 +3136,9 @@ static void hci_encrypt_change_evt(struct hci_dev
> > *hdev, struct sk_buff *skb)
> >
> > unlock:
> > hci_dev_unlock(hdev);
> > +
> > + if (conn && !ev->status && ev->encrypt)
> > + l2cap_process_pending_encrypt_conn(conn);
> > }
> >
> > static void hci_change_link_key_complete_evt(struct hci_dev *hdev,
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 35d2bc569a2d..fc6fe2c80c46 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -62,6 +62,10 @@ static void l2cap_send_disconn_req(struct l2cap_chan
> > *chan, int err);
> > static void l2cap_tx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
> > struct sk_buff_head *skbs, u8 event);
> >
> > +static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
> > + u8 ident, u8 *data, u8 rsp_code,
> > + u8 amp_id, bool queue_if_fail);
> > +
> > static inline u8 bdaddr_type(u8 link_type, u8 bdaddr_type)
> > {
> > if (link_type == LE_LINK) {
> > @@ -1902,6 +1906,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int
> > err)
> > if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
> > cancel_delayed_work_sync(&conn->info_timer);
> >
> > + cancel_delayed_work_sync(&conn->remove_pending_encrypt_conn);
> > +
> > hcon->l2cap_data = NULL;
> > conn->hchan = NULL;
> > l2cap_conn_put(conn);
> > @@ -2023,6 +2029,55 @@ static void l2cap_retrans_timeout(struct work_struct
> > *work)
> > l2cap_chan_put(chan);
> > }
> >
> > +static void l2cap_add_pending_encrypt_conn(struct l2cap_conn *conn,
> > + struct l2cap_conn_req *req,
> > + u8 ident, u8 rsp_code, u8 amp_id)
> > +{
> > + struct sk_buff *skb = bt_skb_alloc(0, GFP_KERNEL);
> > +
> > + bt_cb(skb)->l2cap.psm = req->psm;
> > + bt_cb(skb)->l2cap.scid = req->scid;
> > + bt_cb(skb)->l2cap.ident = ident;
> > + bt_cb(skb)->l2cap.rsp_code = rsp_code;
> > + bt_cb(skb)->l2cap.amp_id = amp_id;
> > +
> > + skb_queue_tail(&conn->pending_conn_q, skb);
> > + queue_delayed_work(conn->hcon->hdev->workqueue,
> > + &conn->remove_pending_encrypt_conn,
> > + L2CAP_PEND_ENC_CONN_TIMEOUT);
> > +}
> > +
> > +void l2cap_process_pending_encrypt_conn(struct hci_conn *hcon)
> > +{
> > + struct sk_buff *skb;
> > + struct l2cap_conn *conn = hcon->l2cap_data;
> > +
> > + if (!conn)
> > + return;
> > +
> > + while ((skb = skb_dequeue(&conn->pending_conn_q))) {
> > + struct l2cap_conn_req req;
> > + u8 ident, rsp_code, amp_id;
> > +
> > + req.psm = bt_cb(skb)->l2cap.psm;
> > + req.scid = bt_cb(skb)->l2cap.scid;
> > + ident = bt_cb(skb)->l2cap.ident;
> > + rsp_code = bt_cb(skb)->l2cap.rsp_code;
> > + amp_id = bt_cb(skb)->l2cap.amp_id;
> > +
> > + l2cap_connect(conn, ident, (u8 *)&req, rsp_code, amp_id,
> > false);
> > + kfree_skb(skb);
> > + }
> > +}
> > +
> > +static void l2cap_remove_pending_encrypt_conn(struct work_struct *work)
> > +{
> > + struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
> > + remove_pending_encrypt_conn.work);
> > +
> > + l2cap_process_pending_encrypt_conn(conn->hcon);
> > +}
> > +
> > static void l2cap_streaming_send(struct l2cap_chan *chan,
> > struct sk_buff_head *skbs)
> > {
> > @@ -4076,8 +4131,8 @@ static inline int l2cap_command_rej(struct l2cap_conn
> > *conn,
> > }
> >
> > static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
> > - struct l2cap_cmd_hdr *cmd,
> > - u8 *data, u8 rsp_code, u8 amp_id)
> > + u8 ident, u8 *data, u8 rsp_code,
> > + u8 amp_id, bool queue_if_fail)
> > {
> > struct l2cap_conn_req *req = (struct l2cap_conn_req *) data;
> > struct l2cap_conn_rsp rsp;
> > @@ -4103,8 +4158,15 @@ static struct l2cap_chan *l2cap_connect(struct
> > l2cap_conn *conn,
> > /* Check if the ACL is secure enough (if not SDP) */
> > if (psm != cpu_to_le16(L2CAP_PSM_SDP) &&
> > !hci_conn_check_link_mode(conn->hcon)) {
> > - conn->disc_reason = HCI_ERROR_AUTH_FAILURE;
> > - result = L2CAP_CR_SEC_BLOCK;
> > + if (!queue_if_fail) {
> > + conn->disc_reason = HCI_ERROR_AUTH_FAILURE;
> > + result = L2CAP_CR_SEC_BLOCK;
> > + goto response;
> > + }
> > +
> > + l2cap_add_pending_encrypt_conn(conn, req, ident, rsp_code,
> > + amp_id);
> > + result = L2CAP_CR_PEND;
> > goto response;
> > }
>
> So I am actually wondering if the approach is not better to send back a
> pending to the connect request like we do for everything else. And then
> proceed with getting our remote L2CAP information. If these come back in
> encrypted, then we can assume that we actually had encryption enabled and
> proceed with a L2CAP connect response saying that all is fine.
I wonder if we should resolve this by having different queues in
hci_recv_frame (e.g. hdev->evt_rx), that way we can dequeue the HCI
events before ACL so we first update the HCI states before start
processing the L2CAP data, thoughts? Something like this:
https://gist.github.com/Vudentz/464fb0065a73e5c99bdb66cd2c5a1a2d
> That also means no queuing of packets is required.
>
> Regards
>
> Marcel
>
--
Luiz Augusto von Dentz
