Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230531141556.1637341-1-lee@kernel.org> <CANn89iJw2N9EbF+Fm8KCPMvo-25ONwba+3PUr8L2ktZC1Z3uLw@mail.gmail.com>
 <CAM0EoMnUgXsr4UBeZR57vPpc5WRJkbWUFsii90jXJ=stoXCGcg@mail.gmail.com> <20230601140640.GG449117@google.com>
In-Reply-To: <20230601140640.GG449117@google.com>
From:   Jamal Hadi Salim <jhs@mojatatu.com>
Date:   Sat, 3 Jun 2023 08:35:04 -0400
Message-ID: <CAM0EoMmXFKNMkEFhnLReMO=jwu8ju8udV6-oZO9-yHL_7ocUYw@mail.gmail.com>
Subject: Re: [PATCH 1/1] net/sched: cls_u32: Fix reference counter leak
 leading to overflow
To:     Lee Jones <lee@kernel.org>
Cc:     Eric Dumazet <edumazet@google.com>, xiyou.wangcong@gmail.com,
        jiri@resnulli.us, davem@davemloft.net, kuba@kernel.org,
        pabeni@redhat.com, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, stable@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Jun 1, 2023 at 10:06=E2=80=AFAM Lee Jones <lee@kernel.org> wrote:
>
> On Wed, 31 May 2023, Jamal Hadi Salim wrote:
>
> > On Wed, May 31, 2023 at 11:03=E2=80=AFAM Eric Dumazet <edumazet@google.=
com> wrote:
> > >
> > > On Wed, May 31, 2023 at 4:16=E2=80=AFPM Lee Jones <lee@kernel.org> wr=
ote:
> > > >
> > > > In the event of a failure in tcf_change_indev(), u32_set_parms() wi=
ll
> > > > 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 ca=
n be
> > > > used to do 'bad things'.
> > > >
> > > > Cc: stable@kernel.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@vger.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@kernel.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, stru=
ct tcf_proto *tp,
> > > >         if (tb[TCA_U32_INDEV]) {
> > > >                 int ret;
> > > >                 ret =3D tcf_change_indev(net, tb[TCA_U32_INDEV], ex=
tack);
> > >
> > > 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..ac957ff2216ae18bcabdd=
3af3b0e127447ef8f91
> > > 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, struc=
t
> > > tcf_proto *tp,
> > >                          struct nlattr *est, u32 flags, u32 fl_flags,
> > >                          struct netlink_ext_ack *extack)
> > >  {
> > > -       int err;
> > > +       int err, ifindex =3D -1;
> > >
> > >         err =3D tcf_exts_validate_ex(net, tp, tb, est, &n->exts, flag=
s,
> > >                                    fl_flags, extack);
> > >         if (err < 0)
> > >                 return err;
> > >
> > > +       if (tb[TCA_U32_INDEV]) {
> > > +               ifindex =3D 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 =3D nla_get_u32(tb[TCA_U32_LINK]);
> > >                 struct tc_u_hnode *ht_down =3D 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 =3D tcf_change_indev(net, tb[TCA_U32_INDEV], exta=
ck);
> > > -               if (ret < 0)
> > > -                       return -EINVAL;
> > > -               n->ifindex =3D ret;
> > > -       }
> > > +       if (ifindex >=3D 0)
> > > +               n->ifindex =3D 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 [=E6=9D=8E=E7=90=BC=E6=96=AF]