Re: [Intel-wired-lan] [PATCH 1/1] fix possible array overflow on receiving too many fragments for a packet

Re: [Intel-wired-lan] [PATCH 1/1] fix possible array overflow on receiving too many fragments for a packet

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2671 bytes --]

Hi Xiaohui,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tnguy-next-queue/dev-queue]
[also build test WARNING on linus/master sparc-next/master v5.10-rc7 next-20201208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Xiaohui-Zhang/fix-possible-array-overflow-on-receiving-too-many-fragments-for-a-packet/20201209-010457
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue.git dev-queue
config: riscv-allyesconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4bc4508ab14b72993c4e9a927f8e82a1dfdcf39c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Xiaohui-Zhang/fix-possible-array-overflow-on-receiving-too-many-fragments-for-a-packet/20201209-010457
        git checkout 4bc4508ab14b72993c4e9a927f8e82a1dfdcf39c
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/ip.h:16,
                    from drivers/net/ethernet/pensando/ionic/ionic_txrx.c:4:
   drivers/net/ethernet/pensando/ionic/ionic_txrx.c: In function 'ionic_rx_frags':
>> include/linux/skbuff.h:1431:53: warning: 'skb' is used uninitialized in this function [-Wuninitialized]
    1431 | #define skb_shinfo(SKB) ((struct skb_shared_info *)(skb_end_pointer(SKB)))
         |                                                     ^~~~~~~~~~~~~~~
   drivers/net/ethernet/pensando/ionic/ionic_txrx.c:76:18: note: 'skb' was declared here
      76 |  struct sk_buff *skb;
         |                  ^~~

vim +/skb +1431 include/linux/skbuff.h

4305b541357ddbd Arnaldo Carvalho de Melo 2007-04-19  1429  
^1da177e4c3f415 Linus Torvalds           2005-04-16  1430  /* Internal */
4305b541357ddbd Arnaldo Carvalho de Melo 2007-04-19 @1431  #define skb_shinfo(SKB)	((struct skb_shared_info *)(skb_end_pointer(SKB)))
^1da177e4c3f415 Linus Torvalds           2005-04-16  1432  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67660 bytes --]

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

[Shannon Nelson]

On 12/7/20 8:06 PM, 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.
> I found these two code fragments were very similar to the vulnerable code
> in CVE-2020-12465, so I submitted these two patches.
>
> Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_txrx.c        | 4 +++-
>   drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 4 +++-
>   2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> index eae75260f..f0a252208 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> @@ -821,9 +821,11 @@ ice_add_rx_frag(struct ice_ring *rx_ring, struct ice_rx_buf *rx_buf,
>   	unsigned int truesize = ice_rx_pg_size(rx_ring) / 2;
>   #endif
>   
> +	struct skb_shared_info *shinfo = skb_shinfo(skb);

This declaration should be up directly below the #endif and a blank line 
inserted before the code.

>   	if (!size)
>   		return;
> -	skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, rx_buf->page,
> +	if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags))
> +		skb_add_rx_frag(skb, shinfo, rx_buf->page,
>   			rx_buf->page_offset, size, truesize);
>   
>   	/* page is being used so we must update the page offset */
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> index 169ac4f54..d30e83a4b 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> @@ -74,6 +74,7 @@ static struct sk_buff *ionic_rx_frags(struct ionic_queue *q,
>   	struct device *dev = q->lif->ionic->dev;
>   	struct ionic_page_info *page_info;
>   	struct sk_buff *skb;
> +	struct skb_shared_info *shinfo = skb_shinfo(skb);

As the kernel test robot has suggested, this is using an uninitialized 
skb and will likely cause great unhappiness.

Also, this needs to follow the "reverse xmas tree" formatting style for 
declarations.


>   	unsigned int i;
>   	u16 frag_len;
>   	u16 len;
> @@ -102,7 +103,8 @@ 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,
> +		if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags))
> +			skb_add_rx_frag(skb, shinfo->nr_frags,
>   				page_info->page, 0, frag_len, PAGE_SIZE);

I'm still not convinced this is necessary here, and I'm still not 
thrilled with the result of just quietly dropping the fragments.

A better answer here might be to check the ARRAY_SIZE against 
comp->num_sg_elements before allocating the skb, and if too big, then 
return NULL - this gets the check done before any allocations are made, 
and the packet will be properly dropped and the drop statistic incremented.

sln

>   		page_info->page = NULL;
>   		page_info++;