[PATCH ipsec] esp: remove the skb from the chain when it's enqueued in cryptd_wq

[PATCH ipsec] esp: remove the skb from the chain when it's enqueued in cryptd_wq

[Xin Long]

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@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.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;
 
 			continue;
 		}
 
 		skb_push(skb2, skb2->data - skb_mac_header(skb2));
+		pskb = skb2;
 	}
 
 	return skb;
-- 
2.1.0

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

[Steffen Klassert]

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@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Applied, thanks a lot Xin!

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

[Mika Penttilä]

[-- Attachment #1: Type: text/plain, Size: 3062 bytes --]


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@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.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;


[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 3157 bytes --]

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

[Xin Long]

On Mon, Mar 9, 2020 at 6:07 PM Mika Penttilä <mika.penttila@nextfour.com> wrote:
>
>
> 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@redhat.com>
> > Signed-off-by: Xin Long <lucien.xin@gmail.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?
On the 1st round, skb == skb2.

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

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

[Mika Penttilä]

[-- Attachment #1: Type: text/plain, Size: 3683 bytes --]



On 9.3.2020 12.50, Xin Long wrote:
> On Mon, Mar 9, 2020 at 6:07 PM Mika Penttilä <mika.penttila@nextfour.com> wrote:
>>
>> 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@redhat.com>
>>> Signed-off-by: Xin Long <lucien.xin@gmail.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?
> On the 1st round, skb == skb2.

Yes, I misread the patch, thanks.


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


[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 3157 bytes --]