[PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Dan Carpenter]

My static checker complains that these "vf" variables come from the
user in do_setvfinfo() and haven't been checked to make sure they're
valid.

Fixes: eff901d30e6c ("IB/mlx5: Implement callbacks for manipulating VFs")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Untested static checker stuff.  Please review carefully.

 drivers/infiniband/hw/mlx5/ib_virt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/infiniband/hw/mlx5/ib_virt.c b/drivers/infiniband/hw/mlx5/ib_virt.c
index 649a3364f838..9a8eebe3d462 100644
--- a/drivers/infiniband/hw/mlx5/ib_virt.c
+++ b/drivers/infiniband/hw/mlx5/ib_virt.c
@@ -56,6 +56,9 @@ int mlx5_ib_get_vf_config(struct ib_device *device, int vf, u8 port,
 	struct mlx5_hca_vport_context *rep;
 	int err;
 
+	if (vf < 0 || vf >= pci_sriov_get_totalvfs(mdev->pdev))
+		return -EINVAL;
+
 	rep = kzalloc(sizeof(*rep), GFP_KERNEL);
 	if (!rep)
 		return -ENOMEM;
@@ -99,6 +102,9 @@ int mlx5_ib_set_vf_link_state(struct ib_device *device, int vf,
 	struct mlx5_vf_context *vfs_ctx = mdev->priv.sriov.vfs_ctx;
 	int err;
 
+	if (vf < 0 || vf >= pci_sriov_get_totalvfs(mdev->pdev))
+		return -EINVAL;
+
 	in = kzalloc(sizeof(*in), GFP_KERNEL);
 	if (!in)
 		return -ENOMEM;
-- 
2.17.1

RE: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Parav Pandit]

> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> owner@vger.kernel.org> On Behalf Of Dan Carpenter
> Sent: Friday, April 12, 2019 12:55 PM
> To: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>
> Cc: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> Subject: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> 
> My static checker complains that these "vf" variables come from the user in
> do_setvfinfo() and haven't been checked to make sure they're valid.
> 
> Fixes: eff901d30e6c ("IB/mlx5: Implement callbacks for manipulating VFs")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Untested static checker stuff.  Please review carefully.
> 
>  drivers/infiniband/hw/mlx5/ib_virt.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/mlx5/ib_virt.c
> b/drivers/infiniband/hw/mlx5/ib_virt.c
> index 649a3364f838..9a8eebe3d462 100644
> --- a/drivers/infiniband/hw/mlx5/ib_virt.c
> +++ b/drivers/infiniband/hw/mlx5/ib_virt.c
> @@ -56,6 +56,9 @@ int mlx5_ib_get_vf_config(struct ib_device *device, int
> vf, u8 port,
>  	struct mlx5_hca_vport_context *rep;
>  	int err;
> 
> +	if (vf < 0 || vf >= pci_sriov_get_totalvfs(mdev->pdev))
> +		return -EINVAL;
> +
I traced back ndo_get_vf_config and friend functions. vf number is u32 from user space.

And all the VF operations at ndo ops level and at driver level should be changed from int to u32.
After that vf < 0 check is not needed.

>  	rep = kzalloc(sizeof(*rep), GFP_KERNEL);
>  	if (!rep)
>  		return -ENOMEM;
> @@ -99,6 +102,9 @@ int mlx5_ib_set_vf_link_state(struct ib_device *device,
> int vf,
>  	struct mlx5_vf_context *vfs_ctx = mdev->priv.sriov.vfs_ctx;
>  	int err;
> 
> +	if (vf < 0 || vf >= pci_sriov_get_totalvfs(mdev->pdev))
> +		return -EINVAL;
> +
We are currently working on a patch to post in mid-May to not have access to pdev in this file.
If the vf > desired value, HCA firmware should be failing this command too.

>  	in = kzalloc(sizeof(*in), GFP_KERNEL);
>  	if (!in)
>  		return -ENOMEM;
> --
> 2.17.1

Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Dan Carpenter]

On Fri, Apr 12, 2019 at 08:25:05PM +0000, Parav Pandit wrote:
> 
> 
> > -----Original Message-----
> > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > owner@vger.kernel.org> On Behalf Of Dan Carpenter
> > Sent: Friday, April 12, 2019 12:55 PM
> > To: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>
> > Cc: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> > linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> > Subject: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> > 
> > My static checker complains that these "vf" variables come from the user in
> > do_setvfinfo() and haven't been checked to make sure they're valid.
> > 
> > Fixes: eff901d30e6c ("IB/mlx5: Implement callbacks for manipulating VFs")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > Untested static checker stuff.  Please review carefully.
> > 
> >  drivers/infiniband/hw/mlx5/ib_virt.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/infiniband/hw/mlx5/ib_virt.c
> > b/drivers/infiniband/hw/mlx5/ib_virt.c
> > index 649a3364f838..9a8eebe3d462 100644
> > --- a/drivers/infiniband/hw/mlx5/ib_virt.c
> > +++ b/drivers/infiniband/hw/mlx5/ib_virt.c
> > @@ -56,6 +56,9 @@ int mlx5_ib_get_vf_config(struct ib_device *device, int
> > vf, u8 port,
> >  	struct mlx5_hca_vport_context *rep;
> >  	int err;
> > 
> > +	if (vf < 0 || vf >= pci_sriov_get_totalvfs(mdev->pdev))
> > +		return -EINVAL;
> > +
> I traced back ndo_get_vf_config and friend functions. vf number is u32 from user space.
> 
> And all the VF operations at ndo ops level and at driver level should be changed from int to u32.
> After that vf < 0 check is not needed.
> 
> >  	rep = kzalloc(sizeof(*rep), GFP_KERNEL);
> >  	if (!rep)
> >  		return -ENOMEM;
> > @@ -99,6 +102,9 @@ int mlx5_ib_set_vf_link_state(struct ib_device *device,
> > int vf,
> >  	struct mlx5_vf_context *vfs_ctx = mdev->priv.sriov.vfs_ctx;
> >  	int err;
> > 
> > +	if (vf < 0 || vf >= pci_sriov_get_totalvfs(mdev->pdev))
> > +		return -EINVAL;
> > +
> We are currently working on a patch to post in mid-May to not have access to pdev in this file.
> If the vf > desired value, HCA firmware should be failing this command too.

I'm not sure what you want me to do with this information.  :P

regards,
dan carpenter

Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Dan Carpenter]

On Fri, Apr 12, 2019 at 08:25:05PM +0000, Parav Pandit wrote:
> 
> 
> > -----Original Message-----
> > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > owner@vger.kernel.org> On Behalf Of Dan Carpenter
> > Sent: Friday, April 12, 2019 12:55 PM
> > To: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>
> > Cc: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> > linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> > Subject: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> > 
> > My static checker complains that these "vf" variables come from the user in
> > do_setvfinfo() and haven't been checked to make sure they're valid.
> > 
> > Fixes: eff901d30e6c ("IB/mlx5: Implement callbacks for manipulating VFs")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > Untested static checker stuff.  Please review carefully.
> > 
> >  drivers/infiniband/hw/mlx5/ib_virt.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/infiniband/hw/mlx5/ib_virt.c
> > b/drivers/infiniband/hw/mlx5/ib_virt.c
> > index 649a3364f838..9a8eebe3d462 100644
> > --- a/drivers/infiniband/hw/mlx5/ib_virt.c
> > +++ b/drivers/infiniband/hw/mlx5/ib_virt.c
> > @@ -56,6 +56,9 @@ int mlx5_ib_get_vf_config(struct ib_device *device, int
> > vf, u8 port,
> >  	struct mlx5_hca_vport_context *rep;
> >  	int err;
> > 
> > +	if (vf < 0 || vf >= pci_sriov_get_totalvfs(mdev->pdev))
> > +		return -EINVAL;
> > +
> I traced back ndo_get_vf_config and friend functions. vf number is u32 from user space.
> 
> And all the VF operations at ndo ops level and at driver level should be changed from int to u32.
> After that vf < 0 check is not needed.
> 

I've been thinking about this and I don't think it's a good idea.  It
just makes backporting the fix a lot more complicated.  It might be a
good idea as a cleanup later though.

regards,
dan carpenter

RE: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Parav Pandit]

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Monday, April 15, 2019 4:46 AM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>;
> Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> 
> On Fri, Apr 12, 2019 at 08:25:05PM +0000, Parav Pandit wrote:
> >
> >
> > > -----Original Message-----
> > > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > > owner@vger.kernel.org> On Behalf Of Dan Carpenter
> > > Sent: Friday, April 12, 2019 12:55 PM
> > > To: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>
> > > Cc: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe
> > > <jgg@ziepe.ca>; linux-rdma@vger.kernel.org;
> > > kernel-janitors@vger.kernel.org
> > > Subject: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> > >
> > > My static checker complains that these "vf" variables come from the
> > > user in
> > > do_setvfinfo() and haven't been checked to make sure they're valid.
> > >
> > > Fixes: eff901d30e6c ("IB/mlx5: Implement callbacks for manipulating
> > > VFs")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > > Untested static checker stuff.  Please review carefully.
> > >
> > >  drivers/infiniband/hw/mlx5/ib_virt.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/infiniband/hw/mlx5/ib_virt.c
> > > b/drivers/infiniband/hw/mlx5/ib_virt.c
> > > index 649a3364f838..9a8eebe3d462 100644
> > > --- a/drivers/infiniband/hw/mlx5/ib_virt.c
> > > +++ b/drivers/infiniband/hw/mlx5/ib_virt.c
> > > @@ -56,6 +56,9 @@ int mlx5_ib_get_vf_config(struct ib_device
> > > *device, int vf, u8 port,
> > >  	struct mlx5_hca_vport_context *rep;
> > >  	int err;
> > >
> > > +	if (vf < 0 || vf >= pci_sriov_get_totalvfs(mdev->pdev))
> > > +		return -EINVAL;
> > > +
> > I traced back ndo_get_vf_config and friend functions. vf number is u32
> from user space.
> >
> > And all the VF operations at ndo ops level and at driver level should be
> changed from int to u32.
> > After that vf < 0 check is not needed.
> >
> 
> I've been thinking about this and I don't think it's a good idea.  It just makes
> backporting the fix a lot more complicated.  It might be a good idea as a
> cleanup later though.
> 
Data type correction I think is common approach. I have seen int to bool changes.

Regarding this fix, I am saying if vf index is negative (as very large positive number for fw), it will get failed anyway when its > total_num_vfs.
Do you see any error by passing large number currently which desires this patch or just the static checker?
If it is static checker, I would prefer we fix the datatype..

> regards,
> dan carpenter
> 

Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Dan Carpenter]

On Mon, Apr 15, 2019 at 08:04:43PM +0000, Parav Pandit wrote:
> 
> 
> > -----Original Message-----
> > From: Dan Carpenter <dan.carpenter@oracle.com>
> > Sent: Monday, April 15, 2019 4:46 AM
> > To: Parav Pandit <parav@mellanox.com>
> > Cc: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>;
> > Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> > linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> > Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> > 
> > On Fri, Apr 12, 2019 at 08:25:05PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > > > owner@vger.kernel.org> On Behalf Of Dan Carpenter
> > > > Sent: Friday, April 12, 2019 12:55 PM
> > > > To: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>
> > > > Cc: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe
> > > > <jgg@ziepe.ca>; linux-rdma@vger.kernel.org;
> > > > kernel-janitors@vger.kernel.org
> > > > Subject: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> > > >
> > > > My static checker complains that these "vf" variables come from the
> > > > user in
> > > > do_setvfinfo() and haven't been checked to make sure they're valid.
> > > >
> > > > Fixes: eff901d30e6c ("IB/mlx5: Implement callbacks for manipulating
> > > > VFs")
> > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > ---
> > > > Untested static checker stuff.  Please review carefully.
> > > >
> > > >  drivers/infiniband/hw/mlx5/ib_virt.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/drivers/infiniband/hw/mlx5/ib_virt.c
> > > > b/drivers/infiniband/hw/mlx5/ib_virt.c
> > > > index 649a3364f838..9a8eebe3d462 100644
> > > > --- a/drivers/infiniband/hw/mlx5/ib_virt.c
> > > > +++ b/drivers/infiniband/hw/mlx5/ib_virt.c
> > > > @@ -56,6 +56,9 @@ int mlx5_ib_get_vf_config(struct ib_device
> > > > *device, int vf, u8 port,
> > > >  	struct mlx5_hca_vport_context *rep;
> > > >  	int err;
> > > >
> > > > +	if (vf < 0 || vf >= pci_sriov_get_totalvfs(mdev->pdev))
> > > > +		return -EINVAL;
> > > > +
> > > I traced back ndo_get_vf_config and friend functions. vf number is u32
> > from user space.
> > >
> > > And all the VF operations at ndo ops level and at driver level should be
> > changed from int to u32.
> > > After that vf < 0 check is not needed.
> > >
> > 
> > I've been thinking about this and I don't think it's a good idea.  It just makes
> > backporting the fix a lot more complicated.  It might be a good idea as a
> > cleanup later though.
> > 
> Data type correction I think is common approach. I have seen int to bool changes.
> 
> Regarding this fix, I am saying if vf index is negative (as very large positive number for fw), it will get failed anyway when its > total_num_vfs.

Yeah.  But the call tree here is:

do_setvfinfo()
-> ops->ndo_get_vf_config()
   -> rtnl_fill_vfinfo()
      -> dev->netdev_ops->ndo_get_vf_config(dev, vfs_num, &ivi)
         -> ipoib_get_vf_config()
            -> ib_get_vf_config
               -> device->ops.get_vf_config(device, vf, port, info);

Changing the ->ndo_get_vf_config() pointer means you have to update 20
functions in various drivers.  It becomes quite involved.  We should
apply this simple self contained fix then worry about doing other
cleanups later.

> Do you see any error by passing large number currently which desires this patch or just the static checker?
> If it is static checker, I would prefer we fix the datatype..

I don't really understand the question, but I haven't tested this fix,
it's from static analysis.

regards,
dan carpenter

RE: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Parav Pandit]

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Tuesday, April 16, 2019 3:21 AM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>;
> Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> 
> On Mon, Apr 15, 2019 at 08:04:43PM +0000, Parav Pandit wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dan Carpenter <dan.carpenter@oracle.com>
> > > Sent: Monday, April 15, 2019 4:46 AM
> > > To: Parav Pandit <parav@mellanox.com>
> > > Cc: Leon Romanovsky <leon@kernel.org>; Eli Cohen
> <eli@mellanox.com>;
> > > Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> > > linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> > > Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from
> > > do_setvfinfo()
> > >
> > > On Fri, Apr 12, 2019 at 08:25:05PM +0000, Parav Pandit wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > > > > owner@vger.kernel.org> On Behalf Of Dan Carpenter
> > > > > Sent: Friday, April 12, 2019 12:55 PM
> > > > > To: Leon Romanovsky <leon@kernel.org>; Eli Cohen
> > > > > <eli@mellanox.com>
> > > > > Cc: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe
> > > > > <jgg@ziepe.ca>; linux-rdma@vger.kernel.org;
> > > > > kernel-janitors@vger.kernel.org
> > > > > Subject: [PATCH] IB/mlx5: add checking for "vf" from
> > > > > do_setvfinfo()
> > > > >
> > > > > My static checker complains that these "vf" variables come from
> > > > > the user in
> > > > > do_setvfinfo() and haven't been checked to make sure they're valid.
> > > > >
> > > > > Fixes: eff901d30e6c ("IB/mlx5: Implement callbacks for
> > > > > manipulating
> > > > > VFs")
> > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > > ---
> > > > > Untested static checker stuff.  Please review carefully.
> > > > >
> > > > >  drivers/infiniband/hw/mlx5/ib_virt.c | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/drivers/infiniband/hw/mlx5/ib_virt.c
> > > > > b/drivers/infiniband/hw/mlx5/ib_virt.c
> > > > > index 649a3364f838..9a8eebe3d462 100644
> > > > > --- a/drivers/infiniband/hw/mlx5/ib_virt.c
> > > > > +++ b/drivers/infiniband/hw/mlx5/ib_virt.c
> > > > > @@ -56,6 +56,9 @@ int mlx5_ib_get_vf_config(struct ib_device
> > > > > *device, int vf, u8 port,
> > > > >  	struct mlx5_hca_vport_context *rep;
> > > > >  	int err;
> > > > >
> > > > > +	if (vf < 0 || vf >= pci_sriov_get_totalvfs(mdev->pdev))
> > > > > +		return -EINVAL;
> > > > > +
> > > > I traced back ndo_get_vf_config and friend functions. vf number is
> > > > u32
> > > from user space.
> > > >
> > > > And all the VF operations at ndo ops level and at driver level
> > > > should be
> > > changed from int to u32.
> > > > After that vf < 0 check is not needed.
> > > >
> > >
> > > I've been thinking about this and I don't think it's a good idea.
> > > It just makes backporting the fix a lot more complicated.  It might
> > > be a good idea as a cleanup later though.
> > >
> > Data type correction I think is common approach. I have seen int to bool
> changes.
> >
> > Regarding this fix, I am saying if vf index is negative (as very large positive
> number for fw), it will get failed anyway when its > total_num_vfs.
> 
> Yeah.  But the call tree here is:
> 
> do_setvfinfo()
> -> ops->ndo_get_vf_config()
>    -> rtnl_fill_vfinfo()
>       -> dev->netdev_ops->ndo_get_vf_config(dev, vfs_num, &ivi)
>          -> ipoib_get_vf_config()
>             -> ib_get_vf_config
>                -> device->ops.get_vf_config(device, vf, port, info);
> 
> Changing the ->ndo_get_vf_config() pointer means you have to update 20
> functions in various drivers.  It becomes quite involved.  We should apply
> this simple self contained fix then worry about doing other cleanups later.
> 
But if a static checker is run on following functions, they need for vf < 0 check.

i40e_ndo_get_vf_config
mlx5e_get_vf_config
bnxt_get_vf_config
etc and few more.

So few functions to be updated anyway.
So why not better do the right cleanup.

Also there is nothing prevents for vf to go to zero after pci_sriov_get_totalvfs() is passed.

> > Do you see any error by passing large number currently which desires this
> patch or just the static checker?
> > If it is static checker, I would prefer we fix the datatype..
> 
> I don't really understand the question, but I haven't tested this fix, it's from
> static analysis.
> 
Ok. yeah, that is what I was asking.

> regards,
> dan carpenter

Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Dan Carpenter]

On Tue, Apr 16, 2019 at 10:54:42PM +0000, Parav Pandit wrote:
> > Yeah.  But the call tree here is:
> > 
> > do_setvfinfo()
> > -> ops->ndo_get_vf_config()
> >    -> rtnl_fill_vfinfo()
> >       -> dev->netdev_ops->ndo_get_vf_config(dev, vfs_num, &ivi)
> >          -> ipoib_get_vf_config()
> >             -> ib_get_vf_config
> >                -> device->ops.get_vf_config(device, vf, port, info);
> > 
> > Changing the ->ndo_get_vf_config() pointer means you have to update 20
> > functions in various drivers.  It becomes quite involved.  We should apply
> > this simple self contained fix then worry about doing other cleanups later.
> > 
> But if a static checker is run on following functions, they need for vf < 0 check.
> 
> i40e_ndo_get_vf_config
> mlx5e_get_vf_config
> bnxt_get_vf_config
> etc and few more.

I checked again to see if it was "20 functions" or if it was "etc and
few more"...  It turns out its only 18 functions because I double
counted two functions at first.  Here is the list:

be_get_vf_config
bnx2x_get_vf_config
bnxt_get_vf_config
cxgb4_mgmt_get_vf_config
efx_sriov_get_vf_config
fm10k_ndo_get_vf_config
i40e_ndo_get_vf_config
ice_get_vf_cfg
igb_ndo_get_vf_config
ipoib_get_vf_config
ixgbe_ndo_get_vf_config
liquidio_get_vf_config
mlx4_en_get_vf_config
mlx5e_get_vf_config
nfp_app_get_vf_config
nsim_get_vf_config
qede_get_vf_config
qlcnic_sriov_get_vf_config

But you also have to update the call tress as well...  It's really very
involved.  I seriously did look at how to do this and the original patch
is the Right Thing To Do [tm].  I've probably sent around 92 underflow
patches and sometimes I would definitely agree with you that changing
the type is the right fix but not in this case.

regards,
dan carpenter

RE: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Parav Pandit]

Hi Dan,

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Monday, April 22, 2019 3:09 AM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>;
> Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> 
> On Tue, Apr 16, 2019 at 10:54:42PM +0000, Parav Pandit wrote:
> > > Yeah.  But the call tree here is:
> > >
> > > do_setvfinfo()
> > > -> ops->ndo_get_vf_config()
> > >    -> rtnl_fill_vfinfo()
> > >       -> dev->netdev_ops->ndo_get_vf_config(dev, vfs_num, &ivi)
> > >          -> ipoib_get_vf_config()
> > >             -> ib_get_vf_config
> > >                -> device->ops.get_vf_config(device, vf, port, info);
> > >
> > > Changing the ->ndo_get_vf_config() pointer means you have to update
> > > 20 functions in various drivers.  It becomes quite involved.  We
> > > should apply this simple self contained fix then worry about doing other
> cleanups later.
> > >
> > But if a static checker is run on following functions, they need for vf < 0
> check.
> >
> > i40e_ndo_get_vf_config
> > mlx5e_get_vf_config
> > bnxt_get_vf_config
> > etc and few more.
> 
> I checked again to see if it was "20 functions" or if it was "etc and few
> more"...  It turns out its only 18 functions because I double counted two
> functions at first.  Here is the list:
> 
> be_get_vf_config
> bnx2x_get_vf_config
> bnxt_get_vf_config
> cxgb4_mgmt_get_vf_config
> efx_sriov_get_vf_config
> fm10k_ndo_get_vf_config
> i40e_ndo_get_vf_config
> ice_get_vf_cfg
> igb_ndo_get_vf_config
> ipoib_get_vf_config
> ixgbe_ndo_get_vf_config
> liquidio_get_vf_config
> mlx4_en_get_vf_config
> mlx5e_get_vf_config
> nfp_app_get_vf_config
> nsim_get_vf_config
> qede_get_vf_config
> qlcnic_sriov_get_vf_config
> 
> But you also have to update the call tress as well...  It's really very involved.  I
> seriously did look at how to do this and the original patch is the Right Thing
> To Do [tm].  I've probably sent around 92 underflow patches and sometimes
> I would definitely agree with you that changing the type is the right fix but
> not in this case.
> 
Do you plan to fix all the above functions for < 0?
There are other several other ndo_get_vf_* functions who need < 0 check. What about them?
Will you fix them as well?
Instead of doing all those fixes, why not use right u32 data type to eliminate < 0 check?

What about sriov getting disabled right after > vf check passes?

Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Dan Carpenter]

On Mon, Apr 22, 2019 at 03:09:00PM +0000, Parav Pandit wrote:
> Hi Dan,
> 
> > -----Original Message-----
> > From: Dan Carpenter <dan.carpenter@oracle.com>
> > Sent: Monday, April 22, 2019 3:09 AM
> > To: Parav Pandit <parav@mellanox.com>
> > Cc: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>;
> > Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> > linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> > Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> > 
> > On Tue, Apr 16, 2019 at 10:54:42PM +0000, Parav Pandit wrote:
> > > > Yeah.  But the call tree here is:
> > > >
> > > > do_setvfinfo()
> > > > -> ops->ndo_get_vf_config()
> > > >    -> rtnl_fill_vfinfo()
> > > >       -> dev->netdev_ops->ndo_get_vf_config(dev, vfs_num, &ivi)
> > > >          -> ipoib_get_vf_config()
> > > >             -> ib_get_vf_config
> > > >                -> device->ops.get_vf_config(device, vf, port, info);
> > > >
> > > > Changing the ->ndo_get_vf_config() pointer means you have to update
> > > > 20 functions in various drivers.  It becomes quite involved.  We
> > > > should apply this simple self contained fix then worry about doing other
> > cleanups later.
> > > >
> > > But if a static checker is run on following functions, they need for vf < 0
> > check.
> > >
> > > i40e_ndo_get_vf_config
> > > mlx5e_get_vf_config
> > > bnxt_get_vf_config
> > > etc and few more.
> > 
> > I checked again to see if it was "20 functions" or if it was "etc and few
> > more"...  It turns out its only 18 functions because I double counted two
> > functions at first.  Here is the list:
> > 
> > be_get_vf_config
> > bnx2x_get_vf_config
> > bnxt_get_vf_config
> > cxgb4_mgmt_get_vf_config
> > efx_sriov_get_vf_config
> > fm10k_ndo_get_vf_config
> > i40e_ndo_get_vf_config
> > ice_get_vf_cfg
> > igb_ndo_get_vf_config
> > ipoib_get_vf_config
> > ixgbe_ndo_get_vf_config
> > liquidio_get_vf_config
> > mlx4_en_get_vf_config
> > mlx5e_get_vf_config
> > nfp_app_get_vf_config
> > nsim_get_vf_config
> > qede_get_vf_config
> > qlcnic_sriov_get_vf_config
> > 
> > But you also have to update the call tress as well...  It's really very involved.  I
> > seriously did look at how to do this and the original patch is the Right Thing
> > To Do [tm].  I've probably sent around 92 underflow patches and sometimes
> > I would definitely agree with you that changing the type is the right fix but
> > not in this case.
> > 
> Do you plan to fix all the above functions for < 0?
> There are other several other ndo_get_vf_* functions who need < 0 check. What about them?
> Will you fix them as well?

Oh wow...  I'm looking at these now and there are a lot of bugs.  You
are right.  To be honest, though, I'm tempted to just add a check for
negatives in the core...  I know you don't like that...

My static analysis was supposed to catch these underflows.  It's the end
of the day for me, but I will get this working tomorrow.

> Instead of doing all those fixes, why not use right u32 data type to eliminate < 0 check?
> 
> What about sriov getting disabled right after > vf check passes?

I don't know the subsystem well enough to answer this question.  What
do you suggest?

regards,
dan carpenter

RE: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Parav Pandit]

Hi Dan,

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Tuesday, April 23, 2019 10:50 AM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>;
> Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> 
> On Mon, Apr 22, 2019 at 03:09:00PM +0000, Parav Pandit wrote:
> > Hi Dan,
> >
> > > -----Original Message-----
> > > From: Dan Carpenter <dan.carpenter@oracle.com>
> > > Sent: Monday, April 22, 2019 3:09 AM
> > > To: Parav Pandit <parav@mellanox.com>
> > > Cc: Leon Romanovsky <leon@kernel.org>; Eli Cohen
> <eli@mellanox.com>;
> > > Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> > > linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> > > Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from
> > > do_setvfinfo()
> > >
> > > On Tue, Apr 16, 2019 at 10:54:42PM +0000, Parav Pandit wrote:
> > > > > Yeah.  But the call tree here is:
> > > > >
> > > > > do_setvfinfo()
> > > > > -> ops->ndo_get_vf_config()
> > > > >    -> rtnl_fill_vfinfo()
> > > > >       -> dev->netdev_ops->ndo_get_vf_config(dev, vfs_num, &ivi)
> > > > >          -> ipoib_get_vf_config()
> > > > >             -> ib_get_vf_config
> > > > >                -> device->ops.get_vf_config(device, vf, port,
> > > > > info);
> > > > >
> > > > > Changing the ->ndo_get_vf_config() pointer means you have to
> > > > > update
> > > > > 20 functions in various drivers.  It becomes quite involved.  We
> > > > > should apply this simple self contained fix then worry about
> > > > > doing other
> > > cleanups later.
> > > > >
> > > > But if a static checker is run on following functions, they need
> > > > for vf < 0
> > > check.
> > > >
> > > > i40e_ndo_get_vf_config
> > > > mlx5e_get_vf_config
> > > > bnxt_get_vf_config
> > > > etc and few more.
> > >
> > > I checked again to see if it was "20 functions" or if it was "etc
> > > and few more"...  It turns out its only 18 functions because I
> > > double counted two functions at first.  Here is the list:
> > >
> > > be_get_vf_config
> > > bnx2x_get_vf_config
> > > bnxt_get_vf_config
> > > cxgb4_mgmt_get_vf_config
> > > efx_sriov_get_vf_config
> > > fm10k_ndo_get_vf_config
> > > i40e_ndo_get_vf_config
> > > ice_get_vf_cfg
> > > igb_ndo_get_vf_config
> > > ipoib_get_vf_config
> > > ixgbe_ndo_get_vf_config
> > > liquidio_get_vf_config
> > > mlx4_en_get_vf_config
> > > mlx5e_get_vf_config
> > > nfp_app_get_vf_config
> > > nsim_get_vf_config
> > > qede_get_vf_config
> > > qlcnic_sriov_get_vf_config
> > >
> > > But you also have to update the call tress as well...  It's really
> > > very involved.  I seriously did look at how to do this and the
> > > original patch is the Right Thing To Do [tm].  I've probably sent
> > > around 92 underflow patches and sometimes I would definitely agree
> > > with you that changing the type is the right fix but not in this case.
> > >
> > Do you plan to fix all the above functions for < 0?
> > There are other several other ndo_get_vf_* functions who need < 0 check.
> What about them?
> > Will you fix them as well?
> 
> Oh wow...  I'm looking at these now and there are a lot of bugs.  You are
> right.  To be honest, though, I'm tempted to just add a check for negatives in
> the core...  I know you don't like that...
> 
> My static analysis was supposed to catch these underflows.  It's the end of
> the day for me, but I will get this working tomorrow.
> 
We just make num_vfs to u32 in core and in drivers.
This will eliminate < 0 check. From user space value coming is u32 via netlink.
This makes things clear, forward looking.

> > Instead of doing all those fixes, why not use right u32 data type to
> eliminate < 0 check?
> >
> > What about sriov getting disabled right after > vf check passes?
> 
> I don't know the subsystem well enough to answer this question.  What do
> you suggest?
> 
I don't think > num_vfs check is needed. Device is expected to fail commands for VFs which are not valid.

Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Dan Carpenter]

I think I'm just going to ask netdev for an opinion on this.  It could
be that we're just reading the code wrong...

I'm getting a lot of Smatch warning about buffer underflows.  The
problem is that Smatch marks everything from nla_data() as unknown and
untrusted user data.  In do_setvfinfo() we get the "->vf" values from
nla_data().  It starts as u32, but all the function pointers in
net_device_ops use it as a signed integer.  Most of the functions return
-EINVAL if "vf" is negative but there are at least 48 which potentially
use negative values as an offset into an array.

To me making "vf" a u32 throughout seems like a good idea but it's an
extensive patch and I'm not really able to test it at all.  But maybe
there is a better place to check for negatives.  Or maybe we are already
checking for negatives and I haven't seen it.  (I don't know this code
very well at all).

regards,
dan carpenter

drivers/net/ethernet/emulex/benet/be_main.c:1955 be_clear_vf_tvt() error: buffer underflow 'adapter->vf_cfg' 's32min-65534'
drivers/net/ethernet/emulex/benet/be_main.c:1904 be_get_vf_config() error: buffer underflow 'adapter->vf_cfg' 's32min-s32max'
drivers/net/ethernet/emulex/benet/be_main.c:2095 be_set_vf_link_state() error: buffer underflow 'adapter->vf_cfg' 's32min-65534'
drivers/net/ethernet/emulex/benet/be_main.c:1863 be_set_vf_mac() error: buffer underflow 'adapter->vf_cfg' 's32min-s32max'
drivers/net/ethernet/emulex/benet/be_main.c:2103 be_set_vf_spoofchk() error: buffer underflow 'adapter->vf_cfg' 's32min-s32max'
drivers/net/ethernet/emulex/benet/be_main.c:1926 be_set_vf_tvt() error: buffer underflow 'adapter->vf_cfg' 's32min-65534'
drivers/net/ethernet/emulex/benet/be_main.c:2067 be_set_vf_tx_rate() error: buffer underflow 'adapter->vf_cfg' 's32min-65534'
drivers/net/ethernet/emulex/benet/be_main.c:1984 be_set_vf_vlan() error: buffer underflow 'adapter->vf_cfg' 's32min-s32max'
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c:2281 bnx2x_post_vf_bulletin() error: buffer underflow 'bp->vfdb->vfs' 's32min-63'
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c:1523 bnx2x_set_vf_link_state() error: buffer underflow 'bp->vfdb->vfs' 's32min-s32max'
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c:2963 bnx2x_set_vf_spoofchk() error: buffer underflow 'bp->vfdb->vfs' 's32min-s32max'
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c:2589 bnx2x_vf_op_prep() error: buffer underflow 'bp->vfdb->vfs' 's32min-65534'
drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c:202 bnxt_get_vf_config() error: buffer underflow 'bp->pf.vf' 's32min-65534'
drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c:309 bnxt_set_vf_bw() error: buffer underflow 'bp->pf.vf' 's32min-65534'
drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c:349 bnxt_set_vf_link_state() error: buffer underflow 'bp->pf.vf' 's32min-65534'
drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c:244 bnxt_set_vf_mac() error: buffer underflow 'bp->pf.vf' 's32min-65534'
drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c:96 bnxt_set_vf_spoofchk() error: buffer underflow 'bp->pf.vf' 's32min-65534'
drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c:180 bnxt_set_vf_trust() error: buffer underflow 'bp->pf.vf' 's32min-65534'
drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c:280 bnxt_set_vf_vlan() error: buffer underflow 'bp->pf.vf' 's32min-65534'
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c:2736 cxgb4_mgmt_get_vf_config() error: buffer underflow 'adap->vfinfo' 's32min-254'
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c:2923 cxgb4_mgmt_set_vf_link_state() error: buffer underflow 'adap->vfinfo' 's32min-254'
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c:2723 cxgb4_mgmt_set_vf_mac() error: buffer underflow 'adap->vfinfo' 's32min-s32max'
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c:2797 cxgb4_mgmt_set_vf_rate() error: buffer underflow 'adap->vfinfo' 's32min-254'
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c:2875 cxgb4_mgmt_set_vf_vlan() error: buffer underflow 'adap->vfinfo' 's32min-254'
drivers/net/ethernet/freescale/enetc/enetc_pf.c:377 enetc_pf_set_vf_mac() error: buffer underflow 'pf->vf_state' 's32min-2147483646'
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c:7069 hclge_set_vf_vlan_filter() error: buffer underflow 'hdev->vport' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:4223 i40e_ndo_get_vf_config() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:4177 i40e_ndo_set_vf_bw() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:4287 i40e_ndo_set_vf_link_state() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:3895 i40e_ndo_set_vf_mac() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:4041 i40e_ndo_set_vf_port_vlan() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:4357 i40e_ndo_set_vf_spoofchk() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:4420 i40e_ndo_set_vf_trust() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:3862 i40e_validate_vf() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c:2678 ice_get_vf_cfg() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c:2879 ice_set_vf_link_state() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c:2792 ice_set_vf_mac() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c:2246 ice_set_vf_port_vlan() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c:2731 ice_set_vf_spoofchk() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c:2839 ice_set_vf_trust() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/infiniband/hw/mlx5/ib_virt.c:114 mlx5_ib_set_vf_link_state() error: buffer underflow 'vfs_ctx' 's32min-s32max'
drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c:2005 qlcnic_sriov_get_vf_config() error: buffer underflow 'sriov->vf_info' 's32min-254'
drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c:1832 qlcnic_sriov_set_vf_mac() error: buffer underflow 'sriov->vf_info' 's32min-254'
drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c:2036 qlcnic_sriov_set_vf_spoofchk() error: buffer underflow 'sriov->vf_info' 's32min-254'
drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c:1864 qlcnic_sriov_set_vf_tx_rate() error: buffer underflow 'sriov->vf_info' 's32min-254'
drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c:1937 qlcnic_sriov_set_vf_vlan() error: buffer underflow 'sriov->vf_info' 's32min-254'

Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Dan Carpenter]

