Re: [PATCH v3] Bluetooth: hci_event: move wake reason storage into validated event handlers

[Luiz Augusto von Dentz]

Hi Oleh,

On Thu, Mar 26, 2026 at 12:22 PM Oleh Konko <security@xxxxxxxxx> wrote:
>
> v5 now applies cleanly and gets through build/sparse, but the
> current CI run still shows runtime regressions (mgmt/iso/sco/mesh).

Those are probably the already existing errors, so no need to spin a v6.

> looking at the delta, the most suspicious part is not the move of
> wake-address storage into validated handlers itself, but the new global
> fallback after hci_event_func(). that changes both timing and semantics:
> it adds a post-handler hci_dev_lock() path, and it also changes the old
> HCI_EV_LE_META fallback behavior into MGMT_WAKE_REASON_UNEXPECTED for
> cases that previously stayed on the remote-wake path.
>
> my plan for v6 is therefore to narrow the change:
> - keep wake address storage only in validated event handlers
> - drop the global post-handler fallback in hci_event_packet()
> - restore the non-address fallback paths without reintroducing the
>   pre-validation dereference
> - handle the LE meta fallback separately so its old semantics are
>   preserved without touching unvalidated payload
>
> unless you object, i'll respin in that direction rather than send
> another blind reroll.
>
> thanks,
> Oleh
> ________________________________
> От: Oleh Konko <security@xxxxxxxxx>
> Отправлено: 26 марта 2026 г. 16:06
> Кому: gregkh@xxxxxxxxxxxxxxxxxxx <gregkh@xxxxxxxxxxxxxxxxxxx>
> Копия: linux-bluetooth@xxxxxxxxxxxxxxx <linux-bluetooth@xxxxxxxxxxxxxxx>; marcel@xxxxxxxxxxxx <marcel@xxxxxxxxxxxx>; luiz.dentz@xxxxxxxxx <luiz.dentz@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>; stable@xxxxxxxxxxxxxxx <stable@xxxxxxxxxxxxxxx>
> Тема: RE: [PATCH v3] Bluetooth: hci_event: move wake reason storage into validated event handlers
>
> hi Greg,
>
> thanks. the embedded headers were a formatting issue on my side.
>
> i have now resent the patch as v5 with proper inline formatting. this
> revision also adds the lock contract you asked for:
> __must_hold(&hdev->lock) on hci_store_wake_reason(), plus
> lockdep_assert_held(&hdev->lock) inside the helper.
>
> thanks,
> Oleh
> ________________________________
> От: gregkh@xxxxxxxxxxxxxxxxxxx <gregkh@xxxxxxxxxxxxxxxxxxx>
> Отправлено: 26 марта 2026 г. 15:57
> Кому: Oleh Konko <security@xxxxxxxxx>
> Копия: linux-bluetooth@xxxxxxxxxxxxxxx <linux-bluetooth@xxxxxxxxxxxxxxx>; marcel@xxxxxxxxxxxx <marcel@xxxxxxxxxxxx>; luiz.dentz@xxxxxxxxx <luiz.dentz@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>; stable@xxxxxxxxxxxxxxx <stable@xxxxxxxxxxxxxxx>
> Тема: Re: [PATCH v3] Bluetooth: hci_event: move wake reason storage into validated event handlers
>
> On Thu, Mar 26, 2026 at 01:32:50PM +0000, Oleh Konko wrote:
> > From f0e8b2abaf4a895fad81756277014582c773808d Mon Sep 17 00:00:00 2001
> > From: Oleh Konko <security@xxxxxxxxx>
> > Date: Thu, 26 Mar 2026 14:29:58 +0100
> > Subject: [PATCH v3] Bluetooth: hci_event: move wake reason storage into
> >  validated event handlers
>
> If you use git send-email, the header will not be in the body like this.
>
> > hci_store_wake_reason() is called from hci_event_packet() immediately
> > after stripping the HCI event header but before hci_event_func()
> > enforces the per-event minimum payload length from hci_ev_table.
> > This means a short HCI event frame can reach bacpy() before any bounds
> > check runs.
> >
> > Rather than duplicating skb parsing and per-event length checks inside
> > hci_store_wake_reason(), move wake-address storage into the individual
> > event handlers after their existing event-length validation has
> > succeeded. Convert hci_store_wake_reason() into a small helper that only
> > stores an already-validated bdaddr while the caller holds hci_dev_lock().
> > Use the same helper after hci_event_func() with a NULL address to
> > preserve the existing unexpected-wake fallback semantics when no
> > validated event handler records a wake address.
> >
> > Call the helper from hci_conn_request_evt(), hci_conn_complete_evt(),
> > hci_le_adv_report_evt(), hci_le_ext_adv_report_evt(), and
> > hci_le_direct_adv_report_evt().
> >
> > Fixes: 2f20216c1d6f ("Bluetooth: Emit controller suspend and resume events")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Oleh Konko <security@xxxxxxxxx>
> > ---
> > v3:
> > - route the unexpected-wake fallback through hci_store_wake_reason(NULL, 0)
> >   after hci_event_func(), as suggested in review
> >
> >  net/bluetooth/hci_event.c | 89 +++++++++++++--------------------------
> >  1 file changed, 29 insertions(+), 60 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 286529d2e..c0e0b4a1c 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -80,6 +80,9 @@ static void *hci_le_ev_skb_pull(struct hci_dev *hdev, struct sk_buff *skb,
> >        return data;
> >  }
> >
> > +static void hci_store_wake_reason(struct hci_dev *hdev,
> > +                               const bdaddr_t *bdaddr, u8 addr_type);
> > +
> >  static u8 hci_cc_inquiry_cancel(struct hci_dev *hdev, void *data,
> >                                struct sk_buff *skb)
> >  {
> > @@ -3111,6 +3114,7 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
> >        bt_dev_dbg(hdev, "status 0x%2.2x", status);
> >
> >        hci_dev_lock(hdev);
> > +     hci_store_wake_reason(hdev, &ev->bdaddr, BDADDR_BREDR);
> >
> >        /* Check for existing connection:
> >         *
> > @@ -3274,6 +3278,10 @@ static void hci_conn_request_evt(struct hci_dev *hdev, void *data,
> >
> >        bt_dev_dbg(hdev, "bdaddr %pMR type 0x%x", &ev->bdaddr, ev->link_type);
> >
> > +     hci_dev_lock(hdev);
> > +     hci_store_wake_reason(hdev, &ev->bdaddr, BDADDR_BREDR);
> > +     hci_dev_unlock(hdev);
> > +
> >        /* Reject incoming connection from device with same BD ADDR against
> >         * CVE-2020-26555
> >         */
> > @@ -6403,6 +6411,8 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, void *data,
> >                                        info->length + 1))
> >                        break;
> >
> > +             hci_store_wake_reason(hdev, &info->bdaddr, info->bdaddr_type);
> > +
> >                if (info->length <= max_adv_len(hdev)) {
> >                        rssi = info->data[info->length];
> >                        process_adv_report(hdev, info->type, &info->bdaddr,
> > @@ -6491,6 +6501,8 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, void *data,
> >                                        info->length))
> >                        break;
> >
> > +             hci_store_wake_reason(hdev, &info->bdaddr, info->bdaddr_type);
> > +
> >                evt_type = __le16_to_cpu(info->type) & LE_EXT_ADV_EVT_TYPE_MASK;
> >                legacy_evt_type = ext_evt_type_to_legacy(hdev, evt_type);
> >
> > @@ -6834,6 +6846,8 @@ static void hci_le_direct_adv_report_evt(struct hci_dev *hdev, void *data,
> >        for (i = 0; i < ev->num; i++) {
> >                struct hci_ev_le_direct_adv_info *info = &ev->info[i];
> >
> > +             hci_store_wake_reason(hdev, &info->bdaddr, info->bdaddr_type);
> > +
> >                process_adv_report(hdev, info->type, &info->bdaddr,
> >                                   info->bdaddr_type, &info->direct_addr,
> >                                   info->direct_addr_type, HCI_ADV_PHY_1M, 0,
> > @@ -7517,73 +7531,27 @@ static bool hci_get_cmd_complete(struct hci_dev *hdev, u16 opcode,
> >        return true;
> >  }
> >
> > -static void hci_store_wake_reason(struct hci_dev *hdev, u8 event,
> > -                               struct sk_buff *skb)
> > +/* hdev lock must be held. pass NULL bdaddr to record an unexpected wake. */
> > +static void hci_store_wake_reason(struct hci_dev *hdev,
> > +                               const bdaddr_t *bdaddr, u8 addr_type)
>
> If the lock must be held, why aren't you both adding the static test for
> this at build time __must_hold(), and the runtime lock test so that we
> know this lock is held?  That way any changes in any code paths in the
> future will properly trigger at build and runtime to know to be fixed
> up.
>
> thanks,
>
> greg k-h



-- 
Luiz Augusto von Dentz