Re: [PATCH net] nfc: nci: reject unusable max payload limits

[Simon Horman <horms@kernel.org>]

On Sat, Sep 19, 2026 at 01:49:31PM +0800, Liu Chao wrote:
> nci_core_conn_create_rsp_packet() copies the controller-supplied
> max_ctrl_pkt_payload_len into the new connection without any
> validation, and nci_hci_send_data() sizes its fragments from it.
> When the controller reports 0 or 1, the subtraction in the loop
> underflows (skb->len is unsigned), the "last packet" branch is
> always taken, and skb_put_data() runs past skb->end into
> skb_over_panic(). A limit of 2 still works: the first packet
> carries the two HCP header bytes, and every chained packet one
> payload byte.
> 
> Reject a zero limit at parse time, before the conn_info is
> allocated and published, so there is nothing to unwind. A
> zero-payload connection cannot carry any data anyway, and both
> in-tree creators start sending right after the connection comes
> up: st-nci sets up its HCI session, fdp downloads firmware
> through the generic data path.
> 
> A limit of 1 stays legal at the NCI layer, since
> nci_queue_tx_data_frags() can ship one-byte fragments over such
> a connection. Only HCI needs two bytes for the HCP header, so
> that check lives in nci_hci_send_data(), which snapshots the
> limit the same way nci_queue_tx_data_frags() does and returns
> -EPROTO below 2.
> 
> Fixes: 4aeee6871e8c ("NFC: nci: Add dynamic logical connections support")
> Fixes: 11f54f228643 ("NFC: nci: Add HCI over NCI protocol support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
> Link: https://lore.kernel.org/netdev/20260918185458.2711284-1-liuc63@xiaopeng.com

Hi Liu,

I am wondering if this patch is intended as an alternative or supplement
to your patch at the link above.

If it is an alternative then I'd appreciate some clarification of how
the following point in the cover letter of the patch at the link above
is addressed:

  Reject the zero value in the fragmentation path rather than at the
  assignment sites.  nci_queue_tx_data_frags() is the only place that
  loops over the RF data path's conn_info, and nci_send_data() takes
  the non-fragmenting branch only for skb->len <= max_pkt_payload_len,
  which for a zero limit means empty skbs alone.  Validating on
  assignment would not be sufficient either, because
  nci_rf_disc_rsp_packet() allocates ndev->rf_conn_info with
  devm_kzalloc(), so max_pkt_payload_len is already zero before any
  notification arrives.

In particular the "not be sufficient" part.

And regardless, I have a concern, which is touched on in the AI-generated
review I passed on in my response to the patch at the link above.

My concern is that conn_info->max_pkt_payload_len is read by various
functions in the call chain leading to nci_queue_tx_data_frags(). But it
can change at any time, leading to possible inconsistencies.

I have not analysed what they are or if they manifest in bugs.
But I do wonder if a single snapshot of max_pkt_payload_len needs
to be taken and passed down the call-chain.

...