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

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

[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()

[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()

[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