[PATCH v2] ixgbe: check adapter->vfinfo before dereference

[PATCH v2] ixgbe: check adapter->vfinfo before dereference

[Thierry Herbelot]

this protects against the following panic:
(before a VF was actually created on p96p1 PF Ethernet port)
ip link set p96p1 vf 0 spoofchk off
BUG: unable to handle kernel NULL pointer dereference at 0000000000000052
IP: [<ffffffffa044a1c1>] ixgbe_ndo_set_vf_spoofchk+0x51/0x150 [ixgbe]

Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |   73 +++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index 706fc69..29279ad 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -316,7 +316,7 @@ static int ixgbe_set_vf_multicasts(struct ixgbe_adapter *adapter,
 	int entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK)
 		       >> IXGBE_VT_MSGINFO_SHIFT;
 	u16 *hash_list = (u16 *)&msgbuf[1];
-	struct vf_data_storage *vfinfo = &adapter->vfinfo[vf];
+	struct vf_data_storage *vfinfo;
 	struct ixgbe_hw *hw = &adapter->hw;
 	int i;
 	u32 vector_bit;
@@ -324,6 +324,11 @@ static int ixgbe_set_vf_multicasts(struct ixgbe_adapter *adapter,
 	u32 mta_reg;
 	u32 vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf));
 
+	if (!adapter->vfinfo)
+		return -1;
+
+	vfinfo = &adapter->vfinfo[vf];
+
 	/* only so many hash values supported */
 	entries = min(entries, IXGBE_MAX_VF_MC_ENTRIES);
 
@@ -365,6 +370,9 @@ void ixgbe_restore_vf_multicasts(struct ixgbe_adapter *adapter)
 	u32 vector_reg;
 	u32 mta_reg;
 
+	if (!adapter->vfinfo)
+		return;
+
 	for (i = 0; i < adapter->num_vfs; i++) {
 		u32 vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(i));
 		vfinfo = &adapter->vfinfo[i];
