netdev - RE: [PATCH net v3] skbuff: fix coalescing for page

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]

Date:   Thu, 31 Mar 2022 18:44:22 +0000
From:   Alexander Duyck <alexanderduyck@...com>
To:     Jean-Philippe Brucker <jean-philippe@...aro.org>,
        "ilias.apalodimas@...aro.org" <ilias.apalodimas@...aro.org>,
        "linyunsheng@...wei.com" <linyunsheng@...wei.com>
CC:     "hawk@...nel.org" <hawk@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH net v3] skbuff: fix coalescing for page_pool fragment
 recycling

> -----Original Message-----
> From: Jean-Philippe Brucker <jean-philippe@...aro.org>
> Sent: Thursday, March 31, 2022 3:25 AM
> To: ilias.apalodimas@...aro.org; Alexander Duyck
> <alexanderduyck@...com>; linyunsheng@...wei.com
> Cc: hawk@...nel.org; davem@...emloft.net; kuba@...nel.org;
> pabeni@...hat.com; netdev@...r.kernel.org; Jean-Philippe Brucker <jean-
> philippe@...aro.org>
> Subject: [PATCH net v3] skbuff: fix coalescing for page_pool fragment
> recycling
> 
> Fix a use-after-free when using page_pool with page fragments. We
> encountered this problem during normal RX in the hns3 driver:

<snip>

> ---
>  net/core/skbuff.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c index
> ea51e23e9247..2d6ef6d7ebf5 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5244,11 +5244,18 @@ bool skb_try_coalesce(struct sk_buff *to, struct
> sk_buff *from,
>  	if (skb_cloned(to))
>  		return false;
> 
> -	/* The page pool signature of struct page will eventually figure out
> -	 * which pages can be recycled or not but for now let's prohibit slab
> -	 * allocated and page_pool allocated SKBs from being coalesced.
> +	/* In general, avoid mixing slab allocated and page_pool allocated
> +	 * pages within the same SKB. However when @to is not pp_recycle
> and
> +	 * @from is cloned, we can transition frag pages from page_pool to
> +	 * reference counted.
> +	 *
> +	 * On the other hand, don't allow coalescing two pp_recycle SKBs if
> +	 * @from is cloned, in case the SKB is using page_pool fragment
> +	 * references (PP_FLAG_PAGE_FRAG). Since we only take full page
> +	 * references for cloned SKBs at the moment that would result in
> +	 * inconsistent reference counts.
>  	 */
> -	if (to->pp_recycle != from->pp_recycle)
> +	if (to->pp_recycle != (from->pp_recycle && !skb_cloned(from)))
>  		return false;
> 
>  	if (len <= skb_tailroom(to)) {
> --
> 2.25.1

This looks good to me. The impact should be minimal since it only applies to pp_recycle pages.

Reviewed-by: Alexander Duyck <alexanderduyck@...com>