Re: [PATCH] Bluetooth: RFCOMM: validate skb length in MCC handlers

Dear SeungJu,


Thank you for the patch.

Am 12.04.26 um 06:54 schrieb SeungJu Cheon:
> rfcomm_recv_pn(), rfcomm_recv_rpn(), rfcomm_recv_rls(), and
> rfcomm_recv_msc() cast skb->data to their respective structs
> without first checking skb->len. A remote device can send a
> short MCC frame, causing out-of-bounds reads from the skb buffer.

Nice catch. Do you have a reproducer to create such a short MCC frame?

> For rfcomm_recv_pn(), the uninitialized pn->mtu value is stored
> in d->mtu via rfcomm_apply_pn(), then echoed back to the remote
> device in the PN response, leaking kernel heap data.
> 
> This results in use of uninitialized memory, as reported by KMSAN.
> 
> Add explicit skb->len checks against the expected structure size
> at the start of each handler before accessing the payload.
> 
> =====================================================
> BUG: KMSAN: uninit-value in rfcomm_run+0x7eae/0xee90
>   rfcomm_run+0x7eae/0xee90
>   kthread+0x53f/0x600
>   ret_from_fork+0x20f/0x910
>   ret_from_fork_asm+0x1a/0x30
> 
> Uninit was created at:
>   kmem_cache_alloc_node_noprof+0x3cd/0x12d0
>   __alloc_skb+0x855/0x1190
>   vhci_write+0x125/0x960
>   vfs_write+0xbe1/0x15c0
>   ksys_write+0x1d9/0x470
>   __x64_sys_write+0x97/0xf0
>   x64_sys_call+0x2ff0/0x3ea0
>   do_syscall_64+0x134/0xf80
>   entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> CPU: 0 UID: 0 PID: 3374 Comm: krfcommd Tainted: G        W           7.0.0-rc7
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
> Kernel panic - not syncing: kmsan.panic set ...
> =====================================================
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: SeungJu Cheon <[email protected]>
> ---
>   net/bluetooth/rfcomm/core.c | 40 +++++++++++++++++++++++++++++--------
>   1 file changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> index 611a9a94151e..daeba71a1514 100644
> --- a/net/bluetooth/rfcomm/core.c
> +++ b/net/bluetooth/rfcomm/core.c
> @@ -1431,9 +1431,15 @@ static int rfcomm_apply_pn(struct rfcomm_dlc *d, int cr, struct rfcomm_pn *pn)
>   
>   static int rfcomm_recv_pn(struct rfcomm_session *s, int cr, struct sk_buff *skb)
>   {
> -	struct rfcomm_pn *pn = (void *) skb->data;
> +	struct rfcomm_pn *pn;
>   	struct rfcomm_dlc *d;
> -	u8 dlci = pn->dlci;
> +	u8 dlci;
> +
> +	if (skb->len < sizeof(*pn))
> +		return -EINVAL;
> +
> +	pn = (void *) skb->data;
> +	dlci = pn->dlci;

gemini/gemini-3.1-pro-preview comments [1]. No idea if it’s a valid comment:

> Before these handlers are reached, rfcomm_recv_mcc() unconditionally casts
> skb->data to struct rfcomm_mcc * and reads mcc->type and mcc->len. Does
> this leave an out-of-bounds read unpatched if a remote device sends an MCC
> frame with 0 or 1 bytes of payload?
> Additionally, if rfcomm_recv_frame() unconditionally trims the FCS with
> skb->len--; skb->tail--;, could a 0-byte payload cause skb->len to
> underflow to UINT_MAX?
> Since skb->len is unsigned, this new check (UINT_MAX < 8) would evaluate to
> false, potentially bypassing this protection entirely.
>   
>   	BT_DBG("session %p state %ld dlci %d", s, s->state, dlci);
>   
> @@ -1483,8 +1489,8 @@ static int rfcomm_recv_pn(struct rfcomm_session *s, int cr, struct sk_buff *skb)
>   
>   static int rfcomm_recv_rpn(struct rfcomm_session *s, int cr, int len, struct sk_buff *skb)
>   {
> -	struct rfcomm_rpn *rpn = (void *) skb->data;
> -	u8 dlci = __get_dlci(rpn->dlci);
> +	struct rfcomm_rpn *rpn;
> +	u8 dlci;
>   
>   	u8 bit_rate  = 0;
>   	u8 data_bits = 0;
> @@ -1495,6 +1501,12 @@ static int rfcomm_recv_rpn(struct rfcomm_session *s, int cr, int len, struct sk_
>   	u8 xoff_char = 0;
>   	u16 rpn_mask = RFCOMM_RPN_PM_ALL;
>   
> +	if (skb->len < sizeof(*rpn))
> +		return -EINVAL;

gemini/gemini-3.1-pro-preview comments [1]:

> Does this unconditionally drop legitimate 1-byte Remote Port Negotiation
> (RPN) requests?
> Looking at the existing code further down in rfcomm_recv_rpn():
>     if (len == 1) {
>         /* This is a request, return default (according to ETSI TS 07.10) settings */
>         ...
> Since sizeof(struct rfcomm_rpn) is 8 bytes, enforcing this minimum length
> would reject valid 1-byte queries allowed by the ETSI TS 07.10 standard.


> +
> +	rpn = (void *) skb->data;
> +	dlci = __get_dlci(rpn->dlci);
> +
>   	BT_DBG("dlci %d cr %d len 0x%x bitr 0x%x line 0x%x flow 0x%x xonc 0x%x xoffc 0x%x pm 0x%x",
>   		dlci, cr, len, rpn->bit_rate, rpn->line_settings, rpn->flow_ctrl,
>   		rpn->xon_char, rpn->xoff_char, rpn->param_mask);
> @@ -1589,8 +1601,14 @@ static int rfcomm_recv_rpn(struct rfcomm_session *s, int cr, int len, struct sk_
>   
>   static int rfcomm_recv_rls(struct rfcomm_session *s, int cr, struct sk_buff *skb)
>   {
> -	struct rfcomm_rls *rls = (void *) skb->data;
> -	u8 dlci = __get_dlci(rls->dlci);
> +	struct rfcomm_rls *rls;
> +	u8 dlci;
> +
> +	if (skb->len < sizeof(*rls))
> +		return -EINVAL;
> +
> +	rls = (void *) skb->data;
> +	dlci = __get_dlci(rls->dlci);
>   
>   	BT_DBG("dlci %d cr %d status 0x%x", dlci, cr, rls->status);
>   
> @@ -1608,9 +1626,15 @@ static int rfcomm_recv_rls(struct rfcomm_session *s, int cr, struct sk_buff *skb
>   
>   static int rfcomm_recv_msc(struct rfcomm_session *s, int cr, struct sk_buff *skb)
>   {
> -	struct rfcomm_msc *msc = (void *) skb->data;
> +	struct rfcomm_msc *msc;
>   	struct rfcomm_dlc *d;
> -	u8 dlci = __get_dlci(msc->dlci);
> +	u8 dlci;
> +
> +	if (skb->len < sizeof(*msc))
> +		return -EINVAL;
> +
> +	msc = (void *) skb->data;
> +	dlci = __get_dlci(msc->dlci);
>   
>   	BT_DBG("dlci %d cr %d v24 0x%x", dlci, cr, msc->v24_sig);
>   


Kind regards,

Paul


[1]: 
https://sashiko.dev/#/patchset/20260412045457.53100-1-suunj1331%40gmail.com