Re: [PATCH v5 net] nfc: hci: fix out-of-bounds read in HCP header parsing

[Paolo Abeni <pabeni@redhat.com>]

On 4/18/26 6:30 PM, Simon Horman wrote:
> On Thu, Apr 16, 2026 at 05:15:22AM +0000, Ashutosh Desai wrote:
>> nfc_hci_recv_from_llc() and nci_hci_data_received_cb() cast skb->data
>> to struct hcp_packet and read the message header byte without checking
>> that enough data is present in the linear sk_buff area. A malicious NFC
>> peer can send a 1-byte HCP frame that passes through the SHDLC layer
>> and reaches these functions, causing an out-of-bounds heap read.
>>
>> Fix this by adding pskb_may_pull() before each cast to ensure the full
>> 2-byte HCP header is pulled into the linear area before it is accessed.
>>
>> Fixes: 8b8d2e08bf0d ("NFC: HCI support")
>> Fixes: 11f54f228643 ("NFC: nci: Add HCI over NCI protocol support")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Ashutosh Desai <ashutoshdesai993@gmail.com>
>> ---
>> V4 -> V5: fix whitespace damage
>> V3 -> V4: add Fixes tags
>> V2 -> V3: drop redundant checks from nfc_hci_msg_rx_work/nci_hci_msg_rx_work;
>>           remove incorrect Suggested-by tag
>> V1 -> V2: use pskb_may_pull() instead of skb->len check
>>
>> v4: https://lore.kernel.org/netdev/177614425081.3600288.2536320552978506086@gmail.com/
>> v3: https://lore.kernel.org/netdev/20260413024329.3293075-1-ashutoshdesai993@gmail.com/
>> v2: https://lore.kernel.org/netdev/20260409150825.2217133-1-ashutoshdesai993@gmail.com/
>> v1: https://lore.kernel.org/netdev/20260408223113.2009304-1-ashutoshdesai993@gmail.com/
>>
>>  net/nfc/hci/core.c | 5 +++++
>>  net/nfc/nci/hci.c  | 5 +++++
>>  2 files changed, 10 insertions(+)
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> Review of this patch at Sashiko.dev flags a number of related problems in
> this code. I believe none of them introduced by this patch. And that
> they can all be treated as area for possible follow-up.

I agree that the issue reported by sashiko:
---
Does this patch fully resolve the out-of-bounds access?
Looking at the beginning of nfc_hci_recv_from_llc(), the code accesses
the packet header before checking if the skb has any data:
net/nfc/hci/core.c:nfc_hci_recv_from_llc() {
    packet = (struct hcp_packet *)skb->data;
    if ((packet->header & ~NFC_HCI_FRAGMENT) == 0) {
        skb_queue_tail(&hdev->rx_hcp_frags, skb);
        return;
    }
    ...
}
If a maliciously crafted 0-byte payload is received, couldn't this
result in an out-of-bounds read of uninitialized memory?
Furthermore, if the fragmentation bit is clear and this 0-byte skb
is queued, when a subsequent final fragment arrives, the reassembly
loop calculates the message length:
net/nfc/hci/core.c:nfc_hci_recv_from_llc() {
    ...
    skb_queue_walk(&hdev->rx_hcp_frags, frag_skb) {
        msg_len += (frag_skb->len - NFC_HCI_HCP_PACKET_HEADER_LEN);
    }
    ...
}
Since NFC_HCI_HCP_PACKET_HEADER_LEN is 1, wouldn't a 0-length fragment
cause this calculation to underflow to UINT_MAX, leading to an eventual
skb_over_panic() when skb_put_data() is called?
Would it be safer to add a pskb_may_pull(skb, 1) check at the very start
of the function before packet->header is accessed?
---

is pre-existing but it looks like the validation included here is almost
ineffective without addressing the above.

@Ashutosh, please include the additional validation in the next
revision, thanks!

Paolo