Re: [PATCH net] net: devlink: fix UAF in devlink_compat_running_version()

[Jakub Kicinski <kuba@kernel.org>]

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);