| 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
| ||
|
Date: Fri, 28 Apr 2023 14:03:19 +0300
From: Vlad Buslov <vladbu@...dia.com>
To: Ivan Vecera <ivecera@...hat.com>
CC: Simon Horman <simon.horman@...igine.com>,
Pedro Tammela <pctammela@...atatu.com>, <davem@...emloft.net>,
<kuba@...nel.org>, <netdev@...r.kernel.org>, <jhs@...atatu.com>,
<xiyou.wangcong@...il.com>, <jiri@...nulli.us>,
<marcelo.leitner@...il.com>, <paulb@...dia.com>,
"Paolo Abeni" <pabeni@...hat.com>
Subject: Re: [PATCH net 2/2] net/sched: flower: fix error handler on replace
On Fri 28 Apr 2023 at 10:20, Ivan Vecera <ivecera@...hat.com> wrote:
> On 28. 04. 23 9:11, Simon Horman wrote:
>> On Wed, Apr 26, 2023 at 05:39:09PM +0200, Ivan Vecera wrote:
>>>
>>>
>>> On 26. 04. 23 16:46, Vlad Buslov wrote:
>>>> On Wed 26 Apr 2023 at 11:22, Pedro Tammela <pctammela@...atatu.com> wrote:
>>>>> On 26/04/2023 09:14, Vlad Buslov wrote:
>>>>>> When replacing a filter (i.e. 'fold' pointer is not NULL) the insertion of
>>>>>> new filter to idr is postponed until later in code since handle is already
>>>>>> provided by the user. However, the error handling code in fl_change()
>>>>>> always assumes that the new filter had been inserted into idr. If error
>>>>>> handler is reached when replacing existing filter it may remove it from idr
>>>>>> therefore making it unreachable for delete or dump afterwards. Fix the
>>>>>> issue by verifying that 'fold' argument wasn't provided by caller before
>>>>>> calling idr_remove().
>>>>>> Fixes: 08a0063df3ae ("net/sched: flower: Move filter handle initialization
>>>>>> earlier")
>>>>>> Signed-off-by: Vlad Buslov <vladbu@...dia.com>
>>>>>> ---
>>>>>> net/sched/cls_flower.c | 3 ++-
>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>>>>>> index 1844545bef37..a1c4ee2e0be2 100644
>>>>>> --- a/net/sched/cls_flower.c
>>>>>> +++ b/net/sched/cls_flower.c
>>>>>> @@ -2339,7 +2339,8 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
>>>>>> errout_mask:
>>>>>> fl_mask_put(head, fnew->mask);
>>>>>> errout_idr:
>>>>>> - idr_remove(&head->handle_idr, fnew->handle);
>>>>>> + if (!fold)
>>>>>> + idr_remove(&head->handle_idr, fnew->handle);
>>>>>> __fl_put(fnew);
>>>>>> errout_tb:
>>>>>> kfree(tb);
>>>>>
>>>>> Actually this seems to be fixing the same issue:
>>>>> https://lore.kernel.org/all/20230425140604.169881-1-ivecera@redhat.com/
>>>>
>>>> Indeed it does, I've missed that patch. However, it seems there
>>>> is an issue with Ivan's approach. Consider what would happen when
>>>> fold!=NULL && in_ht==false and rhashtable_insert_fast() fails here:
>>>>
>>>>
>>>> if (fold) {
>>>> /* Fold filter was deleted concurrently. Retry lookup. */
>>>> if (fold->deleted) {
>>>> err = -EAGAIN;
>>>> goto errout_hw;
>>>> }
>>>>
>>>> fnew->handle = handle; // <-- fnew->handle is assigned
>>>>
>>>> if (!in_ht) {
>>>> struct rhashtable_params params =
>>>> fnew->mask->filter_ht_params;
>>>>
>>>> err = rhashtable_insert_fast(&fnew->mask->ht,
>>>> &fnew->ht_node,
>>>> params);
>>>> if (err)
>>>> goto errout_hw; /* <-- err is set, go to
>>>> error handler here */
>>>> in_ht = true;
>>>> }
>>>>
>>>> refcount_inc(&fnew->refcnt);
>>>> rhashtable_remove_fast(&fold->mask->ht,
>>>> &fold->ht_node,
>>>> fold->mask->filter_ht_params);
>>>> /* !!! we never get to insert fnew into idr here, if ht insertion fails */
>>>> idr_replace(&head->handle_idr, fnew, fnew->handle);
>>>> list_replace_rcu(&fold->list, &fnew->list);
>>>> fold->deleted = true;
>>>>
>>>> spin_unlock(&tp->lock);
>>>>
>>>> fl_mask_put(head, fold->mask);
>>>> if (!tc_skip_hw(fold->flags))
>>>> fl_hw_destroy_filter(tp, fold, rtnl_held, NULL);
>>>> tcf_unbind_filter(tp, &fold->res);
>>>> /* Caller holds reference to fold, so refcnt is always > 0
>>>> * after this.
>>>> */
>>>> refcount_dec(&fold->refcnt);
>>>> __fl_put(fold);
>>>> }
>>>>
>>>> ...
>>>>
>>>> errout_ht:
>>>> spin_lock(&tp->lock);
>>>> errout_hw:
>>>> fnew->deleted = true;
>>>> spin_unlock(&tp->lock);
>>>> if (!tc_skip_hw(fnew->flags))
>>>> fl_hw_destroy_filter(tp, fnew, rtnl_held, NULL);
>>>> if (in_ht)
>>>> rhashtable_remove_fast(&fnew->mask->ht, &fnew->ht_node,
>>>> fnew->mask->filter_ht_params);
>>>> errout_mask:
>>>> fl_mask_put(head, fnew->mask);
>>>> errout_idr:
>>>> /* !!! On next line we remove handle that we don't actually own */
>>>> idr_remove(&head->handle_idr, fnew->handle);
>>>> __fl_put(fnew);
>>>> errout_tb:
>>>> kfree(tb);
>>>> errout_mask_alloc:
>>>> tcf_queue_work(&mask->rwork, fl_uninit_mask_free_work);
>>>> errout_fold:
>>>> if (fold)
>>>> __fl_put(fold);
>>>> return err;
>>>>
>>>>
>>>> Also, if I understood the idea behind Ivan's fix correctly, it relies on
>>>> the fact that calling idr_remove() with handle==0 is a noop. I prefer my
>>>> approach slightly better as it is more explicit IMO.
>>>>
>>>> Thoughts?
>>>
>>> Yes, your approach is better...
>>>
>>> Acked-by: Ivan Vecera <ivecera@...hat.com>
>> In the meantime it seems that Ivan's patch has been accepted into net.
>> - [net] net/sched: flower: Fix wrong handle assignment during filter change
>> https://git.kernel.org/netdev/net/c/32eff6bacec2
>> Is some adjustment to this patch required to take that into account?
>
> I think something like this is necessary to cover Vlad's findings:
>
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index 6ab6aadc07b8da..ce937baefcf00e 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -2279,8 +2279,6 @@ static int fl_change(struct net *net, struct sk_buff
> *in_skb,
> goto errout_hw;
> }
>
> - fnew->handle = handle;
> -
> if (!in_ht) {
> struct rhashtable_params params =
> fnew->mask->filter_ht_params;
> @@ -2297,6 +2295,7 @@ static int fl_change(struct net *net, struct sk_buff
> *in_skb,
> rhashtable_remove_fast(&fold->mask->ht,
> &fold->ht_node,
> fold->mask->filter_ht_params);
> + fnew->handle = handle;
> idr_replace(&head->handle_idr, fnew, fnew->handle);
> list_replace_rcu(&fold->list, &fnew->list);
> fold->deleted = true;
>
> Just move fnew->handle assignment immediately prior idr_replace().
>
> Thoughts?
Note that with these changes (both accepted patch and preceding diff)
you are exposing filter to dapapath access (datapath looks up filter via
hash table, not idr) with its handle set to 0 initially and then resent
while already accessible. After taking a quick look at Paul's
miss-to-action code it seems that handle value used by datapath is taken
from struct tcf_exts_miss_cookie_node not from filter directly, so such
approach likely doesn't break anything existing, but I might have missed
something.
Powered by blists - more mailing lists