netdev - Re: [PATCH net v3 2/2] net/sched: act_mirred: don't override retval if we already lost the skb

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM0EoMm3QgW7BRF0HUSCStHWDst=pajq3uFiKhotEeLmu9kJhw@mail.gmail.com>
Date: Thu, 15 Feb 2024 12:33:41 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com, 
	pabeni@...hat.com, Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>, 
	xiyou.wangcong@...il.com, jiri@...nulli.us
Subject: Re: [PATCH net v3 2/2] net/sched: act_mirred: don't override retval
 if we already lost the skb

On Thu, Feb 15, 2024 at 9:33 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> If we're redirecting the skb, and haven't called tcf_mirred_forward(),
> yet, we need to tell the core to drop the skb by setting the retcode
> to SHOT. If we have called tcf_mirred_forward(), however, the skb
> is out of our hands and returning SHOT will lead to UaF.
>
> Move the retval override to the error path which actually need it.
>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> Fixes: e5cf1baf92cb ("act_mirred: use TC_ACT_REINSERT when possible")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

Acked-by: Jamal Hadi Salim <jhs@...atatu.com>


cheers,
jamal

> ---
> CC: jhs@...atatu.com
> CC: xiyou.wangcong@...il.com
> CC: jiri@...nulli.us
> ---
>  net/sched/act_mirred.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index 291d47c9eb69..6faa7d00da09 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -266,8 +266,7 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
>         if (unlikely(!(dev->flags & IFF_UP)) || !netif_carrier_ok(dev)) {
>                 net_notice_ratelimited("tc mirred to Houston: device %s is down\n",
>                                        dev->name);
> -               err = -ENODEV;
> -               goto out;
> +               goto err_cant_do;
>         }
>
>         /* we could easily avoid the clone only if called by ingress and clsact;
> @@ -279,10 +278,8 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
>                 tcf_mirred_can_reinsert(retval);
>         if (!dont_clone) {
>                 skb_to_send = skb_clone(skb, GFP_ATOMIC);
> -               if (!skb_to_send) {
> -                       err =  -ENOMEM;
> -                       goto out;
> -               }
> +               if (!skb_to_send)
> +                       goto err_cant_do;
>         }
>
>         want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
> @@ -319,15 +316,16 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
>         } else {
>                 err = tcf_mirred_forward(at_ingress, want_ingress, skb_to_send);
>         }
> -
> -       if (err) {
> -out:
> +       if (err)
>                 tcf_action_inc_overlimit_qstats(&m->common);
> -               if (is_redirect)
> -                       retval = TC_ACT_SHOT;
> -       }
>
>         return retval;
> +
> +err_cant_do:
> +       if (is_redirect)
> +               retval = TC_ACT_SHOT;
> +       tcf_action_inc_overlimit_qstats(&m->common);
> +       return retval;
>  }
>
>  static int tcf_blockcast_redir(struct sk_buff *skb, struct tcf_mirred *m,
> --
> 2.43.0
>