Messages in this thread Patch in this message | | | Date | Thu, 1 Jun 2023 15:06:40 +0100 | From | Lee Jones <> | Subject | Re: [PATCH 1/1] net/sched: cls_u32: Fix reference counter leak leading to overflow |
| |
On Wed, 31 May 2023, Jamal Hadi Salim wrote:
> On Wed, May 31, 2023 at 11:03 AM Eric Dumazet <edumazet@google.com> wrote: > > > > On Wed, May 31, 2023 at 4:16 PM Lee Jones <lee@kernel.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@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, 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?
-- Lee Jones [李琼斯]
| |