netdev - Re: [PATCH ipsec] esp: remove the skb from the chain when it's enqueued in cryptd

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:   Mon, 9 Mar 2020 12:07:27 +0200
From:   Mika Penttilä <mika.penttila@...tfour.com>
To:     Xin Long <lucien.xin@...il.com>, netdev@...r.kernel.org
Cc:     Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Sabrina Dubroca <sd@...asysnail.net>
Subject: Re: [PATCH ipsec] esp: remove the skb from the chain when it's
 enqueued in cryptd_wq


Hi!

On 4.3.2020 10.51, Xin Long wrote:
> Xiumei found a panic in esp offload:
>
>   BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
>   RIP: 0010:esp_output_done+0x101/0x160 [esp4]
>   Call Trace:
>    ? esp_output+0x180/0x180 [esp4]
>    cryptd_aead_crypt+0x4c/0x90
>    cryptd_queue_worker+0x6e/0xa0
>    process_one_work+0x1a7/0x3b0
>    worker_thread+0x30/0x390
>    ? create_worker+0x1a0/0x1a0
>    kthread+0x112/0x130
>    ? kthread_flush_work_fn+0x10/0x10
>    ret_from_fork+0x35/0x40
>
> It was caused by that skb secpath is used in esp_output_done() after it's
> been released elsewhere.
>
> The tx path for esp offload is:
>
>   __dev_queue_xmit()->
>     validate_xmit_skb_list()->
>       validate_xmit_xfrm()->
>         esp_xmit()->
>           esp_output_tail()->
>             aead_request_set_callback(esp_output_done) <--[1]
>             crypto_aead_encrypt()  <--[2]
>
> In [1], .callback is set, and in [2] it will trigger the worker schedule,
> later on a kernel thread will call .callback(esp_output_done), as the call
> trace shows.
>
> But in validate_xmit_xfrm():
>
>   skb_list_walk_safe(skb, skb2, nskb) {
>     ...
>     err = x->type_offload->xmit(x, skb2, esp_features);  [esp_xmit]
>     ...
>   }
>
> When the err is -EINPROGRESS, which means this skb2 will be enqueued and
> later gets encrypted and sent out by .callback later in a kernel thread,
> skb2 should be removed fromt skb chain. Otherwise, it will get processed
> again outside validate_xmit_xfrm(), which could release skb secpath, and
> cause the panic above.
>
> This patch is to remove the skb from the chain when it's enqueued in
> cryptd_wq. While at it, remove the unnecessary 'if (!skb)' check.
>
> Fixes: 3dca3f38cfb8 ("xfrm: Separate ESP handling from segmentation for GRO packets.")
> Reported-by: Xiumei Mu <xmu@...hat.com>
> Signed-off-by: Xin Long <lucien.xin@...il.com>
> ---
>  net/xfrm/xfrm_device.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index 3231ec6..e2db468 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -78,8 +78,8 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur
>  	int err;
>  	unsigned long flags;
>  	struct xfrm_state *x;
> -	struct sk_buff *skb2, *nskb;
>  	struct softnet_data *sd;
> +	struct sk_buff *skb2, *nskb, *pskb = NULL;
>  	netdev_features_t esp_features = features;
>  	struct xfrm_offload *xo = xfrm_offload(skb);
>  	struct sec_path *sp;
> @@ -168,14 +168,14 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur
>  		} else {
>  			if (skb == skb2)
>  				skb = nskb;
> -
> -			if (!skb)
> -				return NULL;
> +			else
> +				pskb->next = nskb;

pskb can be NULL on the first round?



>  			continue;
>  		}
>  
>  		skb_push(skb2, skb2->data - skb_mac_header(skb2));
> +		pskb = skb2;
>  	}
>  
>  	return skb;


Download attachment "pEpkey.asc" of type "application/pgp-keys" (3107 bytes)