netdev - Re: [PATCH net 1/2] net/sched: act_mirred: Fix leak when redirecting to self on egress

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAM0EoMmDfrgH7wbguUf+T+mos2ehnm4cnwCGxY4+tZ79_7zB8A@mail.gmail.com>
Date: Tue, 30 Dec 2025 16:32:23 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, horms@...nel.org, andrew+netdev@...n.ch
Cc: netdev@...r.kernel.org, xiyou.wangcong@...il.com, jiri@...nulli.us, 
	victor@...atatu.com
Subject: Re: [PATCH net 1/2] net/sched: act_mirred: Fix leak when redirecting
 to self on egress

On Tue, Dec 30, 2025 at 2:18 PM Jamal Hadi Salim <jhs@...atatu.com> wrote:
>
> Whenever a mirred redirect to self on egress happens, mirred allocates a
> new skb (skb_to_send). The loop to self check was done after that
> allocation, but was not freeing the newly allocated skb, causing a leak.
>
> Fix this by moving the if-statement to before the allocation of the new
> skb.
>
> The issue was found by running the accompanying tdc test in 2/2
> with config kmemleak enabled.
> After a few minutes the kmemleak thread ran and reported the leak coming from
> mirred.
>

Grr. There is a bug in the print - i will send a new version tomorrow.

cheers,
jamal

> Fixes: 1d856251a009 ("net/sched: act_mirred: fix loop detection")
> Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>
> ---
>  net/sched/act_mirred.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index 91c96cc625bd..c9653b76a4cf 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -266,6 +266,17 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
>                 goto err_cant_do;
>         }
>
> +       want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
> +
> +       if (dev == skb->dev && want_ingress == at_ingress) {
> +               pr_notice_once("tc mirred: Loop (%s:%s --> %s:%s)\n",
> +                              netdev_name(skb->dev),
> +                              at_ingress ? "ingress" : "egress",
> +                              netdev_name(dev),
> +                              want_ingress ? "ingress" : "egress");
> +               goto err_cant_do;
> +       }
> +
>         /* we could easily avoid the clone only if called by ingress and clsact;
>          * since we can't easily detect the clsact caller, skip clone only for
>          * ingress - that covers the TC S/W datapath.
> @@ -279,17 +290,6 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
>                         goto err_cant_do;
>         }
>
> -       want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
> -
> -       if (dev == skb->dev && want_ingress == at_ingress) {
> -               pr_notice_once("tc mirred: Loop (%s:%s --> %s:%s)\n",
> -                              netdev_name(skb->dev),
> -                              at_ingress ? "ingress" : "egress",
> -                              netdev_name(dev),
> -                              want_ingress ? "ingress" : "egress");
> -               goto err_cant_do;
> -       }
> -
>         /* All mirred/redirected skbs should clear previous ct info */
>         nf_reset_ct(skb_to_send);
>         if (want_ingress && !at_ingress) /* drop dst for egress -> ingress */
> --
> 2.52.0
>