| 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
| ||
|
Message-ID: <bf6591ac-2526-6ca8-b60b-70536a31ae2a@redhat.com>
Date: Fri, 28 Apr 2023 10:20:44 +0200
From: Ivan Vecera <ivecera@...hat.com>
To: Simon Horman <simon.horman@...igine.com>
Cc: Vlad Buslov <vladbu@...dia.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 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?
Ivan
Powered by blists - more mailing lists