Re: [PATCH v4] Bluetooth: bnep: reject short frames before parsing

[Luiz Augusto von Dentz]

Hi Zhang,

On Thu, May 21, 2026 at 1:26 AM Zhang Cen <rollkingzzc@xxxxxxxxx> wrote:
>
> A BNEP peer can send a short BNEP SDU. bnep_rx_frame() reads the
> packet type byte immediately and, for control packets, reads the control
> opcode and setup UUID-size byte before proving that those bytes are
> present. bnep_rx_control() also dereferences the control opcode without
> rejecting an empty control payload.
>
> Use skb_pull_data() for the fixed fields in bnep_rx_frame() so a NULL
> return gates each dereference. Split the control handler so the frame
> path can pass an opcode that has already been pulled, and keep the
> byte-buffer wrapper for extension control payloads.
>
> Validation reproduced this kernel report:
> KASAN slab-out-of-bounds in bnep_rx_frame.isra.0+0x130c/0x1790
> The buggy address belongs to the object at ffff88800c0f7908 which belongs
> to the cache kmalloc-8 of size 8
> The buggy address is located 0 bytes to the right of allocated 1-byte
> region [ffff88800c0f7908, ffff88800c0f7909)
> Read of size 1
> Call trace:
>   dump_stack_lvl+0xb3/0x140 (?:?)
>   print_address_description+0x57/0x3a0 (?:?)
>   bnep_rx_frame+0x130c/0x1790 (net/bluetooth/bnep/core.c:306)
>   print_report+0xb9/0x2b0 (?:?)
>   __virt_addr_valid+0x1ba/0x3a0 (?:?)
>   srso_alias_return_thunk+0x5/0xfbef5 (?:?)
>   kasan_addr_to_slab+0x21/0x60 (?:?)
>   kasan_report+0xe0/0x110 (?:?)
>   process_one_work+0xfce/0x17e0 (kernel/workqueue.c:3200)
>   worker_thread+0x65c/0xe40 (?:?)
>   __kthread_parkme+0x184/0x230 (?:?)
>   kthread+0x35e/0x470 (?:?)
>   _raw_spin_unlock_irq+0x28/0x50 (?:?)
>   ret_from_fork+0x586/0x870 (?:?)
>   __switch_to+0x74f/0xdc0 (?:?)
>   ret_from_fork_asm+0x1a/0x30 (?:?)
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Zhang Cen <rollkingzzc@xxxxxxxxx>
> ---
>  net/bluetooth/bnep/core.c | 51 ++++++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
> index 853c8d764..156cd3026 100644
> --- a/net/bluetooth/bnep/core.c
> +++ b/net/bluetooth/bnep/core.c
> @@ -206,14 +206,11 @@ static int bnep_ctrl_set_mcfilter(struct bnep_session *s, u8 *data, int len)
>         return 0;
>  }
>
> -static int bnep_rx_control(struct bnep_session *s, void *data, int len)
> +static int bnep_rx_control_cmd(struct bnep_session *s, u8 cmd, void *data,
> +                              int len)
>  {
> -       u8  cmd = *(u8 *)data;
>         int err = 0;
>
> -       data++;
> -       len--;
> -
>         switch (cmd) {
>         case BNEP_CMD_NOT_UNDERSTOOD:
>         case BNEP_SETUP_CONN_RSP:
> @@ -254,6 +251,14 @@ static int bnep_rx_control(struct bnep_session *s, void *data, int len)
>         return err;
>  }
>
> +static int bnep_rx_control(struct bnep_session *s, void *data, int len)
> +{
> +       if (len < 1)
> +               return -EILSEQ;
> +
> +       return bnep_rx_control_cmd(s, *(u8 *)data, data + 1, len - 1);
> +}
> +
>  static int bnep_rx_extension(struct bnep_session *s, struct sk_buff *skb)
>  {
>         struct bnep_ext_hdr *h;
> @@ -299,19 +304,26 @@ static int bnep_rx_frame(struct bnep_session *s, struct sk_buff *skb)
>  {
>         struct net_device *dev = s->dev;
>         struct sk_buff *nskb;
> +       u8 *data;
>         u8 type, ctrl_type;
>
>         dev->stats.rx_bytes += skb->len;
>
> -       type = *(u8 *) skb->data;
> -       skb_pull(skb, 1);
> -       ctrl_type = *(u8 *)skb->data;
> +       data = skb_pull_data(skb, sizeof(type));
> +       if (!data)
> +               goto badframe;
> +       type = *data;
>
>         if ((type & BNEP_TYPE_MASK) >= sizeof(__bnep_rx_hlen))
>                 goto badframe;
>
>         if ((type & BNEP_TYPE_MASK) == BNEP_CONTROL) {
> -               if (bnep_rx_control(s, skb->data, skb->len) < 0) {
> +               data = skb_pull_data(skb, sizeof(ctrl_type));
> +               if (!data)
> +                       goto badframe;
> +               ctrl_type = *data;
> +
> +               if (bnep_rx_control_cmd(s, ctrl_type, skb->data, skb->len) < 0) {
>                         dev->stats.tx_errors++;
>                         kfree_skb(skb);
>                         return 0;
> @@ -325,23 +337,22 @@ static int bnep_rx_frame(struct bnep_session *s, struct sk_buff *skb)
>                 /* Verify and pull ctrl message since it's already processed */
>                 switch (ctrl_type) {
>                 case BNEP_SETUP_CONN_REQ:
> -                       /* Pull: ctrl type (1 b), len (1 b), data (len bytes) */
> -                       if (!skb_pull(skb, 2 + *(u8 *)(skb->data + 1) * 2))
> +                       /* Pull: len (1 b), data (len * 2 bytes) */
> +                       data = skb_pull_data(skb, 1);
> +                       if (!data)
> +                               goto badframe;
> +                       if (!skb_pull(skb, *data * 2))

This still looks incorrect to me, why should we do * 2? If look at
BNEP_FILTER_NET_TYPE_SET it just use the header to pull using
get_unaligned, I know this is an existing problem but except if the
BNEP_SETUP_CONN_REQ header length works on a multiple of 2 this makes
no sense to me.

>                                 goto badframe;
>                         break;
>                 case BNEP_FILTER_MULTI_ADDR_SET:
> -               case BNEP_FILTER_NET_TYPE_SET: {
> -                       u8 *hdr;
> -
> -                       /* Pull ctrl type (1 b) + len (2 b) */
> -                       hdr = skb_pull_data(skb, 3);
> -                       if (!hdr)
> +               case BNEP_FILTER_NET_TYPE_SET:
> +                       /* Pull: len (2 b), data (len bytes) */
> +                       data = skb_pull_data(skb, sizeof(u16));
> +                       if (!data)
>                                 goto badframe;
> -                       /* Pull data (len bytes); length is big-endian */
> -                       if (!skb_pull(skb, get_unaligned_be16(&hdr[1])))
> +                       if (!skb_pull(skb, get_unaligned_be16(data)))
>                                 goto badframe;
>                         break;
> -               }
>                 default:
>                         kfree_skb(skb);
>                         return 0;
> --
> 2.43.0
>


-- 
Luiz Augusto von Dentz