I think I'm just going to ask netdev for an opinion on this.  It could
be that we're just reading the code wrong...

I'm getting a lot of Smatch warning about buffer underflows.  The
problem is that Smatch marks everything from nla_data() as unknown and
untrusted user data.  In do_setvfinfo() we get the "->vf" values from
nla_data().  It starts as u32, but all the function pointers in
net_device_ops use it as a signed integer.  Most of the functions return
-EINVAL if "vf" is negative but there are at least 48 which potentially
use negative values as an offset into an array.

To me making "vf" a u32 throughout seems like a good idea but it's an
extensive patch and I'm not really able to test it at all.  But maybe
there is a better place to check for negatives.  Or maybe we are already
checking for negatives and I haven't seen it.  (I don't know this code
very well at all).

regards,
dan carpenter

drivers/net/ethernet/emulex/benet/be_main.c:1955 be_clear_vf_tvt() error: buffer underflow 'adapter->vf_cfg' 's32min-65534'
drivers/net/ethernet/emulex/benet/be_main.c:1904 be_get_vf_config() error: buffer underflow 'adapter->vf_cfg' 's32min-s32max'
drivers/net/ethernet/emulex/benet/be_main.c:2095 be_set_vf_link_state() error: buffer underflow 'adapter->vf_cfg' 's32min-65534'
drivers/net/ethernet/emulex/benet/be_main.c:1863 be_set_vf_mac() error: buffer underflow 'adapter->vf_cfg' 's32min-s32max'
drivers/net/ethernet/emulex/benet/be_main.c:2103 be_set_vf_spoofchk() error: buffer underflow 'adapter->vf_cfg' 's32min-s32max'
drivers/net/ethernet/emulex/benet/be_main.c:1926 be_set_vf_tvt() error: buffer underflow 'adapter->vf_cfg' 's32min-65534'
drivers/net/ethernet/emulex/benet/be_main.c:2067 be_set_vf_tx_rate() error: buffer underflow 'adapter->vf_cfg' 's32min-65534'
drivers/net/ethernet/emulex/benet/be_main.c:1984 be_set_vf_vlan() error: buffer underflow 'adapter->vf_cfg' 's32min-s32max'
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c:2281 bnx2x_post_vf_bulletin() error: buffer underflow 'bp->vfdb->vfs' 's32min-63'
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c:1523 bnx2x_set_vf_link_state() error: buffer underflow 'bp->vfdb->vfs' 's32min-s32max'
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c:2963 bnx2x_set_vf_spoofchk() error: buffer underflow 'bp->vfdb->vfs' 's32min-s32max'
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c:2589 bnx2x_vf_op_prep() error: buffer underflow 'bp->vfdb->vfs' 's32min-65534'
drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c:202 bnxt_get_vf_config() error: buffer underflow 'bp->pf.vf' 's32min-65534'
drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c:309 bnxt_set_vf_bw() error: buffer underflow 'bp->pf.vf' 's32min-65534'
drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c:349 bnxt_set_vf_link_state() error: buffer underflow 'bp->pf.vf' 's32min-65534'
drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c:244 bnxt_set_vf_mac() error: buffer underflow 'bp->pf.vf' 's32min-65534'
drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c:96 bnxt_set_vf_spoofchk() error: buffer underflow 'bp->pf.vf' 's32min-65534'
drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c:180 bnxt_set_vf_trust() error: buffer underflow 'bp->pf.vf' 's32min-65534'
drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c:280 bnxt_set_vf_vlan() error: buffer underflow 'bp->pf.vf' 's32min-65534'
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c:2736 cxgb4_mgmt_get_vf_config() error: buffer underflow 'adap->vfinfo' 's32min-254'
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c:2923 cxgb4_mgmt_set_vf_link_state() error: buffer underflow 'adap->vfinfo' 's32min-254'
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c:2723 cxgb4_mgmt_set_vf_mac() error: buffer underflow 'adap->vfinfo' 's32min-s32max'
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c:2797 cxgb4_mgmt_set_vf_rate() error: buffer underflow 'adap->vfinfo' 's32min-254'
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c:2875 cxgb4_mgmt_set_vf_vlan() error: buffer underflow 'adap->vfinfo' 's32min-254'
drivers/net/ethernet/freescale/enetc/enetc_pf.c:377 enetc_pf_set_vf_mac() error: buffer underflow 'pf->vf_state' 's32min-2147483646'
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c:7069 hclge_set_vf_vlan_filter() error: buffer underflow 'hdev->vport' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:4223 i40e_ndo_get_vf_config() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:4177 i40e_ndo_set_vf_bw() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:4287 i40e_ndo_set_vf_link_state() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:3895 i40e_ndo_set_vf_mac() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:4041 i40e_ndo_set_vf_port_vlan() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:4357 i40e_ndo_set_vf_spoofchk() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:4420 i40e_ndo_set_vf_trust() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:3862 i40e_validate_vf() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c:2678 ice_get_vf_cfg() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c:2879 ice_set_vf_link_state() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c:2792 ice_set_vf_mac() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c:2246 ice_set_vf_port_vlan() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c:2731 ice_set_vf_spoofchk() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c:2839 ice_set_vf_trust() error: buffer underflow 'pf->vf' 's32min-2147483646'
drivers/infiniband/hw/mlx5/ib_virt.c:114 mlx5_ib_set_vf_link_state() error: buffer underflow 'vfs_ctx' 's32min-s32max'
drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c:2005 qlcnic_sriov_get_vf_config() error: buffer underflow 'sriov->vf_info' 's32min-254'
drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c:1832 qlcnic_sriov_set_vf_mac() error: buffer underflow 'sriov->vf_info' 's32min-254'
drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c:2036 qlcnic_sriov_set_vf_spoofchk() error: buffer underflow 'sriov->vf_info' 's32min-254'
drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c:1864 qlcnic_sriov_set_vf_tx_rate() error: buffer underflow 'sriov->vf_info' 's32min-254'
drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c:1937 qlcnic_sriov_set_vf_vlan() error: buffer underflow 'sriov->vf_info' 's32min-254'

Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Jason Gunthorpe]

