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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201206175157.0000170d@intel.com>
Date:   Sun, 6 Dec 2020 17:51:57 -0800
From:   Jesse Brandeburg <jesse.brandeburg@...el.com>
To:     Xiaohui Zhang <ruc_zhangxiaohui@....com>
Cc:     Shannon Nelson <snelson@...sando.io>,
        Pensando Drivers <drivers@...sando.io>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] ionic: fix array overflow on receiving too many
 fragments for a packet

Xiaohui Zhang wrote:

> From: Zhang Xiaohui <ruc_zhangxiaohui@....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@....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--;