[PATCH 1/1] ionic: fix array overflow on receiving too many fragments for a packet

[PATCH 1/1] ionic: fix array overflow on receiving too many fragments for a packet

[Xiaohui Zhang]

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

Re: [PATCH 1/1] ionic: fix array overflow on receiving too many fragments for a packet

[Jesse Brandeburg]

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--;

Re: [PATCH 1/1] ionic: fix array overflow on receiving too many fragments for a packet

[Shannon Nelson]

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--;
>