On Wed, Apr 24, 2019 at 05:08:20PM +0300, Dan Carpenter wrote:
> I think I'm just going to ask netdev for an opinion on this.  It could
> be that we're just reading the code wrong...

I don't think you are reading it wrong.

Allowing the compiler to implicitly cast a user controlled u32 to an
int is simply wrong in all cases, IMHO. 

If the value was intended to be signed from the user it should have
been a s32. Allowing an unsigned value to become interpreted as
negative so often leads to security bugs.

IMHO it would be a good thing for smatch to warn on the general case
of implicit casting of user controlled data to a smaller range
type. Particularly it can do a bounds analysis to show the control
flow hasn't somehow restricted the bounds to be compatible.

I've seen more that a few real world security bugs that are caused by
wrong use of 'int' like this :(

Jason

Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Jason Gunthorpe]

On Wed, Apr 24, 2019 at 05:08:20PM +0300, Dan Carpenter wrote:
> I think I'm just going to ask netdev for an opinion on this.  It could
> be that we're just reading the code wrong...

I don't think you are reading it wrong.

Allowing the compiler to implicitly cast a user controlled u32 to an
int is simply wrong in all cases, IMHO. 

If the value was intended to be signed from the user it should have
been a s32. Allowing an unsigned value to become interpreted as
negative so often leads to security bugs.

