| Message ID | f393212ad3906808ee7eb5cff06ef2e053eb9d2b.1632565508.git.leonro@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Move devlink_register to be last devlink command | expand |
On Sat, Sep 25, 2021 at 02:22:53PM +0300, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@nvidia.com> > > Open user space access to the devlink after driver is probed. Hi Leon, I think a description of why is warranted here. > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
On Mon, Sep 27, 2021 at 10:39:24AM +0200, Simon Horman wrote: > On Sat, Sep 25, 2021 at 02:22:53PM +0300, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@nvidia.com> > > > > Open user space access to the devlink after driver is probed. > > Hi Leon, > > I think a description of why is warranted here. After devlink_register(), users can send GET and SET netlink commands to the uninitialized driver. In some cases, nothing will happen, but not in all and hard to prove that ALL drivers are safe with such early access. It means that local users can (in theory for some and in practice for others) crash the system (or leverage permissions) with early devlink_register() by accessing internal to driver pointers that are not set yet. Like I said in the commit message, I'm not fixing all drivers. https://lore.kernel.org/netdev/cover.1632565508.git.leonro@nvidia.com/T/#m063eb4e67389bafcc3b3ddc07197bf43181b7209 Because some of the driver authors made a wonderful job to obfuscate their driver and write completely unmanageable code. I do move devlink_register() to be last devlink command for all drivers, to allow me to clean devlink core locking and API in next series. This series should raise your eyebrow and trigger a question: "is my driver vulnerable too?". And the answer will depend on devlink_register() position in the .probe() call. Thanks > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
On Mon, Sep 27, 2021 at 02:53:24PM +0300, Leon Romanovsky wrote: > On Mon, Sep 27, 2021 at 10:39:24AM +0200, Simon Horman wrote: > > On Sat, Sep 25, 2021 at 02:22:53PM +0300, Leon Romanovsky wrote: > > > From: Leon Romanovsky <leonro@nvidia.com> > > > > > > Open user space access to the devlink after driver is probed. > > > > Hi Leon, > > > > I think a description of why is warranted here. > > After devlink_register(), users can send GET and SET netlink commands to > the uninitialized driver. In some cases, nothing will happen, but not in > all and hard to prove that ALL drivers are safe with such early access. > > It means that local users can (in theory for some and in practice for > others) crash the system (or leverage permissions) with early devlink_register() > by accessing internal to driver pointers that are not set yet. > > Like I said in the commit message, I'm not fixing all drivers. > https://lore.kernel.org/netdev/cover.1632565508.git.leonro@nvidia.com/T/#m063eb4e67389bafcc3b3ddc07197bf43181b7209 > > Because some of the driver authors made a wonderful job to obfuscate their > driver and write completely unmanageable code. > > I do move devlink_register() to be last devlink command for all drivers, > to allow me to clean devlink core locking and API in next series. > > This series should raise your eyebrow and trigger a question: "is my > driver vulnerable too?". And the answer will depend on devlink_register() > position in the .probe() call. > > Thanks Thanks for the explanation. And thanks for taking time to update the NFP driver. > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> Acked-by: Simon Horman <simon.horman@corigine.com>
diff --git a/drivers/net/ethernet/netronome/nfp/devlink_param.c b/drivers/net/ethernet/netronome/nfp/devlink_param.c index 36491835ac65..db297ee4d7ad 100644 --- a/drivers/net/ethernet/netronome/nfp/devlink_param.c +++ b/drivers/net/ethernet/netronome/nfp/devlink_param.c @@ -233,13 +233,8 @@ int nfp_devlink_params_register(struct nfp_pf *pf) if (err <= 0) return err; - err = devlink_params_register(devlink, nfp_devlink_params, - ARRAY_SIZE(nfp_devlink_params)); - if (err) - return err; - - devlink_params_publish(devlink); - return 0; + return devlink_params_register(devlink, nfp_devlink_params, + ARRAY_SIZE(nfp_devlink_params)); } void nfp_devlink_params_unregister(struct nfp_pf *pf) diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c index 616872928ada..5fbb7c613ff1 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c @@ -701,7 +701,6 @@ int nfp_net_pci_probe(struct nfp_pf *pf) if (err) goto err_unmap; - devlink_register(devlink); err = nfp_shared_buf_register(pf); if (err) goto err_devlink_unreg; @@ -731,6 +730,7 @@ int nfp_net_pci_probe(struct nfp_pf *pf) goto err_stop_app; mutex_unlock(&pf->lock); + devlink_register(devlink); return 0; @@ -748,7 +748,6 @@ int nfp_net_pci_probe(struct nfp_pf *pf) nfp_shared_buf_unregister(pf); err_devlink_unreg: cancel_work_sync(&pf->port_refresh_work); - devlink_unregister(devlink); nfp_net_pf_app_clean(pf); err_unmap: nfp_net_pci_unmap_mem(pf); @@ -759,6 +758,7 @@ void nfp_net_pci_remove(struct nfp_pf *pf) { struct nfp_net *nn, *next; + devlink_unregister(priv_to_devlink(pf)); mutex_lock(&pf->lock); list_for_each_entry_safe(nn, next, &pf->vnics, vnic_list) { if (!nfp_net_is_data_vnic(nn)) @@ -775,7 +775,6 @@ void nfp_net_pci_remove(struct nfp_pf *pf) nfp_devlink_params_unregister(pf); nfp_shared_buf_unregister(pf); - devlink_unregister(priv_to_devlink(pf)); nfp_net_pf_free_irqs(pf); nfp_net_pf_app_clean(pf);