[PATCH net] nfc: fdp: bound the device-supplied read size in fdp_nci_i2c_read()

[PATCH net] nfc: fdp: bound the device-supplied read size in fdp_nci_i2c_read()

[Doruk Tan Ozturk]

fdp_nci_i2c_read() reads a "length packet" from the FDP I2C controller and
computes the size of the next I2C transfer from two device-supplied bytes:

	phy->next_read_size = (tmp[2] << 8) + tmp[3] + 3;

next_read_size is a u16 (up to 65535) and is never bounded. On the next
loop iteration it is used directly as the length passed to

	i2c_master_recv(client, tmp, len);

which reads into the fixed 261-byte stack buffer
tmp[FDP_NCI_I2C_MAX_PAYLOAD]. A malicious or malfunctioning controller
that reports a large length thus overflows the stack buffer -- the
r != len check runs only after the read has already happened.

Reject a next-read size larger than the buffer and resynchronize.

Found by 0sec (https://0sec.ai) using automated source analysis; the
missing bound is evident from source. Compile-tested.

Fixes: a06347c04c13 ("NFC: Add Intel Fields Peak NFC solution driver")
Cc: stable@vger.kernel.org
Assisted-by: 0sec:claude-opus-4-8
Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
---
 drivers/nfc/fdp/i2c.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/nfc/fdp/i2c.c b/drivers/nfc/fdp/i2c.c
index c1896a1d978c..581f85f0dfa8 100644
--- a/drivers/nfc/fdp/i2c.c
+++ b/drivers/nfc/fdp/i2c.c
@@ -128,7 +128,7 @@ static const struct nfc_phy_ops i2c_phy_ops = {
 
 static int fdp_nci_i2c_read(struct fdp_i2c_phy *phy, struct sk_buff **skb)
 {
-	int r, len;
+	int r = -EREMOTEIO, len;
 	u8 tmp[FDP_NCI_I2C_MAX_PAYLOAD], lrc, k;
 	u16 i;
 	struct i2c_client *client = phy->i2c_dev;
@@ -140,6 +140,13 @@ static int fdp_nci_i2c_read(struct fdp_i2c_phy *phy, struct sk_buff **skb)
 
 		len = phy->next_read_size;
 
+		if (len > FDP_NCI_I2C_MAX_PAYLOAD) {
+			dev_dbg(&client->dev, "%s: read size %d too large\n",
+				__func__, len);
+			phy->next_read_size = FDP_NCI_I2C_MIN_PAYLOAD;
+			goto flush;
+		}
+
 		r = i2c_master_recv(client, tmp, len);
 		if (r != len) {
 			dev_dbg(&client->dev, "%s: i2c recv err: %d\n",
-- 
2.43.0

Re: [PATCH net] nfc: fdp: bound the device-supplied read size in fdp_nci_i2c_read()

[Simon Horman]

On Sat, Jul 11, 2026 at 02:36:41PM +0200, Doruk Tan Ozturk wrote:
> fdp_nci_i2c_read() reads a "length packet" from the FDP I2C controller and
> computes the size of the next I2C transfer from two device-supplied bytes:
> 
> 	phy->next_read_size = (tmp[2] << 8) + tmp[3] + 3;
> 
> next_read_size is a u16 (up to 65535) and is never bounded. On the next
> loop iteration it is used directly as the length passed to
> 
> 	i2c_master_recv(client, tmp, len);
> 
> which reads into the fixed 261-byte stack buffer
> tmp[FDP_NCI_I2C_MAX_PAYLOAD]. A malicious or malfunctioning controller
> that reports a large length thus overflows the stack buffer -- the
> r != len check runs only after the read has already happened.
> 
> Reject a next-read size larger than the buffer and resynchronize.
> 
> Found by 0sec (https://0sec.ai) using automated source analysis; the
> missing bound is evident from source. Compile-tested.
> 
> Fixes: a06347c04c13 ("NFC: Add Intel Fields Peak NFC solution driver")
> Cc: stable@vger.kernel.org
> Assisted-by: 0sec:claude-opus-4-8
> Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
> ---
>  drivers/nfc/fdp/i2c.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nfc/fdp/i2c.c b/drivers/nfc/fdp/i2c.c
> index c1896a1d978c..581f85f0dfa8 100644
> --- a/drivers/nfc/fdp/i2c.c
> +++ b/drivers/nfc/fdp/i2c.c
> @@ -128,7 +128,7 @@ static const struct nfc_phy_ops i2c_phy_ops = {
>  
>  static int fdp_nci_i2c_read(struct fdp_i2c_phy *phy, struct sk_buff **skb)
>  {
> -	int r, len;
> +	int r = -EREMOTEIO, len;
>  	u8 tmp[FDP_NCI_I2C_MAX_PAYLOAD], lrc, k;
>  	u16 i;
>  	struct i2c_client *client = phy->i2c_dev;
> @@ -140,6 +140,13 @@ static int fdp_nci_i2c_read(struct fdp_i2c_phy *phy, struct sk_buff **skb)
>  
>  		len = phy->next_read_size;
>  
> +		if (len > FDP_NCI_I2C_MAX_PAYLOAD) {
> +			dev_dbg(&client->dev, "%s: read size %d too large\n",
> +				__func__, len);
> +			phy->next_read_size = FDP_NCI_I2C_MIN_PAYLOAD;

I think it would be more robust to explicitly set r here.
Because it is assigned a little later in the loop, overriding
the default assignment made by the first hunk of this patch.

> +			goto flush;
> +		}
> +
>  		r = i2c_master_recv(client, tmp, len);
>  		if (r != len) {
>  			dev_dbg(&client->dev, "%s: i2c recv err: %d\n",
> -- 
> 2.43.0
> 

Re: [PATCH net] nfc: fdp: bound the device-supplied read size in fdp_nci_i2c_read()

[David Heidelberg]

On 20/07/2026 12:16, Simon Horman wrote:
> On Sat, Jul 11, 2026 at 02:36:41PM +0200, Doruk Tan Ozturk wrote:
>> fdp_nci_i2c_read() reads a "length packet" from the FDP I2C controller and
>> computes the size of the next I2C transfer from two device-supplied bytes:
>>
>> 	phy->next_read_size = (tmp[2] << 8) + tmp[3] + 3;
>>
>> next_read_size is a u16 (up to 65535) and is never bounded. On the next
>> loop iteration it is used directly as the length passed to
>>
>> 	i2c_master_recv(client, tmp, len);
>>
>> which reads into the fixed 261-byte stack buffer
>> tmp[FDP_NCI_I2C_MAX_PAYLOAD]. A malicious or malfunctioning controller
>> that reports a large length thus overflows the stack buffer -- the
>> r != len check runs only after the read has already happened.
>>
>> Reject a next-read size larger than the buffer and resynchronize.
>>
>> Found by 0sec (https://0sec.ai) using automated source analysis; the
>> missing bound is evident from source. Compile-tested.
>>
>> Fixes: a06347c04c13 ("NFC: Add Intel Fields Peak NFC solution driver")
>> Cc: stable@vger.kernel.org
>> Assisted-by: 0sec:claude-opus-4-8
>> Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
>> ---
>>   drivers/nfc/fdp/i2c.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nfc/fdp/i2c.c b/drivers/nfc/fdp/i2c.c
>> index c1896a1d978c..581f85f0dfa8 100644
>> --- a/drivers/nfc/fdp/i2c.c
>> +++ b/drivers/nfc/fdp/i2c.c
>> @@ -128,7 +128,7 @@ static const struct nfc_phy_ops i2c_phy_ops = {
>>   
>>   static int fdp_nci_i2c_read(struct fdp_i2c_phy *phy, struct sk_buff **skb)
>>   {
>> -	int r, len;
>> +	int r = -EREMOTEIO, len;
>>   	u8 tmp[FDP_NCI_I2C_MAX_PAYLOAD], lrc, k;
>>   	u16 i;
>>   	struct i2c_client *client = phy->i2c_dev;
>> @@ -140,6 +140,13 @@ static int fdp_nci_i2c_read(struct fdp_i2c_phy *phy, struct sk_buff **skb)
>>   
>>   		len = phy->next_read_size;
>>   
>> +		if (len > FDP_NCI_I2C_MAX_PAYLOAD) {
>> +			dev_dbg(&client->dev, "%s: read size %d too large\n",
>> +				__func__, len);
>> +			phy->next_read_size = FDP_NCI_I2C_MIN_PAYLOAD;
> 
> I think it would be more robust to explicitly set r here.
> Because it is assigned a little later in the loop, overriding
> the default assignment made by the first hunk of this patch.

I agree, I was looking at this patch yesterday and the current format feels 
unfavorable.

Please send next revision with Simon suggestion :)

Thank you both!
David

> 
>> +			goto flush;
>> +		}
>> +
>>   		r = i2c_master_recv(client, tmp, len);
>>   		if (r != len) {
>>   			dev_dbg(&client->dev, "%s: i2c recv err: %d\n",
>> -- 
>> 2.43.0
>>

-- 
David Heidelberg