Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Wed, 20 Sep 2023 10:29:34 -0700
From:   Stephen Hemminger <stephen@networkplumber.org>
To:     Kees Cook <keescook@chromium.org>
Cc:     kernel test robot <lkp@intel.com>,
        Mirko Lindner <mlindner@marvell.com>,
        oe-kbuild-all@lists.linux.dev, linux-kernel@vger.kernel.org,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        linux-hardening@vger.kernel.org
Subject: Re: include/linux/dma-mapping.h:416:36: warning: array subscript i
 is outside array bounds of 'dma_addr_t[0]' {aka 'long long unsigned int[]'}
Message-ID: <20230920102934.595b755f@hermes.local>
In-Reply-To: <202309200904.CEED70DA33@keescook>
References: <202309191958.UBw1cjXk-lkp@intel.com>
        <202309200904.CEED70DA33@keescook>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 20 Sep 2023 09:09:33 -0700
Kees Cook <keescook@chromium.org> wrote:

> On Tue, Sep 19, 2023 at 07:27:26PM +0800, kernel test robot wrote:
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head:   2cf0f715623872823a72e451243bbf555d10d032
> > commit: df8fc4e934c12b906d08050d7779f292b9c5c6b5 kbuild: Enable -fstrict-flex-arrays=3
> > date:   4 months ago
> > config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20230919/202309191958.UBw1cjXk-lkp@intel.com/config)
> > compiler: loongarch64-linux-gcc (GCC) 13.2.0
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230919/202309191958.UBw1cjXk-lkp@intel.com/reproduce)
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202309191958.UBw1cjXk-lkp@intel.com/
> > 
> > All warnings (new ones prefixed by >>):
> > 
> >    In file included from include/linux/skbuff.h:28,
> >                     from include/net/net_namespace.h:43,
> >                     from include/linux/netdevice.h:38,
> >                     from drivers/net/ethernet/marvell/sky2.c:18:
> >    drivers/net/ethernet/marvell/sky2.c: In function 'sky2_rx_unmap_skb':  
> > >> include/linux/dma-mapping.h:416:36: warning: array subscript i is outside array bounds of 'dma_addr_t[0]' {aka 'long long unsigned int[]'} [-Warray-bounds=]  
> >      416 | #define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0)
> >          |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    drivers/net/ethernet/marvell/sky2.c:1257:17: note: in expansion of macro 'dma_unmap_page'
> >     1257 |                 dma_unmap_page(&pdev->dev, re->frag_addr[i],
> >          |                 ^~~~~~~~~~~~~~
> >    In file included from drivers/net/ethernet/marvell/sky2.c:41:
> >    drivers/net/ethernet/marvell/sky2.h:2198:25: note: while referencing 'frag_addr'
> >     2198 |         dma_addr_t      frag_addr[ETH_JUMBO_MTU >> PAGE_SHIFT];
> >          |                         ^~~~~~~~~  
> 
> The .config has:
> CONFIG_PAGE_SIZE_16KB=y
> which makes PAGE_SHIFT == 14
> 
> #ifdef CONFIG_PAGE_SIZE_16KB
> #define PAGE_SHIFT      14
> 
> ETH_JUMBO_MTU is:
> 
> #define ETH_JUMBO_MTU	9000
> 
> which forces "ETH_JUMBO_MTU >> PAGE_SHIFT" to be 0.
> 
> I think the right fix would be:
> 
> dma_addr_t      frag_addr[ETH_JUMBO_MTU >> PAGE_SHIFT ?: 1]
> 
> Thoughts?
> 
> -Kees
> 

This is old driver, I don't have the HW anymore, it went to Free Geek.
Most of this code was based off of code in other drivers.

The assumption is that the first part of the data will be received in the
skb itself, then pages are used for overflow.

static unsigned sky2_get_rx_data_size(struct sky2_port *sky2)
{
	struct rx_ring_info *re;
	unsigned size;

	/* Space needed for frame data + headers rounded up */
	size = roundup(sky2->netdev->mtu + ETH_HLEN + VLAN_HLEN, 8);

	sky2->rx_nfrags = size >> PAGE_SHIFT;
	BUG_ON(sky2->rx_nfrags > ARRAY_SIZE(re->frag_addr));

Assuming PAGE_SIZE of 16k and MTU of 9000.

	size = roundup(9000 + 14 + 4, 8) => 9024
	sky2->rx_nfrags = 9024 >> 14 = 0

Which means no skb frags will be used.

This is probably suboptimal since it will endup calling alloc_skb()
to get a 9024 skb. Which in turn causes a call to kmalloc() of 9024.

Not really worth fixing if not testable.