Re: [PATCH net v3] nfc: nci: avoid unbounded skb allocation when max_pkt_payload_len is zero

[Simon Horman]

On Sun, Sep 27, 2026 at 07:59:40PM +0800, Liu Chao wrote:
> nci_queue_tx_data_frags() uses conn_info->max_pkt_payload_len as the
> fragment size.  When that value is zero, frag_len is always zero and
> total_len never decreases.  The loop then allocates skbs without bound:
> none of them are freed inside the loop, they accumulate on frags_q, and
> there is no cond_resched() in the loop body.  A single sendmsg() can
> therefore consume all allocatable memory, and on CONFIG_PREEMPT_NONE it
> occupies the CPU long enough to trip the softlockup watchdog:
> 
>   watchdog: BUG: soft lockup - CPU#3 stuck for 26s! [kworker/3:1:57]
>   Workqueue: events rawsock_tx_work [nfc]
>   Call Trace:
>    nci_send_data+0x1ca/0x6b0 [nci]
>    nci_transceive+0xbb/0x170 [nci]
>    rawsock_tx_work+0xb5/0x1a0 [nfc]
> 
> max_pkt_payload_len comes straight from controller-supplied fields with
> no check for zero, and it is read several times along the TX path while
> the rx workqueue can update it without any lock held against this path.
> 
> Take a single READ_ONCE() snapshot of the limit in nci_send_data(),
> reject a zero limit there, and pass the snapshot down to
> nci_queue_tx_data_frags().  The fragmentation decision and the
> fragmentation loop then consume the same value, so the loop cannot spin
> on a limit that differs from the one just validated, and no plain read
> of the field is left in nci_send_data() or nci_queue_tx_data_frags().
> 
> Rejecting the zero limit at the entry of the TX data path covers the RF
> connection as well: ndev->rf_conn_info is allocated with devm_kzalloc(),
> so its max_pkt_payload_len is zero from the moment the object exists and
> only becomes usable when an activation notification assigns a
> controller-supplied value.  Checking where the value is consumed catches
> every producer of a zero limit -- the initial state, the notification,
> and any future writer.
> 
> The two producers of the field are annotated with WRITE_ONCE() to match
> the snapshot read; the remaining plain accesses on the HCI path are
> untouched here, since nci_hci_send_data() loops over a different
> conn_info instance (ndev->hci_dev->conn_info) and needs its own fix,
> which is sent separately.
> 
> The "failed to fragment tx data packet" print sits on a data path driven
> by controller/remote data and can fire on every transmit once
> fragmentation keeps failing.  Rate-limit it so a misbehaving controller
> cannot flood the log.  Note that with the zero-limit rejection moved into
> nci_send_data(), a rejected zero limit returns before this print, so the
> per-sendmsg flood does not occur through that path.
> 
> No legitimate configuration is affected: where the NCI spec does mandate
> a zero Max Data Packet Payload Size -- the NFCEE Direct RF Interface --
> nci_rf_intf_activated_ntf_packet() takes the "goto listen" shortcut,
> bypassing the assignment entirely.
> 
> While at it, drop the conn_info lookup in nci_queue_tx_data_frags():
> the caller has already validated the connection, the function now takes
> everything it needs as arguments, and the lookup was the only use of
> conn_info left in it.
> 
> Fixes: 6a2968aaf50c ("NFC: basic NCI protocol implementation")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Liu Chao <liuc63@xxxxxxxxxxxx>
> ---
> Changes in v3:
>   - Snapshot max_pkt_payload_len once in nci_send_data() with
>     READ_ONCE() and pass it down to nci_queue_tx_data_frags(), as
>     suggested by Simon Horman, so the fragmentation decision and the
>     fragmentation loop consume the same value and no plain read of the
>     field remains on this path (review:
>     https://lore.kernel.org/netdev/20260925155040.GP13925@xxxxxxxxxxxxxxxx/)
>   - Move the zero-limit rejection from nci_queue_tx_data_frags() into
>     nci_send_data(), so the check runs on the same snapshot the loop
>     consumes; this also means a rejected zero limit returns before the
>     "failed to fragment" print rather than triggering it per sendmsg
>   - Annotate the two producers of the field (RF activation NTF and
>     CORE_CONN_CREATE_RSP) with WRITE_ONCE() to match the snapshot read
>   - Drop the now-redundant conn_info lookup in nci_queue_tx_data_frags();
>     the caller already validated the connection
>   - Rate-limit the "failed to fragment tx data packet" print, which sits
>     on a data path driven by remote data, as discussed in the review of
>     the v2 series
>   - Keep the v1/v2 subject so the revision is tracked as the same fix
> 
> Changes in v2:
>   - READ_ONCE() snapshot of max_pkt_payload_len in
>     nci_queue_tx_data_frags(): with the check and the loop reading the
>     field independently, a store from the rx workqueue in between
>     could let the loop spin on a value the check had just rejected

Thanks for the updates.

Reviewed-by: Simon Horman <horms@xxxxxxxxxx>