On Thu, Jun 1, 2023 at 10:06 AM Lee Jones [off-list ref] wrote:

On Wed, 31 May 2023, Jamal Hadi Salim wrote:

quoted

On Wed, May 31, 2023 at 11:03 AM Eric Dumazet [off-list ref] wrote:

quoted

On Wed, May 31, 2023 at 4:16 PM Lee Jones [off-list ref] wrote:

quoted

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.

quoted

quoted

quoted

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.

quoted

quoted

        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 ;->

quoted

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 [李琼斯]