netdev - Re: [PATCH net] net: devlink: fix UAF in devlink_compat_running

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: <20221122122740.4b10d67d@kernel.org>
Date:   Tue, 22 Nov 2022 12:27:40 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     Yang Yingliang <yangyingliang@...wei.com>, netdev@...r.kernel.org,
        jiri@...dia.com, davem@...emloft.net, edumazet@...gle.com,
        pabeni@...hat.com
Subject: Re: [PATCH net] net: devlink: fix UAF in
 devlink_compat_running_version()

On Tue, 22 Nov 2022 21:04:29 +0200 Leon Romanovsky wrote:
> > > Please fix nsim instead of devlink.  
> > I think if devlink is not registered, it can not be get and used, but there
> > is no API for driver to check this, can I introduce a new helper name
> > devlink_is_registered() for driver using.  
> 
> There is no need in such API as driver calls to devlink_register() and
> as such it knows when devlink is registered.
> 
> This UAF is nsim specific issue. Real devices have single .probe()
> routine with serialized registration flow. None of them will use
> devlink_is_registered() call.

Agreed, the fix is to move the register call back.
Something along the lines of the untested patch below?
Yang Yingliang would you be able to turn that into a real patch?

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index e14686594a71..26602d5fe0a2 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -1566,12 +1566,15 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 	err = devlink_params_register(devlink, nsim_devlink_params,
 				      ARRAY_SIZE(nsim_devlink_params));
 	if (err)
-		goto err_dl_unregister;
+		goto err_resource_unregister;
 	nsim_devlink_set_params_init_values(nsim_dev, devlink);
 
+	/* here, because params API still expect devlink to be unregistered */
+	devl_register(devlink);
+
 	err = nsim_dev_dummy_region_init(nsim_dev, devlink);
 	if (err)
-		goto err_params_unregister;
+		goto err_dl_unregister;
 
 	err = nsim_dev_traps_init(devlink);
 	if (err)
@@ -1610,7 +1613,6 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
 	devlink_set_features(devlink, DEVLINK_F_RELOAD);
 	devl_unlock(devlink);
-	devlink_register(devlink);
 	return 0;
 
 err_hwstats_exit:
@@ -1629,10 +1631,11 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 	nsim_dev_traps_exit(devlink);
 err_dummy_region_exit:
 	nsim_dev_dummy_region_exit(nsim_dev);
-err_params_unregister:
+err_dl_unregister:
+	devl_unregister(devlink);
 	devlink_params_unregister(devlink, nsim_devlink_params,
 				  ARRAY_SIZE(nsim_devlink_params));
-err_dl_unregister:
+err_resource_unregister:
 	devl_resources_unregister(devlink);
 err_vfc_free:
 	kfree(nsim_dev->vfconfigs);
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 074a79b8933f..e0f13100fc6b 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1593,6 +1593,14 @@ void devlink_set_features(struct devlink *devlink, u64 features);
 void devlink_register(struct devlink *devlink);
 void devlink_unregister(struct devlink *devlink);
 void devlink_free(struct devlink *devlink);
+
+void devl_register(struct devlink *devlink)
+{
+	devl_assert_locked(devlink);
+	/* doesn't actually take the instance lock */
+	devlink_register(devlink);
+}
+
 void devlink_port_init(struct devlink *devlink,
 		       struct devlink_port *devlink_port);
 void devlink_port_fini(struct devlink_port *devlink_port);