IMHO it would be a good thing for smatch to warn on the general case
of implicit casting of user controlled data to a smaller range
type. Particularly it can do a bounds analysis to show the control
flow hasn't somehow restricted the bounds to be compatible.

I've seen more that a few real world security bugs that are caused by
wrong use of 'int' like this :(

Jason

RE: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Parav Pandit]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, April 24, 2019 9:24 AM
> To: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Parav Pandit <parav@mellanox.com>; netdev@vger.kernel.org; Leon
> Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>; Doug
> Ledford <dledford@redhat.com>; linux-rdma@vger.kernel.org; kernel-
> janitors@vger.kernel.org
> Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> 
> On Wed, Apr 24, 2019 at 05:08:20PM +0300, Dan Carpenter wrote:
> > I think I'm just going to ask netdev for an opinion on this.  It could
> > be that we're just reading the code wrong...
> 
> I don't think you are reading it wrong.
> 
> Allowing the compiler to implicitly cast a user controlled u32 to an int is
> simply wrong in all cases, IMHO.
> 
> If the value was intended to be signed from the user it should have been a
> s32. Allowing an unsigned value to become interpreted as negative so often
> leads to security bugs.
> 
> IMHO it would be a good thing for smatch to warn on the general case of
> implicit casting of user controlled data to a smaller range type. Particularly it
> can do a bounds analysis to show the control flow hasn't somehow
> restricted the bounds to be compatible.
> 
> I've seen more that a few real world security bugs that are caused by wrong
> use of 'int' like this :(
> 
> Jason

Hence we should fix the type to be u32 in ndo ops to match netlink type core and in driver, instead of < 0 check.

Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Jakub Kicinski]

