netdev - Re: [PATCH net-next 2/3] xfrm: Fix offload dev state addition to occur after

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

Message-ID: <a11f0ebc-165c-8adf-7531-18672d0e5ab2@avivh.com>
Date:   Tue, 28 Nov 2017 20:17:38 +0200
From:   Aviv Heller <aviv@...vh.com>
To:     Steffen Klassert <steffen.klassert@...unet.com>
Cc:     netdev-owner@...r.kernel.org, netdev@...r.kernel.org,
        Boris Pismenny <borisp@...lanox.com>,
        Yossi Kuperman <yossiku@...lanox.com>,
        Yevgeny Kliteynik <kliteyn@...lanox.com>,
        Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: [PATCH net-next 2/3] xfrm: Fix offload dev state addition to
 occur after

+ mailing list again.. (due to email client problem, sorry for the 
inconvenience).

On 11/28/2017 8:06 PM, Aviv Heller wrote:
> + mailing list
> 
> On 11/28/2017 7:55 PM, Aviv Heller wrote:
>>> On Wed, Oct 25, 2017 at 01:09:44PM +0000, Aviv Heller wrote:
>>>> -----Original message-----
>>>>> From: Steffen Klassert
>>>>> Sent: Wednesday, October 25 2017, 10:22 am
>>>>> To: avivh@...lanox.com
>>>>> Cc: Herbert Xu; Boris Pismenny; Yossi Kuperman; Yevgeny Kliteynik;
>>>>> netdev@...r.kernel.org
>>>>> Subject: Re: [PATCH net-next 2/3] xfrm: Fix offload dev state
>>>>> addition to occur after insertion
>>>>>
>>>>> On Tue, Oct 24, 2017 at 06:10:30PM +0300, avivh@...lanox.com wrote:
>>>>>> From: Aviv Heller <avivh@...lanox.com>
>>>>>>
>>>>>> Adding the state to the offload device prior to replay init in
>>>>>> xfrm_state_construct() will result in NULL dereference if a
>>>>>> matching ESP packet is received in between.
>>>>>>
>>>>>> Adding it after insertion also has the benefit of the driver not
>>>>>> having to check whether a state with the same match criteria
>>>>>> already exists, but forces us to use an atomic type for the
>>>>>> offload_handle, to make certain a stack-read/driver-write race
>>>>>> won't result in reading corrupt data.
>>>>>
>>>>> No, this will add multiple atomic operations to the packet path,
>>>>> even in the non offloaded case.
>>>>>
>>>>> I think the problem is that we set XFRM_STATE_VALID to early.
>>>>> This was not a problem before we had offloading because it was not
>>>>> possible to lookup this state before we inserted it into the SADB.
>>>>> Now that the driver holds a subset of states too, we need to make
>>>>> sure the state is fully initialized before we mark it as valid.
>>>>>
>>>>> The patch below should do it, in combination with your patch 1/3.
>>>>>
>>>>> Could you please test this?
>>>>>
>>>>> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index
>>>>> b997f13..96eb263 100644
>>>>> --- a/net/xfrm/xfrm_user.c
>>>>> +++ b/net/xfrm/xfrm_user.c
>>>>> @@ -587,10 +587,6 @@ static struct xfrm_state 
>>>>> *xfrm_state_construct(struct net *net,
>>>>>       if (attrs[XFRMA_OUTPUT_MARK])
>>>>>           x->props.output_mark = 
>>>>> nla_get_u32(attrs[XFRMA_OUTPUT_MARK]);
>>>>> -    err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV]);
>>>>> -    if (err)
>>>>> -        goto error;
>>>>> -
>>>>>       if (attrs[XFRMA_SEC_CTX]) {
>>>>>           err = security_xfrm_state_alloc(x,
>>>>>                           nla_data(attrs[XFRMA_SEC_CTX])); @@ 
>>>>> -620,6 +616,10
>>> @@ static
>>>>> struct xfrm_state *xfrm_state_construct(struct net *net,
>>>>>       /* override default values from above */
>>>>>       xfrm_update_ae_params(x, attrs, 0);
>>>>> +    err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV]);
>>>>> +    if (err)
>>>>> +        goto error;
>>>>> +
>>>>>       return x;
>>>>>   error:
>>>>>
>>>>
>>>> Hi Steffen,
>>>>
>>>> This patch does not work, due to:
>>>>     if (!x->type_offload)
>>>>         return -EINVAL;
>>>>
>>>> test in xfrm_dev_state_add().
>>>
>>> There is certainly a way arround that :) The easiest I can think of 
>>> would be to propagate
>>> XFRM_STATE_VALID only after the state is inserted into the SADBs. 
>>> I.e. move the setting of
>>> XFRM_STATE_VALID out of __xfrm_init_state() and let the callers do it.
>>>
>>>>
>>>> I agree with your analysis, and that we take a little performance 
>>>> hit due to the atomics, but we get the
>>> benefit of calling xfrm_dev_state_add() after the state is completely 
>>> initialized, and passed the criteria
>>> for addition by xfrm_state_add().
>>>
>>> We already have too many of these atomic operatons in the packet 
>>> path, adding more is a no go.
>>>
>>
>> Okay, I'm submitting v2 with a solution proposed by Yossi Kuperman, 
>> which involves adding an activate() op to notify the driver that the 
>> state object is ready and added to kernel SADBs.
>>
>> This way we can avoid the offload_handle problem, while not modifying 
>> km.state after insertion (which looks safer to me).
>>
>> What do you think?
>>
>> Thanks,
>> Aviv
>