linux-hardening - Re: [PATCH] bnx2: Pass allocation size to build

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] [day] [month] [year] [list]

Message-ID: <20221021204412.4ee726c8@kernel.org>
Date:   Fri, 21 Oct 2022 20:44:12 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     Rasesh Mody <rmody@...vell.com>, GR-Linux-NIC-Dev@...vell.com,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] bnx2: Pass allocation size to build_skb()

On Fri, 21 Oct 2022 19:06:26 -0700 Kees Cook wrote:
> On Wed, Oct 19, 2022 at 05:02:55PM -0700, Jakub Kicinski wrote:
> > On Tue, 18 Oct 2022 01:59:29 -0700 Kees Cook wrote:  
> > > In preparation for requiring that build_skb() have a non-zero size
> > > argument, pass the actual data allocation size explicitly into
> > > build_skb().  
> > 
> > build_skb(, 0) has the special meaning of "head buf has been kmalloc'd",
> > rather than alloc_page(). Was this changed and I missed it?  
> 
> Hm, I'm not clear on it. I see ksize() being called, but I guess that
> works for alloc_page() allocations too?
> 
> build_skb
> 	__build_skb:
> 		__build_skb_around:
> 		        unsigned int size = frag_size ? : ksize(data);

Hm, what I'm saying is the definition of the frag_size is - the size of
the frag if page-backed, or 0 if kmalloc-backed.

So the ternary op above applies ksize only in the kmalloc case.

> So I guess in this case, this patch is wrong, and should instead be this
> to match the ksize() used in build_skb():
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2.c
> b/drivers/net/ethernet/broadcom/bnx2.c
> index fec57f1982c8..dbe310144780 100644
> --- a/drivers/net/ethernet/broadcom/bnx2.c
> +++ b/drivers/net/ethernet/broadcom/bnx2.c
> @@ -5415,8 +5415,9 @@ bnx2_set_rx_ring_size(struct bnx2 *bp, u32 size)
>  
>         bp->rx_buf_use_size = rx_size;
>         /* hw alignment + build_skb() overhead*/
> -       bp->rx_buf_size = SKB_DATA_ALIGN(bp->rx_buf_use_size + BNX2_RX_ALIGN) +
> -               NET_SKB_PAD + SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +       bp->rx_buf_size = kmalloc_size_roundup(
> +               SKB_DATA_ALIGN(bp->rx_buf_use_size + BNX2_RX_ALIGN) +
> +               NET_SKB_PAD + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
>         bp->rx_jumbo_thresh = rx_size - BNX2_RX_OFFSET;
>         bp->rx_ring_size = size;
>         bp->rx_max_ring = bnx2_find_max_ring(size, BNX2_MAX_RX_RINGS);

IIUC you want the size of the allocation to match exactly to the result
of ksize()?  In that case - yup, the above looks good.

FWIW the kmalloc backed heads are actually a performance bottleneck 
so we'd be doing everyone a favor if we just converted the two drivers
which do this to use pages and killed the "feature".

But the roundup works well enough.