Re: Re: [PATCH v2] Bluetooth: HIDP: add missing length check for incoming frames

Hi,

On Sun, Jul 19, 2026 at 1:38 AM jiale yao <[email protected]> wrote:
>
>
>
>
>
>
>
> In hidp_recv_ctrl_frame() and hidp_recv_intr_frame(), skb->data[0] is
> read without verifying that skb->len >= 1.  A zero-length L2CAP PDU
> delivered via the HIDP control or interrupt channel causes an
> out-of-bounds read.
>
> Use skb_pull_data(skb, 1) which combines the length check and pull
> in one operation, matching the existing pattern in hidp_input_report()
> and the fix in commit 6770d3a8acdf ("Bluetooth: bnep: reject short
> frames before parsing") which addressed the same class of bug in BNEP.
>
> Assisted-by: Claude:deepseek-v4-pro
> Signed-off-by: Jiale Yao <[email protected]>
> ---
>  net/bluetooth/hidp/core.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 0e24c5e2955e..bb61602b02e1 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -560,16 +560,18 @@ static int hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
>  static void hidp_recv_ctrl_frame(struct hidp_session *session,
>                                         struct sk_buff *skb)
>  {
> -       unsigned char hdr, type, param;
> +       unsigned char *hdr;
> +       unsigned char type, param;
>         int free_skb = 1;
>
>         BT_DBG("session %p skb %p len %u", session, skb, skb->len);
>
> -       hdr = skb->data[0];
> -       skb_pull(skb, 1);
> +       hdr = skb_pull_data(skb, 1);
> +       if (!hdr)
> +               return;

https://sashiko.dev/#/patchset/20260717153238.2002330-1-yaojiale02%40163.com

It seems that by early returning we are leaking the skb.

>
> -       type = hdr & HIDP_HEADER_TRANS_MASK;
> -       param = hdr & HIDP_HEADER_PARAM_MASK;
> +       type = *hdr & HIDP_HEADER_TRANS_MASK;
> +       param = *hdr & HIDP_HEADER_PARAM_MASK;
>
>         switch (type) {
>         case HIDP_TRANS_HANDSHAKE:
> @@ -597,14 +599,15 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
>  static void hidp_recv_intr_frame(struct hidp_session *session,
>                                 struct sk_buff *skb)
>  {
> -       unsigned char hdr;
> +       unsigned char *hdr;
>
>         BT_DBG("session %p skb %p len %u", session, skb, skb->len);
>
> -       hdr = skb->data[0];
> -       skb_pull(skb, 1);
> +       hdr = skb_pull_data(skb, 1);
> +       if (!hdr)
> +               return;
>
> -       if (hdr == (HIDP_TRANS_DATA | HIDP_DATA_RTYPE_INPUT)) {
> +       if (*hdr == (HIDP_TRANS_DATA | HIDP_DATA_RTYPE_INPUT)) {
>                 hidp_set_timer(session);
>
>                 if (session->input)
> --
> 2.34.1
>
>
>
>
>
>
>
>
>
>
>
>
> At 2026-07-17 23:40:11, "Luiz Augusto von Dentz" <[email protected]> wrote:
> >Hi,
> >
> >On Fri, Jul 17, 2026 at 11:33 AM Jiale Yao <[email protected]> wrote:
> >>
> >> In hidp_recv_ctrl_frame() and hidp_recv_intr_frame(), skb->data[0] is
> >> read without verifying that skb->len >= 1.  A zero-length L2CAP PDU
> >> delivered via the HIDP control or interrupt channel causes an
> >> out-of-bounds read.
> >>
> >> Add pskb_may_pull(skb, 1) guards before both reads, matching the fix
> >> in commit 6770d3a8acdf ("Bluetooth: bnep: reject short frames before
> >> parsing") which addressed the same class of bug in BNEP.
> >>
> >> Assisted-by: Claude:deepseek-v4-pro
> >> Signed-off-by: Jiale Yao <[email protected]>
> >> ---
> >>  net/bluetooth/hidp/core.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> >> index 0e24c5e2955e..6c7da8aca732 100644
> >> --- a/net/bluetooth/hidp/core.c
> >> +++ b/net/bluetooth/hidp/core.c
> >> @@ -565,6 +565,8 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
> >>
> >>         BT_DBG("session %p skb %p len %u", session, skb, skb->len);
> >>
> >> +       if (!pskb_may_pull(skb, 1))
> >> +               return;
> >
> >We can probably replace this with skb_pull_data.
> >
> >>         hdr = skb->data[0];
> >>         skb_pull(skb, 1);
> >>
> >> @@ -601,6 +603,8 @@ static void hidp_recv_intr_frame(struct hidp_session *session,
> >>
> >>         BT_DBG("session %p skb %p len %u", session, skb, skb->len);
> >>
> >> +       if (!pskb_may_pull(skb, 1))
> >> +               return;
> >
> >Ditto.
> >
> >>         hdr = skb->data[0];
> >>         skb_pull(skb, 1);
> >>
> >> --
> >> 2.34.1
> >>
> >
> >
> >--
> >Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz