| 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: <ZEtxvPaa/L3jHa2d@corigine.com>
Date: Fri, 28 Apr 2023 09:11:56 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Ivan Vecera <ivecera@...hat.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
Subject: Re: [PATCH net 2/2] net/sched: flower: fix error handler on replace
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?
>
Powered by blists - more mailing lists