netdev - Re: [PATCH net-next v10 04/14] net: hold netdev instance lock during qdisc ndo_setup

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: <CANn89i+4F1f2FSUxmxP=qqir0z_3ZDNpQoqkE3X7bwp81U3sCw@mail.gmail.com>
Date: Thu, 13 Mar 2025 09:51:23 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Stanislav Fomichev <sdf@...ichev.me>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org, 
	pabeni@...hat.com, Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>, 
	Jiri Pirko <jiri@...nulli.us>, Saeed Mahameed <saeed@...nel.org>
Subject: Re: [PATCH net-next v10 04/14] net: hold netdev instance lock during
 qdisc ndo_setup_tc

On Wed, Mar 5, 2025 at 5:37 PM Stanislav Fomichev <sdf@...ichev.me> wrote:
>
> Qdisc operations that can lead to ndo_setup_tc might need
> to have an instance lock. Add netdev_lock_ops/netdev_unlock_ops
> invocations for all psched_rtnl_msg_handlers operations.
>
> Cc: Jamal Hadi Salim <jhs@...atatu.com>
> Cc: Cong Wang <xiyou.wangcong@...il.com>
> Cc: Jiri Pirko <jiri@...nulli.us>
> Cc: Saeed Mahameed <saeed@...nel.org>
> Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>
> Signed-off-by: Stanislav Fomichev <sdf@...ichev.me>
> ---
>  net/sched/sch_api.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 21940f3ae66f..f5101c2ffc66 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1279,9 +1279,11 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
>                          * We replay the request because the device may
>                          * go away in the mean time.
>                          */
> +                       netdev_unlock_ops(dev);
>                         rtnl_unlock();
>                         request_module(NET_SCH_ALIAS_PREFIX "%s", name);
>                         rtnl_lock();

Oops, dev might have disappeared.

As explained a few lines above in the comment :

/* We dropped the RTNL semaphore in order to
* perform the module load.  So, even if we
* succeeded in loading the module we have to
* tell the caller to replay the request.  We
* indicate this using -EAGAIN.
* We replay the request because the device may
* go away in the mean time.
*/



> +                       netdev_lock_ops(dev);

So this might trigger an UAF.

>                         ops = qdisc_lookup_ops(kind);
>                         if (ops != NULL) {
>                                 /* We will try again qdisc_lookup_ops,
> @@ -1591,7 +1593,11 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
>         if (!dev)
>                 return -ENODEV;
>
> -       return __tc_get_qdisc(skb, n, extack, dev, tca, tcm);
> +       netdev_lock_ops(dev);
> +       err = __tc_get_qdisc(skb, n, extack, dev, tca, tcm);
> +       netdev_unlock_ops(dev);
> +
> +       return err;
>  }
>
>  static bool req_create_or_replace(struct nlmsghdr *n)
> @@ -1828,7 +1834,9 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
>                 return -ENODEV;
>
>         replay = false;
> +       netdev_lock_ops(dev);
>         err = __tc_modify_qdisc(skb, n, extack, dev, tca, tcm, &replay);
> +       netdev_unlock_ops(dev);
>         if (replay)
>                 goto replay;
>
> @@ -1919,17 +1927,23 @@ static int tc_dump_qdisc(struct sk_buff *skb, struct netlink_callback *cb)
>                         s_q_idx = 0;
>                 q_idx = 0;
>
> +               netdev_lock_ops(dev);
>                 if (tc_dump_qdisc_root(rtnl_dereference(dev->qdisc),
>                                        skb, cb, &q_idx, s_q_idx,
> -                                      true, tca[TCA_DUMP_INVISIBLE]) < 0)
> +                                      true, tca[TCA_DUMP_INVISIBLE]) < 0) {
> +                       netdev_unlock_ops(dev);
>                         goto done;
> +               }
>
>                 dev_queue = dev_ingress_queue(dev);
>                 if (dev_queue &&
>                     tc_dump_qdisc_root(rtnl_dereference(dev_queue->qdisc_sleeping),
>                                        skb, cb, &q_idx, s_q_idx, false,
> -                                      tca[TCA_DUMP_INVISIBLE]) < 0)
> +                                      tca[TCA_DUMP_INVISIBLE]) < 0) {
> +                       netdev_unlock_ops(dev);
>                         goto done;
> +               }
> +               netdev_unlock_ops(dev);
>
>  cont:
>                 idx++;
> @@ -2308,7 +2322,11 @@ static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n,
>         if (!dev)
>                 return -ENODEV;
>
> -       return __tc_ctl_tclass(skb, n, extack, dev, tca, tcm);
> +       netdev_lock_ops(dev);
> +       err = __tc_ctl_tclass(skb, n, extack, dev, tca, tcm);
> +       netdev_unlock_ops(dev);
> +
> +       return err;
>  }
>
>  struct qdisc_dump_args {
> @@ -2426,7 +2444,9 @@ static int tc_dump_tclass(struct sk_buff *skb, struct netlink_callback *cb)
>         if (!dev)
>                 return 0;
>
> +       netdev_lock_ops(dev);
>         err = __tc_dump_tclass(skb, cb, tcm, dev);
> +       netdev_unlock_ops(dev);
>
>         dev_put(dev);
>
> --
> 2.48.1
>