Messages in this thread | | | From | Jamal Hadi Salim <> | Date | Wed, 31 May 2023 11:07:21 -0400 | Subject | Re: [PATCH 1/1] net/sched: cls_u32: Fix reference counter leak leading to overflow |
| |
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. > > > 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; > + } > 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.
cheers, jamal
> return 0; > }
| |