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 10:15:49 +0100
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     Xin Long <lucien.xin@...il.com>
CC:     <netdev@...r.kernel.org>, 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

On Wed, Mar 04, 2020 at 04:51:42PM +0800, 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>

Applied, thanks a lot Xin!