Re: [PATCH net v2 1/3] nfc: nci: fix u8 underflow in nci_store_general_bytes_nfc_dep

[Paolo Abeni <pabeni@redhat.com>]

On 4/14/26 9:34 AM, Paolo Abeni wrote:
> On 4/9/26 8:59 PM, Lekë Hapçiu wrote:
>> From: Lekë Hapçiu <framemain@outlook.com>
>>
>> nci_store_general_bytes_nfc_dep() computes the number of General Bytes
>> to copy from an ATR_RES or ATR_REQ frame by subtracting a fixed header
>> offset from the peer-supplied length field:
>>
>>   ndev->remote_gb_len = min_t(__u8,
>>       (atr_res_len - NFC_ATR_RES_GT_OFFSET),   /* offset = 15 */
>>       NFC_ATR_RES_GB_MAXSIZE);
>>
>> Both length fields are __u8.  When a malicious NFC-DEP target (POLL mode)
>> or initiator (LISTEN mode) sends an ATR_RES/ATR_REQ whose length field is
>> smaller than the fixed offset (< 15 or < 14 respectively), the subtraction
>> wraps in unsigned u8 arithmetic:
>>
>>   e.g. atr_res_len = 0 -> (u8)(0 - 15) = 241
>>
>> min_t(__u8, 241, 47) then yields 47, so the subsequent memcpy reads
>> 47 bytes from beyond the end of the valid activation parameter data into
>> ndev->remote_gb[].  This buffer is later passed to nfc_llcp_parse_gb_tlv()
>> as a TLV array, feeding directly into the TLV parser hardened by the
>> companion patch.
>>
>> Fix: add an explicit lower-bound check on each length field before the
>> subtraction.  If the length is smaller than the required offset the frame
>> is malformed; leave remote_gb_len at zero and skip the memcpy.
>>
>> Both the POLL (atr_res_len / NFC_ATR_RES_GT_OFFSET = 15) and the LISTEN
>> (atr_req_len / NFC_ATR_REQ_GT_OFFSET = 14) paths are affected; both are
>> fixed symmetrically.
>>
>> Reachability: the ATR_RES is sent by an NFC-DEP target during RF
>> activation, before any authentication or pairing.  The bug is therefore
>> reachable from any NFC peer within ~4 cm.
>>
>> Fixes: a99903ec4566 ("NFC: NCI: Handle Target mode activation")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Lekë Hapçiu <framemain@outlook.com>
>> ---
>>  net/nfc/nci/ntf.c | 22 ++++++++++++++--------
>>  1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
>> index c96512bb8..8eb295580 100644
>> --- a/net/nfc/nci/ntf.c
>> +++ b/net/nfc/nci/ntf.c
>> @@ -631,25 +631,31 @@ static int nci_store_general_bytes_nfc_dep(struct nci_dev *ndev,
>>  	switch (ntf->activation_rf_tech_and_mode) {
>>  	case NCI_NFC_A_PASSIVE_POLL_MODE:
>>  	case NCI_NFC_F_PASSIVE_POLL_MODE:
>> +		if (ntf->activation_params.poll_nfc_dep.atr_res_len <
>> +		    NFC_ATR_RES_GT_OFFSET)
>> +			break;
> 
> This does not look the right fix: nci_store_general_bytes_nfc_dep() will
> return success to the caller, and processing will proceed even if the
> packet is malformed.
> 
> Looking at the (rather incomplete) error handling in
> nci_rf_intf_activated_ntf_packet(), the latter function should error out
> with EINVAL for truncated/malformed packets.
> 
> You should return a proper error code here _and_ handle such error in
> nci_rf_intf_activated_ntf_packet().
> 
> The same comment applies to the simlar check below.
> 
>>  		ndev->remote_gb_len = min_t(__u8,
>> -			(ntf->activation_params.poll_nfc_dep.atr_res_len
>> -						- NFC_ATR_RES_GT_OFFSET),
>> +			ntf->activation_params.poll_nfc_dep.atr_res_len
>> +						- NFC_ATR_RES_GT_OFFSET,
> 
> Please do not include style-related changes in 'net' fix: it should
> include the minimal delta to address the issue.
> 
> Other similar chuncks below.

I almost forgot: do not send you patches in reply to older revision: it
will foul patchwork and make the review process harder, if possible at all.

/P