* [PATCH] nfc: st-nci: Fix an incorrect skb_buff size in 'st_nci_i2c_read()'
@ 2019-08-06 14:16 Christophe JAILLET
2019-08-12 3:57 ` David Miller
0 siblings, 1 reply; 3+ messages in thread
From: Christophe JAILLET @ 2019-08-06 14:16 UTC (permalink / raw)
To: tglx, gregkh, colin.king, davem, allison
Cc: netdev, linux-kernel, kernel-janitors, Christophe JAILLET
In 'st_nci_i2c_read()', we allocate a sk_buff with a size of
ST_NCI_I2C_MIN_SIZE + len.
However, later on, we first 'skb_reserve()' ST_NCI_I2C_MIN_SIZE bytes, then
we 'skb_put()' ST_NCI_I2C_MIN_SIZE bytes.
Finally, if 'len' is not 0, we 'skb_put()' 'len' bytes.
So we use ST_NCI_I2C_MIN_SIZE*2 + len bytes.
This is incorrect and should already panic. I guess that it does not occur
because of extra memory allocated because of some rounding.
Fix it and allocate enough room for the 'skb_reserve()' and the 'skb_put()'
calls.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is LIKELY INCORRECT. So think twice to what is the correct
solution before applying it.
Maybe the skb_reserve should be axed or some other sizes are incorrect.
There seems to be an issue, that's all I can say.
---
drivers/nfc/st-nci/i2c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nfc/st-nci/i2c.c b/drivers/nfc/st-nci/i2c.c
index 55d600cd3861..12e0425131c8 100644
--- a/drivers/nfc/st-nci/i2c.c
+++ b/drivers/nfc/st-nci/i2c.c
@@ -126,7 +126,7 @@ static int st_nci_i2c_read(struct st_nci_i2c_phy *phy,
return -EBADMSG;
}
- *skb = alloc_skb(ST_NCI_I2C_MIN_SIZE + len, GFP_KERNEL);
+ *skb = alloc_skb(ST_NCI_I2C_MIN_SIZE * 2 + len, GFP_KERNEL);
if (*skb == NULL)
return -ENOMEM;
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] nfc: st-nci: Fix an incorrect skb_buff size in 'st_nci_i2c_read()'
2019-08-06 14:16 [PATCH] nfc: st-nci: Fix an incorrect skb_buff size in 'st_nci_i2c_read()' Christophe JAILLET
@ 2019-08-12 3:57 ` David Miller
2019-08-20 5:33 ` Christophe JAILLET
0 siblings, 1 reply; 3+ messages in thread
From: David Miller @ 2019-08-12 3:57 UTC (permalink / raw)
To: christophe.jaillet
Cc: tglx, gregkh, colin.king, allison, netdev, linux-kernel, kernel-janitors
From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Date: Tue, 6 Aug 2019 16:16:40 +0200
> In 'st_nci_i2c_read()', we allocate a sk_buff with a size of
> ST_NCI_I2C_MIN_SIZE + len.
>
> However, later on, we first 'skb_reserve()' ST_NCI_I2C_MIN_SIZE bytes, then
> we 'skb_put()' ST_NCI_I2C_MIN_SIZE bytes.
> Finally, if 'len' is not 0, we 'skb_put()' 'len' bytes.
>
> So we use ST_NCI_I2C_MIN_SIZE*2 + len bytes.
>
> This is incorrect and should already panic. I guess that it does not occur
> because of extra memory allocated because of some rounding.
>
> Fix it and allocate enough room for the 'skb_reserve()' and the 'skb_put()'
> calls.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This patch is LIKELY INCORRECT. So think twice to what is the correct
> solution before applying it.
> Maybe the skb_reserve should be axed or some other sizes are incorrect.
> There seems to be an issue, that's all I can say.
The skb_reserve() should be removed, and the second memcpy() should remove
the " + ST_NCI_I2C_MIN_SIZE".
This SKB just get sent down to ndlc_recv() so the content returned from I2C
should places at skb->data to be processed.
Pretty clear this code was never tested.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] nfc: st-nci: Fix an incorrect skb_buff size in 'st_nci_i2c_read()'
2019-08-12 3:57 ` David Miller
@ 2019-08-20 5:33 ` Christophe JAILLET
0 siblings, 0 replies; 3+ messages in thread
From: Christophe JAILLET @ 2019-08-20 5:33 UTC (permalink / raw)
To: David Miller
Cc: tglx, gregkh, colin.king, allison, netdev, linux-kernel, kernel-janitors
Le 12/08/2019 à 05:57, David Miller a écrit :
> From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Date: Tue, 6 Aug 2019 16:16:40 +0200
>
>> In 'st_nci_i2c_read()', we allocate a sk_buff with a size of
>> ST_NCI_I2C_MIN_SIZE + len.
>>
>> However, later on, we first 'skb_reserve()' ST_NCI_I2C_MIN_SIZE bytes, then
>> we 'skb_put()' ST_NCI_I2C_MIN_SIZE bytes.
>> Finally, if 'len' is not 0, we 'skb_put()' 'len' bytes.
>>
>> So we use ST_NCI_I2C_MIN_SIZE*2 + len bytes.
>>
>> This is incorrect and should already panic. I guess that it does not occur
>> because of extra memory allocated because of some rounding.
>>
>> Fix it and allocate enough room for the 'skb_reserve()' and the 'skb_put()'
>> calls.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> This patch is LIKELY INCORRECT. So think twice to what is the correct
>> solution before applying it.
>> Maybe the skb_reserve should be axed or some other sizes are incorrect.
>> There seems to be an issue, that's all I can say.
> The skb_reserve() should be removed,
I don't fully understand the potential implications, but looks ok to me.
At least, the allocated memory and the size of the used memory would match.
What I don't understand is why is does not BUG_ON with the current code.
Does my suspected "over allocation" because of rounding/aligment could
hide the issue?
A Tested-by: by someone who has the corresponding hardware would also be
useful IMHO.
> and the second memcpy() should remove
> the " + ST_NCI_I2C_MIN_SIZE".
Hmm, not sure on this one.
The skb is manipulated only with skb_put. So only the tail pointer and
len are updated. The data pointer remains at the same position, so there
should effectively be an offset of ST_NCI_I2C_MIN_SIZE for the 2nd memcpy.
Maybe, using skb_put_data would be cleaner here, in order to
"concatenate" these 2 parts without having to handle by hand the right
position in the buffer.
If you agree, I'll send a V2.
Thx for the review and comments.
CJ
> This SKB just get sent down to ndlc_recv() so the content returned from I2C
> should places at skb->data to be processed.
>
> Pretty clear this code was never tested.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-08-20 5:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-06 14:16 [PATCH] nfc: st-nci: Fix an incorrect skb_buff size in 'st_nci_i2c_read()' Christophe JAILLET
2019-08-12 3:57 ` David Miller
2019-08-20 5:33 ` Christophe JAILLET
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).