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

[Liu Chao <liuc63@xiaopeng.com>]

Hi Simon,

Thanks for stepping in. Answering this is on me, and v2, posted
right after this mail, covers the other form.

To your question: no, I don't think any of the three should block
this patch. The TOCTOU race and the conn_info lifetime problem are
pre-existing and this patch widens neither. I'd rather fix them in
follow-ups than fold a lifetime fix into a 5-line bounds check.

On the UAF, you wrote that it "may actually be made worse" by
this patch. I don't see how. The guard adds no dereference the
loop doesn't already perform: with the check removed, the next
iteration still reads the same field through the same unlocked
pointer, and the list walk is unlocked either way. If you have a
concrete window in mind, please spell it out.

> - [High] Incomplete fix: a controller-supplied
>   conn_info->max_pkt_payload_len of 0 (or 1) is still consumed by
>   nci_hci_send_data() in net/nfc/nci/hci.c ...

Agreed, pre-existing and not made worse by this patch. Sashiko is
right that "nci_queue_tx_data_frags() is the only place that loops"
was too broad; nci_hci_send_data() loops over the same field. Two
scope facts, though: the RF path uses ndev->rf_conn_info (allocated
in nci_rf_disc_rsp_packet(), limit set from ntf.max_data_pkt_payload_size
in nci_rf_intf_activated_ntf_packet()), while the HCI path uses
ndev->hci_dev->conn_info (allocated in
nci_core_conn_create_rsp_packet(), limit set from
rsp->max_ctrl_pkt_payload_len). Different objects, so the patch
doesn't miss the bug it fixes, but the sentence oversold it. v2
rewords it.

The new check can still reject a zero on the HCI path, but that
buys little. For non-empty payloads the unsigned underflow in the
loop above drives len past skb->end into skb_over_panic() -> BUG()
before nci_send_data() is ever called, and what does reach
nci_send_data() gets there only after skb_put_data() has run. So
the path needs its own fix.

That fix belongs where the response is parsed, before the conn_info
is published. rsp->max_ctrl_pkt_payload_len is available as soon as
rsp is set up (rsp.c:315); the object is allocated after that, and
is on ndev->conn_info_list (rsp.c:342) and installed as
ndev->hci_dev->conn_info (rsp.c:345) before the field is written
(rsp.c:348). A check at parse time means the object is never
published, nothing to unwind. A check at the assignment comes too
late: the object is already linked and already pointed at, and the
existing error path (free_conn_info, rsp.c:352) only covers the
pre-list_add allocation failure, with no list_del and no clearing
of hci_dev->conn_info. A zero there can only be the controller
reporting a broken limit, so rejecting the response outright is
the right call.

The RF path is the opposite case. nci_rf_disc_rsp_packet()
devm_kzallocs rf_conn_info, so zero is its legitimate initial
state, the "not yet activated" value, not something the controller
reported. The activation notification can't cover that window, and
rejecting a zero there only restores it for the next transmitter.
The reproducer walks exactly this path: RF_DISCOVER_RSP creates
rf_conn_info, the injected ACTIVATED_NTF stores zero, the target
still activates, the next data frame spins.

The entry path doesn't help either. nci_send_data() takes the
non-fragmenting branch only for skb->len <= max_pkt_payload_len,
so with a zero limit a non-empty frame reaches the fragmentation
loop no matter what the writers do. The check at the point of use
covers every producer of a zero limit: initial state, the
notification, any future writer. That's why it lives there for RF
and won't for HCI.

The arithmetic in nci_hci_send_data() needs its own fix regardless.
With max_pkt_payload_len 0 or 1, i + max_pkt_payload_len -
(skb->len + 1) is evaluated unsigned and wraps, the "last packet"
branch is always taken, len becomes the whole payload, and
skb_put_data() runs past skb->end into skb_over_panic() -> BUG()
instead of merely spinning. A plain != 0 test wouldn't cover that.
I'll send the arithmetic fix plus the parse-time zero check for
that path.

> - [High] TOCTOU: the new guard reads conn_info->max_pkt_payload_len
>   once but the fragmentation loop re-reads it on every iteration.

Agreed, the window is real and pre-existing. I've folded Sashiko's
snapshot into v2 rather than deferring it: the loop re-reads the
field either way, so the race itself is old, but the new check
shouldn't be bypassable by the store it guards. v2 snapshots the
field once with READ_ONCE() and uses the snapshot for both the
check and the min_t() bound.

> Pre-existing issues:
> - [High] Use-after-free of struct nci_conn_info ...

Agreed. Unlocked list walk, devm_kfree() straight from the rx
worker, and nci_rsp_packet() dispatching CORE_CONN_CLOSE_RSP with
no outstanding command. The HCI side has the same shape:
nci_core_conn_close_rsp_packet() clears ndev->rf_conn_info but
leaves ndev->hci_dev->conn_info (written only at rsp.c:345, never
cleared) pointing at the freed object. I'll do the lifetime work
as its own series rather than smuggle it into a 5-line bounds fix.

v2 follows this mail with the changelog correction and the
READ_ONCE() snapshot; the HCI loop gets its own patch after that,
then the lifetime series.

Thanks,

Liu Chao