netdev - Re: [PATCH 1/1] net/sched: cls_u32: Fix reference counter leak leading to overflow

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

Date: Sat, 3 Jun 2023 08:35:04 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Lee Jones <lee@...nel.org>
Cc: Eric Dumazet <edumazet@...gle.com>, xiyou.wangcong@...il.com, jiri@...nulli.us, 
	davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, 
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org, stable@...nel.org
Subject: Re: [PATCH 1/1] net/sched: cls_u32: Fix reference counter leak
 leading to overflow

On Thu, Jun 1, 2023 at 10:06 AM Lee Jones <lee@...nel.org> wrote:
>
> On Wed, 31 May 2023, Jamal Hadi Salim wrote:
>
> > On Wed, May 31, 2023 at 11:03 AM Eric Dumazet <edumazet@...gle.com> wrote:
> > >
> > > On Wed, May 31, 2023 at 4:16 PM Lee Jones <lee@...nel.org> wrote:
> > > >
> > > > In the event of a failure in tcf_change_indev(), u32_set_parms() will
> > > > immediately return without decrementing the recently incremented
> > > > reference counter.  If this happens enough times, the counter will
> > > > rollover and the reference freed, leading to a double free which can be
> > > > used to do 'bad things'.
> > > >
> > > > Cc: stable@...nel.org # v4.14+
> > >
> > > Please add a Fixes: tag.
>
> Why?
>
> From memory, I couldn't identify a specific commit to fix, which is why
> I used a Cc tag as per the Stable documentation:
>
> Option 1
> ********
>
> To have the patch automatically included in the stable tree, add the tag
>
> .. code-block:: none
>
>      Cc: stable@...r.kernel.org
>
> in the sign-off area. Once the patch is merged it will be applied to
> the stable tree without anything else needing to be done by the author
> or subsystem maintainer.
>
> > > > Signed-off-by: Lee Jones <lee@...nel.org>
> > > > ---
> > > >  net/sched/cls_u32.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> > > > index 4e2e269f121f8..fad61ca5e90bf 100644
> > > > --- a/net/sched/cls_u32.c
> > > > +++ b/net/sched/cls_u32.c
> > > > @@ -762,8 +762,11 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
> > > >         if (tb[TCA_U32_INDEV]) {
> > > >                 int ret;
> > > >                 ret = tcf_change_indev(net, tb[TCA_U32_INDEV], extack);
> > >
> > > This call should probably be done earlier in the function, next to
> > > tcf_exts_validate_ex()
> > >
> > > Otherwise we might ask why the tcf_bind_filter() does not need to be undone.
> > >
> > > Something like:
> > >
> > > diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> > > index 4e2e269f121f8a301368b9783753e055f5af6a4e..ac957ff2216ae18bcabdd3af3b0e127447ef8f91
> > > 100644
> > > --- a/net/sched/cls_u32.c
> > > +++ b/net/sched/cls_u32.c
> > > @@ -718,13 +718,18 @@ static int u32_set_parms(struct net *net, struct
> > > tcf_proto *tp,
> > >                          struct nlattr *est, u32 flags, u32 fl_flags,
> > >                          struct netlink_ext_ack *extack)
> > >  {
> > > -       int err;
> > > +       int err, ifindex = -1;
> > >
> > >         err = tcf_exts_validate_ex(net, tp, tb, est, &n->exts, flags,
> > >                                    fl_flags, extack);
> > >         if (err < 0)
> > >                 return err;
> > >
> > > +       if (tb[TCA_U32_INDEV]) {
> > > +               ifindex = tcf_change_indev(net, tb[TCA_U32_INDEV], extack);
> > > +               if (ifindex < 0)
> > > +                       return -EINVAL;
> > > +       }
>
> Thanks for the advice.  Leave it with me.
>
> > >         if (tb[TCA_U32_LINK]) {
> > >                 u32 handle = nla_get_u32(tb[TCA_U32_LINK]);
> > >                 struct tc_u_hnode *ht_down = NULL, *ht_old;
> > > @@ -759,13 +764,9 @@ static int u32_set_parms(struct net *net, struct
> > > tcf_proto *tp,
> > >                 tcf_bind_filter(tp, &n->res, base);
> > >         }
> > >
> > > -       if (tb[TCA_U32_INDEV]) {
> > > -               int ret;
> > > -               ret = tcf_change_indev(net, tb[TCA_U32_INDEV], extack);
> > > -               if (ret < 0)
> > > -                       return -EINVAL;
> > > -               n->ifindex = ret;
> > > -       }
> > > +       if (ifindex >= 0)
> > > +               n->ifindex = ifindex;
> > > +
> >
> > I guess we crossed paths ;->
>
> > Please, add a tdc test as well - it doesnt have to be in this patch,
> > can be a followup.
>
> I don't know how to do that, or even what a 'tdc' is.  Is it trivial?
>
> Can you point me towards the documentation please?

There is a README in tools/testing/selftests/tc-testing/README
If you created the scenario by running some tc command line it should
not be difficult to create such a test. Or just tell us what command
line you used to create it and we can help do one for you this time.
If you found the issue by just eyeballing the code or syzkaller then
just say that in the commit.

cheers,
jamal

> --
> Lee Jones [李琼斯]