On Wed, 24 Apr 2019 17:08:20 +0300, Dan Carpenter wrote:
> To me making "vf" a u32 throughout seems like a good idea but it's an
> extensive patch and I'm not really able to test it at all.  But maybe
> there is a better place to check for negatives.  Or maybe we are already
> checking for negatives and I haven't seen it.  (I don't know this code
> very well at all).

Could we please add the checks in the core?

We already have the infra in place for calculating dump sizes - i.e.

int num_vfs = dev_num_vf(dev->dev.parent);

We just need to validate the set params.


Callback parameters should really be validated by the core to the extent
possible, unfortunately driver authors have little incentive to improve
that once an API is implemented, realistically we all need to support
old kernels wouldn't do the checking..

Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Jakub Kicinski]

On Wed, 24 Apr 2019 17:08:20 +0300, Dan Carpenter wrote:
> To me making "vf" a u32 throughout seems like a good idea but it's an
> extensive patch and I'm not really able to test it at all.  But maybe
> there is a better place to check for negatives.  Or maybe we are already
> checking for negatives and I haven't seen it.  (I don't know this code
> very well at all).

Could we please add the checks in the core?

We already have the infra in place for calculating dump sizes - i.e.

int num_vfs = dev_num_vf(dev->dev.parent);

We just need to validate the set params.


Callback parameters should really be validated by the core to the extent
possible, unfortunately driver authors have little incentive to improve
that once an API is implemented, realistically we all need to support
old kernels wouldn't do the checking..

RE: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Parav Pandit]

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Wednesday, April 24, 2019 9:08 AM
> To: Parav Pandit <parav@mellanox.com>; netdev@vger.kernel.org
> Cc: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>;
> Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> 
> I think I'm just going to ask netdev for an opinion on this.  It could be that
> we're just reading the code wrong...
> 
> I'm getting a lot of Smatch warning about buffer underflows.  The problem is
> that Smatch marks everything from nla_data() as unknown and untrusted
> user data.  In do_setvfinfo() we get the "->vf" values from nla_data().  It
> starts as u32, but all the function pointers in net_device_ops use it as a
> signed integer.  Most of the functions return -EINVAL if "vf" is negative but
> there are at least 48 which potentially use negative values as an offset into
> an array.
> 
> To me making "vf" a u32 throughout seems like a good idea but it's an
> extensive patch and I'm not really able to test it at all.  

I will be try to get you patch early next week for core and in mlx5, tested on mlx5 VFs, that possibly you can carry forward?

Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Dan Carpenter]

On Thu, Apr 25, 2019 at 06:15:13AM +0000, Parav Pandit wrote:
> 
> 
> > -----Original Message-----
> > From: Dan Carpenter <dan.carpenter@oracle.com>
> > Sent: Wednesday, April 24, 2019 9:08 AM
> > To: Parav Pandit <parav@mellanox.com>; netdev@vger.kernel.org
> > Cc: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>;
> > Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> > linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> > Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> > 
> > I think I'm just going to ask netdev for an opinion on this.  It could be that
> > we're just reading the code wrong...
> > 
> > I'm getting a lot of Smatch warning about buffer underflows.  The problem is
> > that Smatch marks everything from nla_data() as unknown and untrusted
> > user data.  In do_setvfinfo() we get the "->vf" values from nla_data().  It
> > starts as u32, but all the function pointers in net_device_ops use it as a
> > signed integer.  Most of the functions return -EINVAL if "vf" is negative but
> > there are at least 48 which potentially use negative values as an offset into
> > an array.
> > 
> > To me making "vf" a u32 throughout seems like a good idea but it's an
> > extensive patch and I'm not really able to test it at all.
> 
> I will be try to get you patch early next week for core and in mlx5,
> tested on mlx5 VFs, that possibly you can carry forward?

