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: [email protected] # v4.14+
Signed-off-by: Lee Jones <[email protected]>
---
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);
- if (ret < 0)
+ if (ret < 0) {
+ if (tb[TCA_U32_LINK])
+ n->ht_down->refcnt--;
return -EINVAL;
+ }
n->ifindex = ret;
}
return 0;
--
2.41.0.rc0.172.g3f132b7071-goog
On Wed, May 31, 2023 at 11:03 AM Eric Dumazet <[email protected]> wrote:
>
> On Wed, May 31, 2023 at 4:16 PM Lee Jones <[email protected]> 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: [email protected] # v4.14+
>
> Please add a Fixes: tag.
>
> > Signed-off-by: Lee Jones <[email protected]>
> > ---
> > 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;
> }
On Wed, May 31, 2023 at 10:16 AM Lee Jones <[email protected]> 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: [email protected] # v4.14+
> Signed-off-by: Lee Jones <[email protected]>
> ---
> 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);
> - if (ret < 0)
> + if (ret < 0) {
> + if (tb[TCA_U32_LINK])
> + n->ht_down->refcnt--;
> return -EINVAL;
> + }
> n->ifindex = ret;
> }
> return 0;
The spirit of the patch looks right I dont think this fully solves the
issue you state.
My suggestion: Move the if (tb[TCA_U32_INDEV]) above the if
(tb[TCA_U32_LINK]) {
Did you see this in practice or you found it by eyeballing the code?
Can you also add a tdc test for it? There are simple ways to create
the scenario.
cheers,
jamal
> 2.41.0.rc0.172.g3f132b7071-goog
>
On Wed, May 31, 2023 at 4:16 PM Lee Jones <[email protected]> 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: [email protected] # v4.14+
Please add a Fixes: tag.
> Signed-off-by: Lee Jones <[email protected]>
> ---
> 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;
+
return 0;
}
On Wed, 31 May 2023, Jamal Hadi Salim wrote:
> On Wed, May 31, 2023 at 11:03 AM Eric Dumazet <[email protected]> wrote:
> >
> > On Wed, May 31, 2023 at 4:16 PM Lee Jones <[email protected]> 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: [email protected] # 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: [email protected]
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 <[email protected]>
> > > ---
> > > 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 [李琼斯]
On Thu, Jun 1, 2023 at 4:06 PM Lee Jones <[email protected]> wrote:
>
> On Wed, 31 May 2023, Jamal Hadi Salim wrote:
>
> > On Wed, May 31, 2023 at 11:03 AM Eric Dumazet <[email protected]> wrote:
> > >
> > > On Wed, May 31, 2023 at 4:16 PM Lee Jones <[email protected]> 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: [email protected] # v4.14+
> > >
> > > Please add a Fixes: tag.
>
> Why?
How have you identified v4.14+ ?
Probably you did some research/"git archeology".
By adding the Fixes: tag, you allow us to double check immediately,
and see if other bugs need to be fixed at the same time.
You can also CC blamed patch authors, to get some feedback.
Otherwise, we (people reviewing this patch) have to also do this
research from scratch.
In this case, it seems bug was added in
commit 705c7091262d02b09eb686c24491de61bf42fdb2
Author: Jiri Pirko <[email protected]>
Date: Fri Aug 4 14:29:14 2017 +0200
net: sched: cls_u32: no need to call tcf_exts_change for newly
allocated struct
A nice Fixes: tag would then be
Fixes: 705c7091262d ("net: sched: cls_u32: no need to call
tcf_exts_change for newly allocated struct")
Thanks.
On Thu, 01 Jun 2023, Eric Dumazet wrote:
> On Thu, Jun 1, 2023 at 4:06 PM Lee Jones <[email protected]> wrote:
> >
> > On Wed, 31 May 2023, Jamal Hadi Salim wrote:
> >
> > > On Wed, May 31, 2023 at 11:03 AM Eric Dumazet <[email protected]> wrote:
> > > >
> > > > On Wed, May 31, 2023 at 4:16 PM Lee Jones <[email protected]> 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: [email protected] # v4.14+
> > > >
> > > > Please add a Fixes: tag.
> >
> > Why?
>
> How have you identified v4.14+ ?
>
> Probably you did some research/"git archeology".
>
> By adding the Fixes: tag, you allow us to double check immediately,
> and see if other bugs need to be fixed at the same time.
>
> You can also CC blamed patch authors, to get some feedback.
>
> Otherwise, we (people reviewing this patch) have to also do this
> research from scratch.
>
> In this case, it seems bug was added in
>
> commit 705c7091262d02b09eb686c24491de61bf42fdb2
> Author: Jiri Pirko <[email protected]>
> Date: Fri Aug 4 14:29:14 2017 +0200
>
> net: sched: cls_u32: no need to call tcf_exts_change for newly
> allocated struct
>
>
> A nice Fixes: tag would then be
>
> Fixes: 705c7091262d ("net: sched: cls_u32: no need to call
> tcf_exts_change for newly allocated struct")
Thanks for digging this out. I will add it.
--
Lee Jones [李琼斯]
On Thu, Jun 1, 2023 at 10:06 AM Lee Jones <[email protected]> wrote:
>
> On Wed, 31 May 2023, Jamal Hadi Salim wrote:
>
> > On Wed, May 31, 2023 at 11:03 AM Eric Dumazet <[email protected]> wrote:
> > >
> > > On Wed, May 31, 2023 at 4:16 PM Lee Jones <[email protected]> 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: [email protected] # 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: [email protected]
>
> 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 <[email protected]>
> > > > ---
> > > > 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 [李琼斯]