@@ -418,6 +426,9 @@ static s32 ixgbe_set_vf_lpe(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
 		u32 reg_offset, vf_shift, vfre;
 		s32 err = 0;
 
+		if (!adapter->vfinfo)
+			return -1;
+
 #ifdef CONFIG_FCOE
 		if (dev->features & NETIF_F_FCOE_MTU)
 			pf_max_frame = max_t(int, pf_max_frame,
@@ -507,6 +518,9 @@ static inline void ixgbe_vf_reset_event(struct ixgbe_adapter *adapter, u32 vf)
 	struct vf_data_storage *vfinfo = &adapter->vfinfo[vf];
 	u8 num_tcs = netdev_get_num_tc(adapter->netdev);
 
+	if (!adapter->vfinfo)
+		return;
+
 	/* add PF assigned VLAN or VLAN 0 */
 	ixgbe_set_vf_vlan(adapter, true, vfinfo->pf_vlan, vf);
 
@@ -543,6 +557,8 @@ static inline void ixgbe_vf_reset_event(struct ixgbe_adapter *adapter, u32 vf)
 static int ixgbe_set_vf_mac(struct ixgbe_adapter *adapter,
 			    int vf, unsigned char *mac_addr)
 {
+	if (!adapter->vfinfo)
+		return -1;
 	ixgbe_del_mac_filter(adapter, adapter->vfinfo[vf].vf_mac_addresses, vf);
 	memcpy(adapter->vfinfo[vf].vf_mac_addresses, mac_addr, ETH_ALEN);
 	ixgbe_add_mac_filter(adapter, adapter->vfinfo[vf].vf_mac_addresses, vf);
@@ -612,6 +628,9 @@ int ixgbe_vf_configuration(struct pci_dev *pdev, unsigned int event_mask)
 
 	bool enable = ((event_mask & 0x10000000U) != 0);
 
+	if (!adapter->vfinfo)
+		return -1;
+
 	if (enable)
 		eth_zero_addr(adapter->vfinfo[vfn].vf_mac_addresses);
 
@@ -622,13 +641,18 @@ static int ixgbe_vf_reset_msg(struct ixgbe_adapter *adapter, u32 vf)
 {
 	struct ixgbe_ring_feature *vmdq = &adapter->ring_feature[RING_F_VMDQ];
 	struct ixgbe_hw *hw = &adapter->hw;
-	unsigned char *vf_mac = adapter->vfinfo[vf].vf_mac_addresses;
+	unsigned char *vf_mac;
 	u32 reg, reg_offset, vf_shift;
 	u32 msgbuf[4] = {0, 0, 0, 0};
 	u8 *addr = (u8 *)(&msgbuf[1]);
 	u32 q_per_pool = __ALIGN_MASK(1, ~vmdq->mask);
 	int i;
 
+	if (!adapter->vfinfo)
+		return -1;
+
+	vf_mac = adapter->vfinfo[vf].vf_mac_addresses;
+
 	e_info(probe, "VF Reset msg received from vf %d\n", vf);
 
 	/* reset the filters for the device */
@@ -723,6 +747,9 @@ static int ixgbe_set_vf_mac_addr(struct ixgbe_adapter *adapter,
 {
 	u8 *new_mac = ((u8 *)(&msgbuf[1]));
 
+	if (!adapter->vfinfo)
+		return -1;
+
 	if (!is_valid_ether_addr(new_mac)) {
 		e_warn(drv, "VF %d attempted to set invalid mac\n", vf);
 		return -1;
@@ -775,6 +802,9 @@ static int ixgbe_set_vf_vlan_msg(struct ixgbe_adapter *adapter,
 	u32 bits;
 	u8 tcs = netdev_get_num_tc(adapter->netdev);
 
+	if (!adapter->vfinfo)
+		return -1;
+
 	if (adapter->vfinfo[vf].pf_vlan || tcs) {
 		e_warn(drv,
 		       "VF %d attempted to override administratively set VLAN configuration\n"
@@ -841,6 +871,9 @@ static int ixgbe_set_vf_macvlan_msg(struct ixgbe_adapter *adapter,
 		    IXGBE_VT_MSGINFO_SHIFT;
 	int err;
 
+	if (!adapter->vfinfo)
+		return -1;
+
 	if (adapter->vfinfo[vf].pf_set_mac && index > 0) {
 		e_warn(drv,
 		       "VF %d requested MACVLAN filter but is administratively denied\n",
@@ -877,6 +910,9 @@ static int ixgbe_negotiate_vf_api(struct ixgbe_adapter *adapter,
 {
 	int api = msgbuf[1];
 
+	if (!adapter->vfinfo)
+		return -1;
+
 	switch (api) {
 	case ixgbe_mbox_api_10:
 	case ixgbe_mbox_api_11:
@@ -899,6 +935,9 @@ static int ixgbe_get_vf_queues(struct ixgbe_adapter *adapter,
 	unsigned int default_tc = 0;
 	u8 num_tcs = netdev_get_num_tc(dev);
 
+	if (!adapter->vfinfo)
+		return -1;
+
 	/* verify the PF is supporting the correct APIs */
 	switch (adapter->vfinfo[vf].vf_api) {
 	case ixgbe_mbox_api_20:
@@ -937,6 +976,9 @@ static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 	struct ixgbe_hw *hw = &adapter->hw;
 	s32 retval;
 
+	if (!adapter->vfinfo)
+		return -1;
+
 	retval = ixgbe_read_mbx(hw, msgbuf, mbx_size, vf);
 
 	if (retval) {
@@ -1010,6 +1052,9 @@ static void ixgbe_rcv_ack_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 msg = IXGBE_VT_MSGTYPE_NACK;
 
+	if (!adapter->vfinfo)
+		return;
+
 	/* if device isn't clear to send it shouldn't be reading either */
 	if (!adapter->vfinfo[vf].clear_to_send)
 		ixgbe_write_mbx(hw, &msg, 1, vf);
@@ -1053,6 +1098,9 @@ void ixgbe_ping_all_vfs(struct ixgbe_adapter *adapter)
 	u32 ping;
 	int i;
 
+	if (!adapter->vfinfo)
+		return;
+
 	for (i = 0 ; i < adapter->num_vfs; i++) {
 		ping = IXGBE_PF_CONTROL_MSG;
 		if (adapter->vfinfo[i].clear_to_send)
@@ -1066,6 +1114,9 @@ int ixgbe_ndo_set_vf_mac(struct net_device *netdev, int vf, u8 *mac)
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	if (!is_valid_ether_addr(mac) || (vf >= adapter->num_vfs))
 		return -EINVAL;
+	if (!adapter->vfinfo)
+		return -1;
+
 	adapter->vfinfo[vf].pf_set_mac = true;
 	dev_info(&adapter->pdev->dev, "setting MAC %pM on VF %d\n", mac, vf);
 	dev_info(&adapter->pdev->dev, "Reload the VF driver to make this"
@@ -1085,6 +1136,9 @@ int ixgbe_ndo_set_vf_vlan(struct net_device *netdev, int vf, u16 vlan, u8 qos)
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	struct ixgbe_hw *hw = &adapter->hw;
 
+	if (!adapter->vfinfo)
+		return -1;
+
 	if ((vf >= adapter->num_vfs) || (vlan > 4095) || (qos > 7))
 		return -EINVAL;
 	if (vlan || qos) {
@@ -1149,8 +1203,12 @@ static void ixgbe_set_vf_rate_limit(struct ixgbe_adapter *adapter, int vf)
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 bcnrc_val = 0;
 	u16 queue, queues_per_pool;
-	u16 tx_rate = adapter->vfinfo[vf].tx_rate;
+	u16 tx_rate;
 
+	if (!adapter->vfinfo)
+		return;
+
+	tx_rate = adapter->vfinfo[vf].tx_rate;
 	if (tx_rate) {
 		/* start with base link speed value */
 		bcnrc_val = adapter->vf_rate_link_speed;
@@ -1199,6 +1257,9 @@ void ixgbe_check_vf_rate_limit(struct ixgbe_adapter *adapter)
 {
 	int i;
 
+	if (!adapter->vfinfo)
+		return;
+
 	/* VF Tx rate limit was not set */
 	if (!adapter->vf_rate_link_speed)
 		return;
@@ -1261,6 +1322,9 @@ int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting)
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 regval;
 
+	if (!adapter->vfinfo)
+		return -1;
+
 	adapter->vfinfo[vf].spoofchk_enabled = setting;
 
 	regval = IXGBE_READ_REG(hw, IXGBE_PFVFSPOOF(vf_target_reg));
@@ -1285,6 +1349,9 @@ int ixgbe_ndo_get_vf_config(struct net_device *netdev,
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	if (vf >= adapter->num_vfs)
 		return -EINVAL;
+	if (!adapter->vfinfo)
+		return -EINVAL;
+
 	ivi->vf = vf;
 	memcpy(&ivi->mac, adapter->vfinfo[vf].vf_mac_addresses, ETH_ALEN);
 	ivi->max_tx_rate = adapter->vfinfo[vf].tx_rate;
-- 
1.7.10.4

Re: [PATCH v2] ixgbe: check adapter->vfinfo before dereference

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

On Fri, 2014-10-10 at 10:45 +0200, Thierry Herbelot wrote:
> this protects against the following panic:
> (before a VF was actually created on p96p1 PF Ethernet port)
> ip link set p96p1 vf 0 spoofchk off
> BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000052
> IP: [<ffffffffa044a1c1>] ixgbe_ndo_set_vf_spoofchk+0x51/0x150 [ixgbe]
> 
> Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |   73
> +++++++++++++++++++++++-
>  1 file changed, 70 insertions(+), 3 deletions(-)

Thanks for fixing that up Thierry.  I have added your patch to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] ixgbe: check adapter->vfinfo before dereference

[Thierry Herbelot]

On 10/10/2014 10:50 AM, Jeff Kirsher wrote:
> On Fri, 2014-10-10 at 10:45 +0200, Thierry Herbelot wrote:
>> this protects against the following panic:
>> (before a VF was actually created on p96p1 PF Ethernet port)
>> ip link set p96p1 vf 0 spoofchk off
>> BUG: unable to handle kernel NULL pointer dereference at
>> 0000000000000052
>> IP: [<ffffffffa044a1c1>] ixgbe_ndo_set_vf_spoofchk+0x51/0x150 [ixgbe]
>>
>> Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>
>> ---
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |   73
>> +++++++++++++++++++++++-
>>   1 file changed, 70 insertions(+), 3 deletions(-)
>
> Thanks for fixing that up Thierry.  I have added your patch to my queue.

Sorry for the miscompile : I wrote the patch back in August, and left it 
to rot while doing other things and I did not check it was really 
correct before sending it yesterday.

	Thierry

>


-- 
Thierry Herbelot
6WIND
Software Engineer

RE: [PATCH v2] ixgbe: check adapter->vfinfo before dereference

[Tantilov, Emil S]

>-----Original Message-----
>From: netdev-owner@vger.kernel.org [mailto:netdev-
>owner@vger.kernel.org] On Behalf Of Thierry Herbelot
>Sent: Friday, October 10, 2014 1:46 AM
>To: Kirsher, Jeffrey T; Brandeburg, Jesse; Allan, Bruce W;
>netdev@vger.kernel.org
>Cc: Thierry Herbelot
>Subject: [PATCH v2] ixgbe: check adapter->vfinfo before
>dereference
>
>this protects against the following panic:
>(before a VF was actually created on p96p1 PF Ethernet port)
>ip link set p96p1 vf 0 spoofchk off
>BUG: unable to handle kernel NULL pointer dereference at
>0000000000000052
>IP: [<ffffffffa044a1c1>]
>ixgbe_ndo_set_vf_spoofchk+0x51/0x150 [ixgbe]
>
>Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>
>---
> drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |   73
>+++++++++++++++++++++++-
> 1 file changed, 70 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>index 706fc69..29279ad 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>@@ -316,7 +316,7 @@ static int ixgbe_set_vf_multicasts(struct ixgbe_adapter *adapter,
> 	int entries = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK)
> 		       >> IXGBE_VT_MSGINFO_SHIFT;
> 	u16 *hash_list = (u16 *)&msgbuf[1];
>-	struct vf_data_storage *vfinfo = &adapter->vfinfo[vf];
>+	struct vf_data_storage *vfinfo;
> 	struct ixgbe_hw *hw = &adapter->hw;
> 	int i;
> 	u32 vector_bit;
>@@ -324,6 +324,11 @@ static int
>ixgbe_set_vf_multicasts(struct ixgbe_adapter *adapter,
> 	u32 mta_reg;
> 	u32 vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf));
>
>+	if (!adapter->vfinfo)
>+		return -1;
>+
>+	vfinfo = &adapter->vfinfo[vf];

This check makes sense for the ndo functions that get called by the ip command, but I don't think we need to add them before every use of adapter->vfinfo. In this case for example ixgbe_set_vf_multicasts() is called from
ixgbe_rcv_msg_from_vf() which will be called when an actual VF exists.

Also for the error -EINVAL probably makes more sense than -1.

Thanks,
Emil

[PATCH v3 net] ixgbe: check adapter->vfinfo before dereference

[Thierry Herbelot]

this protects against the following panic:
(before a VF was actually created on p96p1 PF Ethernet port)

ip link set p96p1 vf 0 spoofchk off
BUG: unable to handle kernel NULL pointer dereference at 0000000000000052
IP: [<ffffffffa044a1c1>] ixgbe_ndo_set_vf_spoofchk+0x51/0x150 [ixgbe]

Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>
---

v2:
  compilation fixes

v3:
  remove checks in functions where vfinfo is known not to be NULL
  return -EINVAL as error code

 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |   42 ++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index 706fc69..fab0157 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -365,6 +365,9 @@ void ixgbe_restore_vf_multicasts(struct ixgbe_adapter *adapter)
 	u32 vector_reg;
 	u32 mta_reg;
 
+	if (!adapter->vfinfo)
+		return;
+
 	for (i = 0; i < adapter->num_vfs; i++) {
 		u32 vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(i));
 		vfinfo = &adapter->vfinfo[i];
@@ -504,9 +507,13 @@ static void ixgbe_clear_vmvir(struct ixgbe_adapter *adapter, u32 vf)
 static inline void ixgbe_vf_reset_event(struct ixgbe_adapter *adapter, u32 vf)
 {
 	struct ixgbe_hw *hw = &adapter->hw;
-	struct vf_data_storage *vfinfo = &adapter->vfinfo[vf];
+	struct vf_data_storage *vfinfo;
 	u8 num_tcs = netdev_get_num_tc(adapter->netdev);
 
+	if (!adapter->vfinfo)
+		return;
+	vfinfo = &adapter->vfinfo[vf];
+
 	/* add PF assigned VLAN or VLAN 0 */
 	ixgbe_set_vf_vlan(adapter, true, vfinfo->pf_vlan, vf);
 
@@ -612,6 +619,9 @@ int ixgbe_vf_configuration(struct pci_dev *pdev, unsigned int event_mask)
 
 	bool enable = ((event_mask & 0x10000000U) != 0);
 
+	if (!adapter->vfinfo)
+		return -EINVAL;
+
 	if (enable)
 		eth_zero_addr(adapter->vfinfo[vfn].vf_mac_addresses);
 
@@ -937,6 +947,9 @@ static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 	struct ixgbe_hw *hw = &adapter->hw;
 	s32 retval;
 
+	if (!adapter->vfinfo)
+		return -EINVAL;
+
 	retval = ixgbe_read_mbx(hw, msgbuf, mbx_size, vf);
 
 	if (retval) {
@@ -1010,6 +1023,9 @@ static void ixgbe_rcv_ack_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 msg = IXGBE_VT_MSGTYPE_NACK;
 
+	if (!adapter->vfinfo)
+		return;
+
 	/* if device isn't clear to send it shouldn't be reading either */
 	if (!adapter->vfinfo[vf].clear_to_send)
 		ixgbe_write_mbx(hw, &msg, 1, vf);
@@ -1053,6 +1069,9 @@ void ixgbe_ping_all_vfs(struct ixgbe_adapter *adapter)
 	u32 ping;
 	int i;
 
+	if (!adapter->vfinfo)
+		return;
+
 	for (i = 0 ; i < adapter->num_vfs; i++) {
 		ping = IXGBE_PF_CONTROL_MSG;
 		if (adapter->vfinfo[i].clear_to_send)
@@ -1066,6 +1085,9 @@ int ixgbe_ndo_set_vf_mac(struct net_device *netdev, int vf, u8 *mac)
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	if (!is_valid_ether_addr(mac) || (vf >= adapter->num_vfs))
 		return -EINVAL;
+	if (!adapter->vfinfo)
+		return -EINVAL;
+
 	adapter->vfinfo[vf].pf_set_mac = true;
 	dev_info(&adapter->pdev->dev, "setting MAC %pM on VF %d\n", mac, vf);
 	dev_info(&adapter->pdev->dev, "Reload the VF driver to make this"
@@ -1085,6 +1107,9 @@ int ixgbe_ndo_set_vf_vlan(struct net_device *netdev, int vf, u16 vlan, u8 qos)
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	struct ixgbe_hw *hw = &adapter->hw;
 
+	if (!adapter->vfinfo)
+		return -EINVAL;
+
 	if ((vf >= adapter->num_vfs) || (vlan > 4095) || (qos > 7))
 		return -EINVAL;
 	if (vlan || qos) {
@@ -1149,7 +1174,11 @@ static void ixgbe_set_vf_rate_limit(struct ixgbe_adapter *adapter, int vf)
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 bcnrc_val = 0;
 	u16 queue, queues_per_pool;
-	u16 tx_rate = adapter->vfinfo[vf].tx_rate;
+	u16 tx_rate;
+
+	if (!adapter->vfinfo)
+		return;
+	tx_rate = adapter->vfinfo[vf].tx_rate;
 
 	if (tx_rate) {
 		/* start with base link speed value */
@@ -1199,6 +1228,9 @@ void ixgbe_check_vf_rate_limit(struct ixgbe_adapter *adapter)
 {
 	int i;
 
+	if (!adapter->vfinfo)
+		return;
+
 	/* VF Tx rate limit was not set */
 	if (!adapter->vf_rate_link_speed)
 		return;
@@ -1261,6 +1293,9 @@ int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting)
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 regval;
 
+	if (!adapter->vfinfo)
+		return -EINVAL;
+
 	adapter->vfinfo[vf].spoofchk_enabled = setting;
 
 	regval = IXGBE_READ_REG(hw, IXGBE_PFVFSPOOF(vf_target_reg));
@@ -1285,6 +1320,9 @@ int ixgbe_ndo_get_vf_config(struct net_device *netdev,
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	if (vf >= adapter->num_vfs)
 		return -EINVAL;
+	if (!adapter->vfinfo)
+		return -EINVAL;
+
 	ivi->vf = vf;
 	memcpy(&ivi->mac, adapter->vfinfo[vf].vf_mac_addresses, ETH_ALEN);
 	ivi->max_tx_rate = adapter->vfinfo[vf].tx_rate;
-- 
1.7.10.4

Re: [PATCH v3 net] ixgbe: check adapter->vfinfo before dereference

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 811 bytes --]

On Wed, 2014-10-15 at 11:58 +0200, Thierry Herbelot wrote:
> this protects against the following panic:
> (before a VF was actually created on p96p1 PF Ethernet port)
> 
> ip link set p96p1 vf 0 spoofchk off
> BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000052
> IP: [<ffffffffa044a1c1>] ixgbe_ndo_set_vf_spoofchk+0x51/0x150 [ixgbe]
> 
> Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>
> ---
> 
> v2:
>   compilation fixes
> 
> v3:
>   remove checks in functions where vfinfo is known not to be NULL
>   return -EINVAL as error code
> 
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |   42
> ++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)

Thanks Thierry, I have added this patch to my queue (and dropped your
v2).

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH v3 net] ixgbe: check adapter->vfinfo before dereference

[Tantilov, Emil S]

>-----Original Message-----
>From: Thierry Herbelot [mailto:thierry.herbelot@6wind.com]
>Sent: Wednesday, October 15, 2014 2:58 AM
>To: Kirsher, Jeffrey T; Brandeburg, Jesse; Allan, Bruce W;
>netdev@vger.kernel.org; Tantilov, Emil S
>Cc: Thierry Herbelot
>Subject: [PATCH v3 net] ixgbe: check adapter->vfinfo before dereference
>
>this protects against the following panic:
>(before a VF was actually created on p96p1 PF Ethernet port)
>
>ip link set p96p1 vf 0 spoofchk off
>BUG: unable to handle kernel NULL pointer dereference at 0000000000000052
>IP: [<ffffffffa044a1c1>]
>ixgbe_ndo_set_vf_spoofchk+0x51/0x150 [ixgbe]
>
>Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>
>---
>
>v2:
>  compilation fixes
>
>v3:
>  remove checks in functions where vfinfo is known not to be NULL
>  return -EINVAL as error code
>
> drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |   42
>++++++++++++++++++++++--
> 1 file changed, 40 insertions(+), 2 deletions(-)

Actually looking into this a bit more, the check for vfinfo is not sufficient
because it does not protect against specifying vf that is outside of sriov_num_vfs range.

All of the ndo functions have a check for it except for ixgbevf_ndo_set_spoofcheck().

The following patch should be all we need to protect against this panic:

This patch adds a check to return -EINVAL when setting spoofcheck on
VF that is not configured.

Reported-by: Thierry Herbelot <thierry.herbelot@6wind.com>
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index 706fc69..97c85b8 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -1261,6 +1261,9 @@ int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting)
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 regval;
 
+	if (vf >= adapter->num_vfs)
+		return -EINVAL;
+
 	adapter->vfinfo[vf].spoofchk_enabled = setting;
 
 	regval = IXGBE_READ_REG(hw, IXGBE_PFVFSPOOF(vf_target_reg));

Re: [PATCH v3 net] ixgbe: check adapter->vfinfo before dereference

[Thierry Herbelot]

On 10/16/2014 12:50 AM, Tantilov, Emil S wrote:
>> -----Original Message-----
>> From: Thierry Herbelot [mailto:thierry.herbelot@6wind.com]
>> Sent: Wednesday, October 15, 2014 2:58 AM
>> To: Kirsher, Jeffrey T; Brandeburg, Jesse; Allan, Bruce W;
>> netdev@vger.kernel.org; Tantilov, Emil S
>> Cc: Thierry Herbelot
>> Subject: [PATCH v3 net] ixgbe: check adapter->vfinfo before dereference
>>
>> this protects against the following panic:
>> (before a VF was actually created on p96p1 PF Ethernet port)
>>
>> ip link set p96p1 vf 0 spoofchk off
>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000052
>> IP: [<ffffffffa044a1c1>]
>> ixgbe_ndo_set_vf_spoofchk+0x51/0x150 [ixgbe]
>>
>> Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>
>> ---
>>
>> v2:
>>   compilation fixes
>>
>> v3:
>>   remove checks in functions where vfinfo is known not to be NULL
>>   return -EINVAL as error code
>>
>> drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |   42
>> ++++++++++++++++++++++--
>> 1 file changed, 40 insertions(+), 2 deletions(-)
>
> Actually looking into this a bit more, the check for vfinfo is not sufficient
> because it does not protect against specifying vf that is outside of sriov_num_vfs range.
>
> All of the ndo functions have a check for it except for ixgbevf_ndo_set_spoofcheck().
>
> The following patch should be all we need to protect against this panic:
>
> This patch adds a check to return -EINVAL when setting spoofcheck on
> VF that is not configured.
>
> Reported-by: Thierry Herbelot <thierry.herbelot@6wind.com>
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> ---
>   drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |    3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> index 706fc69..97c85b8 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> @@ -1261,6 +1261,9 @@ int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting)
>   	struct ixgbe_hw *hw = &adapter->hw;
>   	u32 regval;
>
> +	if (vf >= adapter->num_vfs)
> +		return -EINVAL;
> +
>   	adapter->vfinfo[vf].spoofchk_enabled = setting;
>
>   	regval = IXGBE_READ_REG(hw, IXGBE_PFVFSPOOF(vf_target_reg));
>
>

Hello,

Indeed your patch solves the initial issue.

And indeed I also double-checked that all other instances are protected 
by the (vf >= adapter->num_vfs) condition.

	Best regards

	Thierry

-- 
Thierry Herbelot
6WIND
Software Engineer

Re: [PATCH v3 net] ixgbe: check adapter->vfinfo before dereference

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 2836 bytes --]

On Thu, 2014-10-16 at 09:23 +0200, Thierry Herbelot wrote:
> On 10/16/2014 12:50 AM, Tantilov, Emil S wrote:
> >> -----Original Message-----
> >> From: Thierry Herbelot [mailto:thierry.herbelot@6wind.com]
> >> Sent: Wednesday, October 15, 2014 2:58 AM
> >> To: Kirsher, Jeffrey T; Brandeburg, Jesse; Allan, Bruce W;
> >> netdev@vger.kernel.org; Tantilov, Emil S
> >> Cc: Thierry Herbelot
> >> Subject: [PATCH v3 net] ixgbe: check adapter->vfinfo before dereference
> >>
> >> this protects against the following panic:
> >> (before a VF was actually created on p96p1 PF Ethernet port)
> >>
> >> ip link set p96p1 vf 0 spoofchk off
> >> BUG: unable to handle kernel NULL pointer dereference at 0000000000000052
> >> IP: [<ffffffffa044a1c1>]
> >> ixgbe_ndo_set_vf_spoofchk+0x51/0x150 [ixgbe]
> >>
> >> Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>
> >> ---
> >>
> >> v2:
> >>   compilation fixes
> >>
> >> v3:
> >>   remove checks in functions where vfinfo is known not to be NULL
> >>   return -EINVAL as error code
> >>
> >> drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |   42
> >> ++++++++++++++++++++++--
> >> 1 file changed, 40 insertions(+), 2 deletions(-)
> >
> > Actually looking into this a bit more, the check for vfinfo is not sufficient
> > because it does not protect against specifying vf that is outside of sriov_num_vfs range.
> >
> > All of the ndo functions have a check for it except for ixgbevf_ndo_set_spoofcheck().
> >
> > The following patch should be all we need to protect against this panic:
> >
> > This patch adds a check to return -EINVAL when setting spoofcheck on
> > VF that is not configured.
> >
> > Reported-by: Thierry Herbelot <thierry.herbelot@6wind.com>
> > Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> > ---
> >   drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |    3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > index 706fc69..97c85b8 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > @@ -1261,6 +1261,9 @@ int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting)
> >   	struct ixgbe_hw *hw = &adapter->hw;
> >   	u32 regval;
> >
> > +	if (vf >= adapter->num_vfs)
> > +		return -EINVAL;
> > +
> >   	adapter->vfinfo[vf].spoofchk_enabled = setting;
> >
> >   	regval = IXGBE_READ_REG(hw, IXGBE_PFVFSPOOF(vf_target_reg));
> >
> >
> 
> Hello,
> 
> Indeed your patch solves the initial issue.
> 
> And indeed I also double-checked that all other instances are protected 
> by the (vf >= adapter->num_vfs) condition.

So Thierry, can I add your Acked-by or Tested-by to Emil's patch?

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 net] ixgbe: check adapter->vfinfo before dereference

[Thierry Herbelot]

On 10/16/2014 09:32 AM, Jeff Kirsher wrote:
> On Thu, 2014-10-16 at 09:23 +0200, Thierry Herbelot wrote:
>> On 10/16/2014 12:50 AM, Tantilov, Emil S wrote:
>>>> -----Original Message-----
>>>> From: Thierry Herbelot [mailto:thierry.herbelot@6wind.com]
>>>> Sent: Wednesday, October 15, 2014 2:58 AM
>>>> To: Kirsher, Jeffrey T; Brandeburg, Jesse; Allan, Bruce W;
>>>> netdev@vger.kernel.org; Tantilov, Emil S
>>>> Cc: Thierry Herbelot
>>>> Subject: [PATCH v3 net] ixgbe: check adapter->vfinfo before dereference
>>>>
>>>> this protects against the following panic:
>>>> (before a VF was actually created on p96p1 PF Ethernet port)
>>>>
>>>> ip link set p96p1 vf 0 spoofchk off
>>>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000052
>>>> IP: [<ffffffffa044a1c1>]
>>>> ixgbe_ndo_set_vf_spoofchk+0x51/0x150 [ixgbe]
>>>>
>>>> Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>
>>>> ---
>>>>
>>>> v2:
>>>>    compilation fixes
>>>>
>>>> v3:
>>>>    remove checks in functions where vfinfo is known not to be NULL
>>>>    return -EINVAL as error code
>>>>
>>>> drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |   42
>>>> ++++++++++++++++++++++--
>>>> 1 file changed, 40 insertions(+), 2 deletions(-)
>>>
>>> Actually looking into this a bit more, the check for vfinfo is not sufficient
>>> because it does not protect against specifying vf that is outside of sriov_num_vfs range.
>>>
>>> All of the ndo functions have a check for it except for ixgbevf_ndo_set_spoofcheck().
>>>
>>> The following patch should be all we need to protect against this panic:
>>>
>>> This patch adds a check to return -EINVAL when setting spoofcheck on
>>> VF that is not configured.
>>>
>>> Reported-by: Thierry Herbelot <thierry.herbelot@6wind.com>
>>> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
>>> ---
>>>    drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |    3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>>> index 706fc69..97c85b8 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>>> @@ -1261,6 +1261,9 @@ int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting)
>>>    	struct ixgbe_hw *hw = &adapter->hw;
>>>    	u32 regval;
>>>
>>> +	if (vf >= adapter->num_vfs)
>>> +		return -EINVAL;
>>> +
>>>    	adapter->vfinfo[vf].spoofchk_enabled = setting;
>>>
>>>    	regval = IXGBE_READ_REG(hw, IXGBE_PFVFSPOOF(vf_target_reg));
>>>
>>>
>>
>> Hello,
>>
>> Indeed your patch solves the initial issue.
>>
>> And indeed I also double-checked that all other instances are protected
>> by the (vf >= adapter->num_vfs) condition.
>
> So Thierry, can I add your Acked-by or Tested-by to Emil's patch?
>

Hello,

I agree with the patch.

Acked-by Thierry Herbelot <thierry.herbelot@6wind.com>

	Best regards

	Th

-- 
Thierry Herbelot
6WIND
Software Engineer

Re: [PATCH v3 net] ixgbe: check adapter->vfinfo before dereference

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 160 bytes --]

On Thu, 2014-10-16 at 09:34 +0200, Thierry Herbelot wrote:
> I agree with the patch.
> 
> Acked-by Thierry Herbelot <thierry.herbelot@6wind.com>

Thanks!

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]