Whatever happened with this?

regards,
dan carpenter

Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Dan Carpenter]

On Thu, Apr 25, 2019 at 06:15:13AM +0000, Parav Pandit wrote:
> 
> 
> > -----Original Message-----
> > From: Dan Carpenter <dan.carpenter@oracle.com>
> > Sent: Wednesday, April 24, 2019 9:08 AM
> > To: Parav Pandit <parav@mellanox.com>; netdev@vger.kernel.org
> > Cc: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>;
> > Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> > linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> > Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> > 
> > I think I'm just going to ask netdev for an opinion on this.  It could be that
> > we're just reading the code wrong...
> > 
> > I'm getting a lot of Smatch warning about buffer underflows.  The problem is
> > that Smatch marks everything from nla_data() as unknown and untrusted
> > user data.  In do_setvfinfo() we get the "->vf" values from nla_data().  It
> > starts as u32, but all the function pointers in net_device_ops use it as a
> > signed integer.  Most of the functions return -EINVAL if "vf" is negative but
> > there are at least 48 which potentially use negative values as an offset into
> > an array.
> > 
> > To me making "vf" a u32 throughout seems like a good idea but it's an
> > extensive patch and I'm not really able to test it at all.
> 
> I will be try to get you patch early next week for core and in mlx5,
> tested on mlx5 VFs, that possibly you can carry forward?

