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: <Z9KhK_M7IinJu8Ih@mini-arch>
Date: Thu, 13 Mar 2025 02:11:07 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Stanislav Fomichev <sdf@...ichev.me>, 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 03/13, Eric Dumazet wrote:
> 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.

Oh, good catch, let me try to add more logic here to be more careful
on the replay path. We can make the caller not unlock the instance lock
in this case I think..