* [PATCH 1/1] ionic: fix array overflow on receiving too many fragments for a packet
@ 2020-12-06 13:35 Xiaohui Zhang
2020-12-07 1:51 ` Jesse Brandeburg
0 siblings, 1 reply; 3+ messages in thread
From: Xiaohui Zhang @ 2020-12-06 13:35 UTC (permalink / raw)
To: Xiaohui Zhang, Shannon Nelson, Pensando Drivers,
David S . Miller, Jakub Kicinski, netdev, linux-kernel
From: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
If the hardware receives an oversized packet with too many rx fragments,
skb_shinfo(skb)->frags can overflow and corrupt memory of adjacent pages.
This becomes especially visible if it corrupts the freelist pointer of
a slab page.
Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
---
drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
index 169ac4f54..a3e274c65 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
@@ -102,8 +102,12 @@ static struct sk_buff *ionic_rx_frags(struct ionic_queue *q,
dma_unmap_page(dev, dma_unmap_addr(page_info, dma_addr),
PAGE_SIZE, DMA_FROM_DEVICE);
- skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
+ struct skb_shared_info *shinfo = skb_shinfo(skb);
+
+ if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags)) {
+ skb_add_rx_frag(skb, shinfo->nr_frags,
page_info->page, 0, frag_len, PAGE_SIZE);
+ }
page_info->page = NULL;
page_info++;
i--;
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] ionic: fix array overflow on receiving too many fragments for a packet
2020-12-06 13:35 [PATCH 1/1] ionic: fix array overflow on receiving too many fragments for a packet Xiaohui Zhang
@ 2020-12-07 1:51 ` Jesse Brandeburg
2020-12-07 3:41 ` Shannon Nelson
0 siblings, 1 reply; 3+ messages in thread
From: Jesse Brandeburg @ 2020-12-07 1:51 UTC (permalink / raw)
To: Xiaohui Zhang
Cc: Shannon Nelson, Pensando Drivers, David S . Miller,
Jakub Kicinski, netdev, linux-kernel
Xiaohui Zhang wrote:
> From: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
>
> If the hardware receives an oversized packet with too many rx fragments,
> skb_shinfo(skb)->frags can overflow and corrupt memory of adjacent pages.
> This becomes especially visible if it corrupts the freelist pointer of
> a slab page.
>
> Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
Hi, thanks for your patch.
It appears this is a part of a series of patches (at least this one and
one to the ice driver) - please send as one series, with a cover letter
explanation.
Please justify how this is a bug and how this is found / reproduced.
I'll respond separately to the ice driver patch as I don't know this
hardware and it's limits, but I suspect that you've tried to fix a bug
where there was none. (It seems like something a code scanner might find
and be confused about)
> ---
> drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> index 169ac4f54..a3e274c65 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> @@ -102,8 +102,12 @@ static struct sk_buff *ionic_rx_frags(struct ionic_queue *q,
>
> dma_unmap_page(dev, dma_unmap_addr(page_info, dma_addr),
> PAGE_SIZE, DMA_FROM_DEVICE);
> - skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
> + struct skb_shared_info *shinfo = skb_shinfo(skb);
you can't declare variables in the middle of a code flow in C, did you
compile this?
> +
> + if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags)) {
> + skb_add_rx_frag(skb, shinfo->nr_frags,
> page_info->page, 0, frag_len, PAGE_SIZE);
> + }
> page_info->page = NULL;
> page_info++;
> i--;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] ionic: fix array overflow on receiving too many fragments for a packet
2020-12-07 1:51 ` Jesse Brandeburg
@ 2020-12-07 3:41 ` Shannon Nelson
0 siblings, 0 replies; 3+ messages in thread
From: Shannon Nelson @ 2020-12-07 3:41 UTC (permalink / raw)
To: Jesse Brandeburg, Xiaohui Zhang
Cc: Pensando Drivers, David S . Miller, Jakub Kicinski, netdev, linux-kernel
On 12/6/20 5:51 PM, Jesse Brandeburg wrote:
> Xiaohui Zhang wrote:
>
>> From: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
>>
>> If the hardware receives an oversized packet with too many rx fragments,
>> skb_shinfo(skb)->frags can overflow and corrupt memory of adjacent pages.
>> This becomes especially visible if it corrupts the freelist pointer of
>> a slab page.
>>
>> Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
> Hi, thanks for your patch.
>
> It appears this is a part of a series of patches (at least this one and
> one to the ice driver) - please send as one series, with a cover letter
> explanation.
>
> Please justify how this is a bug and how this is found / reproduced.
>
> I'll respond separately to the ice driver patch as I don't know this
> hardware and it's limits, but I suspect that you've tried to fix a bug
> where there was none. (It seems like something a code scanner might find
> and be confused about)
>
>> ---
>> drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> index 169ac4f54..a3e274c65 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> @@ -102,8 +102,12 @@ static struct sk_buff *ionic_rx_frags(struct ionic_queue *q,
>>
>> dma_unmap_page(dev, dma_unmap_addr(page_info, dma_addr),
>> PAGE_SIZE, DMA_FROM_DEVICE);
>> - skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
>> + struct skb_shared_info *shinfo = skb_shinfo(skb);
> you can't declare variables in the middle of a code flow in C, did you
> compile this?
>
>> +
>> + if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags)) {
>> + skb_add_rx_frag(skb, shinfo->nr_frags,
>> page_info->page, 0, frag_len, PAGE_SIZE);
>> + }
Is this just dropping the remaining frags without dropping the rest of
the skb? Is this going to leave an incorrect length in the skb?
A single statement after the 'if' doesn't need {}'s
This might be better handled by making sure ahead of time in
configuration that the HW doesn't do this, rather than add a test into
the fast path. As it is, between the definitions of shinfo->frags[] and
the ionic's rx sg list, I don't think this is a possible error.
As Jesse suggests, I'd like to see the test case so i can add it to our
internal testing.
Thanks,
sln
>> page_info->page = NULL;
>> page_info++;
>> i--;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-12-07 3:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-06 13:35 [PATCH 1/1] ionic: fix array overflow on receiving too many fragments for a packet Xiaohui Zhang
2020-12-07 1:51 ` Jesse Brandeburg
2020-12-07 3:41 ` Shannon Nelson
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).