Whatever happened with this?

regards,
dan carpenter

RE: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()

[Parav Pandit]

Hi Dan,

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Tuesday, September 24, 2019 4:21 AM
> To: Parav Pandit <parav@mellanox.com>
> Cc: netdev@vger.kernel.org; Leon Romanovsky <leon@kernel.org>; Eli Cohen
> <eli@mellanox.com>; Doug Ledford <dledford@redhat.com>; Jason Gunthorpe
> <jgg@ziepe.ca>; linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> 
> On Thu, Apr 25, 2019 at 06:15:13AM +0000, Parav Pandit wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dan Carpenter <dan.carpenter@oracle.com>
> > > Sent: Wednesday, April 24, 2019 9:08 AM
> > > To: Parav Pandit <parav@mellanox.com>; netdev@vger.kernel.org
> > > Cc: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>;
> > > Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> > > linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> > > Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from
> > > do_setvfinfo()
> > >
> > > I think I'm just going to ask netdev for an opinion on this.  It
> > > could be that we're just reading the code wrong...
> > >
> > > I'm getting a lot of Smatch warning about buffer underflows.  The
> > > problem is that Smatch marks everything from nla_data() as unknown
> > > and untrusted user data.  In do_setvfinfo() we get the "->vf" values
> > > from nla_data().  It starts as u32, but all the function pointers in
> > > net_device_ops use it as a signed integer.  Most of the functions
> > > return -EINVAL if "vf" is negative but there are at least 48 which
> > > potentially use negative values as an offset into an array.
> > >
> > > To me making "vf" a u32 throughout seems like a good idea but it's
> > > an extensive patch and I'm not really able to test it at all.
> >
> > I will be try to get you patch early next week for core and in mlx5,
> > tested on mlx5 VFs, that possibly you can carry forward?
> 
> Whatever happened with this?
> 
I had internal few patches that Leon and Saeed reviewed, but it needs more rework at core and driver level.
I haven't had chance to finish it.

> regards,
> dan carpenter