RE: [net-next v2 6/6] ixgbe: Add malicious driver detection support

[Tantilov, Emil S]

>-Original Message-
>From: David Miller [mailto:da...@davemloft.net]
>Sent: Thursday, June 29, 2017 11:28 AM
>To: gerlitz...@gmail.com
>Cc: Tantilov, Emil S <emil.s.tanti...@intel.com>; Kirsher, Jeffrey T
><jeffrey.t.kirs...@intel.com>; Greenwalt, Paul <paul.greenw...@intel.com>;
>netdev@vger.kernel.org; nhor...@redhat.com; sassm...@redhat.com;
>jogre...@redhat.com
>Subject: Re: [net-next v2 6/6] ixgbe: Add malicious driver detection
>support
>
>From: Or Gerlitz <gerlitz...@gmail.com>
>Date: Wed, 28 Jun 2017 17:28:59 +0300
>
>> On Wed, Jun 28, 2017 at 1:14 AM, Tantilov, Emil S
>> <emil.s.tanti...@intel.com> wrote:
>>
>>> Mainly because I am not sure that other (non-Intel) drivers will benefit
>from
>>> such an option. In normal operation this functionality should not cause
>issues
>>> and if it doesn't we may be able to deprecate the private flag in the
>future.
>>
>> If you think this functionality makes sense any driver running over HW
>> implementing
>> it would like to be able to expose that and hence you better not use
>> private flag.

As I mentioned I don't know if this will be useful for other drivers.
The i40e driver enables it by default without possibility to disable it
and if this protection does not cause problems for ixgbe then we may not
need the option in the future. Because of this I wasn't sure if it's
worth polluting the tools with options that may end up not being 
needed/used at all.
 
>> Are we sure the trust UAPI can't be extended for that matter?
>
>Yeah, we should probably make this a generic control if possible.

MDD is set globally for the device, while the trusted option is set per VF.
So if we do end up adding an option it probably won't work as extension for
trusted.

Thanks,
Emil

Re: [net-next v2 6/6] ixgbe: Add malicious driver detection support

[David Miller]

From: Or Gerlitz 
Date: Wed, 28 Jun 2017 17:28:59 +0300

> On Wed, Jun 28, 2017 at 1:14 AM, Tantilov, Emil S
>  wrote:
> 
>> Mainly because I am not sure that other (non-Intel) drivers will benefit from
>> such an option. In normal operation this functionality should not cause 
>> issues
>> and if it doesn't we may be able to deprecate the private flag in the future.
> 
> If you think this functionality makes sense any driver running over HW
> implementing
> it would like to be able to expose that and hence you better not use
> private flag.
> 
> Are we sure the trust UAPI can't be extended for that matter?

Yeah, we should probably make this a generic control if possible.

Re: [net-next v2 6/6] ixgbe: Add malicious driver detection support

[Or Gerlitz]

On Wed, Jun 28, 2017 at 1:14 AM, Tantilov, Emil S
 wrote:

> Mainly because I am not sure that other (non-Intel) drivers will benefit from
> such an option. In normal operation this functionality should not cause issues
> and if it doesn't we may be able to deprecate the private flag in the future.

If you think this functionality makes sense any driver running over HW
implementing
it would like to be able to expose that and hence you better not use
private flag.

Are we sure the trust UAPI can't be extended for that matter?

RE: [net-next v2 6/6] ixgbe: Add malicious driver detection support

[Tantilov, Emil S]

>-Original Message-
>From: Or Gerlitz [mailto:gerlitz...@gmail.com]
>Sent: Tuesday, June 27, 2017 2:07 PM
>To: Tantilov, Emil S <emil.s.tanti...@intel.com>
>Cc: Kirsher, Jeffrey T <jeffrey.t.kirs...@intel.com>; David Miller
><da...@davemloft.net>; Greenwalt, Paul <paul.greenw...@intel.com>; Linux
>Netdev List <netdev@vger.kernel.org>; nhor...@redhat.com;
>sassm...@redhat.com; jogre...@redhat.com
>Subject: Re: [net-next v2 6/6] ixgbe: Add malicious driver detection
>support
>
>On Tue, Jun 27, 2017 at 11:59 PM, Tantilov, Emil S
><emil.s.tanti...@intel.com> wrote:
>>>-Original Message-
>>>From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
>On
>>>Behalf Of Or Gerlitz
>>>Sent: Tuesday, June 27, 2017 2:08 AM
>>>To: Kirsher, Jeffrey T <jeffrey.t.kirs...@intel.com>
>>>Cc: David Miller <da...@davemloft.net>; Greenwalt, Paul
>>><paul.greenw...@intel.com>; Linux Netdev List <netdev@vger.kernel.org>;
>>>nhor...@redhat.com; sassm...@redhat.com; jogre...@redhat.com
>>>Subject: Re: [net-next v2 6/6] ixgbe: Add malicious driver detection
>>>support
>>>
>>>On Tue, Jun 27, 2017 at 11:51 AM, Jeff Kirsher
>>><jeffrey.t.kirs...@intel.com> wrote:
>>>> From: Paul Greenwalt <paul.greenw...@intel.com>
>>>>
>>>> Add malicious driver detection (MDD) support for X550, X550em_a,
>>>> and X550em_x devices.
>>>>
>>>> MDD is a hardware SR-IOV security feature which the driver enables by
>>>> default, but can be controlled on|off by ethtool set-priv-flags
>>>
>>>wait, we have the trusted vf concept, which you implement
>>>(ixgbe_ndo_set_vf_trust)
>>>so you can enable by default for all vfs and disable it for trusted
>>>ones, why create[]  an ixgbe special config knob? IMHO we should max all
>possible efforts to
>>>avoid priv ethtool flags usage.
>>
>> The "trusted" option was added to allow use cases that were not possible in 
>> the
>> default driver configuration for SRIOV (promiscuous mode, overriding the 
>> MAC).
>> While these modes can lead to issues (performance with promisc) they can 
>> still
>> be useful for certain configurations.
>>
>> MDD is a completely different type of protection that incorporates checks for
>> queue context, Tx descriptors and out-of-bounds DMA/memory access that can
>> disrupt the operation of the interfaces. You can read more about it in the 
>> X550
>> datasheet (section 7.9.4.3 malicious Driver Detection):
>>
>https://www.intel.com/content/www/us/en/embedded/products/networking/ethern
>et-controller-x550-family-documentation.html
>>
>> For that reason we do not want to make it part of the "trusted" option.
>
>you can extend the trusted option without breaking the UAPI, currently
>it's one bit y/n, but you should have there at least seven more bits
>to use.
>
>> In addition MDD is a global setting and cannot be configured per-VF.
>
>can you state more clearly why use think the right configuration knob
>here is per driver ethtool private flag?

Mainly because I am not sure that other (non-Intel) drivers will benefit from
such an option. In normal operation this functionality should not cause issues
and if it doesn't we may be able to deprecate the private flag in the future.

On the other hand if the same/similar feature exists in other drivers then
it would perhaps make more sense to introduce a new option altogether.

Thanks,
Emil

Re: [net-next v2 6/6] ixgbe: Add malicious driver detection support

[Or Gerlitz]

On Tue, Jun 27, 2017 at 11:59 PM, Tantilov, Emil S
<emil.s.tanti...@intel.com> wrote:
>>-Original Message-
>>From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org] On
>>Behalf Of Or Gerlitz
>>Sent: Tuesday, June 27, 2017 2:08 AM
>>To: Kirsher, Jeffrey T <jeffrey.t.kirs...@intel.com>
>>Cc: David Miller <da...@davemloft.net>; Greenwalt, Paul
>><paul.greenw...@intel.com>; Linux Netdev List <netdev@vger.kernel.org>;
>>nhor...@redhat.com; sassm...@redhat.com; jogre...@redhat.com
>>Subject: Re: [net-next v2 6/6] ixgbe: Add malicious driver detection
>>support
>>
>>On Tue, Jun 27, 2017 at 11:51 AM, Jeff Kirsher
>><jeffrey.t.kirs...@intel.com> wrote:
>>> From: Paul Greenwalt <paul.greenw...@intel.com>
>>>
>>> Add malicious driver detection (MDD) support for X550, X550em_a,
>>> and X550em_x devices.
>>>
>>> MDD is a hardware SR-IOV security feature which the driver enables by
>>> default, but can be controlled on|off by ethtool set-priv-flags
>>
>>wait, we have the trusted vf concept, which you implement
>>(ixgbe_ndo_set_vf_trust)
>>so you can enable by default for all vfs and disable it for trusted
>>ones, why create[]  an ixgbe special config knob? IMHO we should max all 
>>possible efforts to
>>avoid priv ethtool flags usage.
>
> The "trusted" option was added to allow use cases that were not possible in 
> the
> default driver configuration for SRIOV (promiscuous mode, overriding the MAC).
> While these modes can lead to issues (performance with promisc) they can still
> be useful for certain configurations.
>
> MDD is a completely different type of protection that incorporates checks for
> queue context, Tx descriptors and out-of-bounds DMA/memory access that can
> disrupt the operation of the interfaces. You can read more about it in the 
> X550
> datasheet (section 7.9.4.3 malicious Driver Detection):
> https://www.intel.com/content/www/us/en/embedded/products/networking/ethernet-controller-x550-family-documentation.html
>
> For that reason we do not want to make it part of the "trusted" option.

you can extend the trusted option without breaking the UAPI, currently
it's one bit y/n, but you should have there at least seven more bits
to use.

> In addition MDD is a global setting and cannot be configured per-VF.

can you state more clearly why use think the right configuration knob
here is per driver ethtool private flag?

Or.

RE: [net-next v2 6/6] ixgbe: Add malicious driver detection support

[Tantilov, Emil S]

>-Original Message-
>From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org] On
>Behalf Of Or Gerlitz
>Sent: Tuesday, June 27, 2017 2:08 AM
>To: Kirsher, Jeffrey T <jeffrey.t.kirs...@intel.com>
>Cc: David Miller <da...@davemloft.net>; Greenwalt, Paul
><paul.greenw...@intel.com>; Linux Netdev List <netdev@vger.kernel.org>;
>nhor...@redhat.com; sassm...@redhat.com; jogre...@redhat.com
>Subject: Re: [net-next v2 6/6] ixgbe: Add malicious driver detection
>support
>
>On Tue, Jun 27, 2017 at 11:51 AM, Jeff Kirsher
><jeffrey.t.kirs...@intel.com> wrote:
>> From: Paul Greenwalt <paul.greenw...@intel.com>
>>
>> Add malicious driver detection (MDD) support for X550, X550em_a,
>> and X550em_x devices.
>>
>> MDD is a hardware SR-IOV security feature which the driver enables by
>> default, but can be controlled on|off by ethtool set-priv-flags
>
>wait, we have the trusted vf concept, which you implement
>(ixgbe_ndo_set_vf_trust)
>so you can enable by default for all vfs and disable it for trusted
>ones, why create[]  an ixgbe special config knob? IMHO we should max all 
>possible efforts to
>avoid priv ethtool flags usage.

The "trusted" option was added to allow use cases that were not possible in the
default driver configuration for SRIOV (promiscuous mode, overriding the MAC).
While these modes can lead to issues (performance with promisc) they can still
be useful for certain configurations.

MDD is a completely different type of protection that incorporates checks for
queue context, Tx descriptors and out-of-bounds DMA/memory access that can
disrupt the operation of the interfaces. You can read more about it in the X550
datasheet (section 7.9.4.3 malicious Driver Detection):
https://www.intel.com/content/www/us/en/embedded/products/networking/ethernet-controller-x550-family-documentation.html

For that reason we do not want to make it part of the "trusted" option.

In addition MDD is a global setting and cannot be configured per-VF.

Thanks,
Emil

Re: [net-next v2 6/6] ixgbe: Add malicious driver detection support

[Or Gerlitz]

On Tue, Jun 27, 2017 at 11:51 AM, Jeff Kirsher
 wrote:
> From: Paul Greenwalt 
>
> Add malicious driver detection (MDD) support for X550, X550em_a,
> and X550em_x devices.
>
> MDD is a hardware SR-IOV security feature which the driver enables by
> default, but can be controlled on|off by ethtool set-priv-flags

wait, we have the trusted vf concept, which you implement
(ixgbe_ndo_set_vf_trust)
so you can enable by default for all vfs and disable it for trusted
ones, why create
an ixgbe special config knob? IMHO we should max all possible efforts to avoid
priv ethtool flags usage.


Or.

[net-next v2 6/6] ixgbe: Add malicious driver detection support

[Jeff Kirsher]

From: Paul Greenwalt 

Add malicious driver detection (MDD) support for X550, X550em_a,
and X550em_x devices.

MDD is a hardware SR-IOV security feature which the driver enables by
default, but can be controlled on|off by ethtool set-priv-flags
parameter. When enabled MDD disables a VF drivers transmit queue
when a malformed descriptor is detected. The PF will log the event
and re-enable the VF queue.

Signed-off-by: Paul Greenwalt 
Tested-by: Andrew Bowers 
Signed-off-by: Jeff Kirsher 
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h |   3 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_dcb_nl.c  |  25 +++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c |  13 ++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c|   6 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c   |  50 
 drivers/net/ethernet/intel/ixgbe/ixgbe_type.h|   8 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c| 138 +++
 7 files changed, 241 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h 
b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index dd5578756ae0..2e9df66f6e18 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -563,6 +563,8 @@ struct ixgbe_mac_addr {
 #define IXGBE_TRY_LINK_TIMEOUT (4 * HZ)
 #define IXGBE_SFP_POLL_JIFFIES (2 * HZ)/* SFP poll every 2 seconds */
 
+#define IXGBE_MDD_Q_BITMAP_DEPTH 2
+
 /* board specific private data structure */
 struct ixgbe_adapter {
unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
@@ -603,6 +605,7 @@ struct ixgbe_adapter {
 #define IXGBE_FLAG_RX_HWTSTAMP_IN_REGISTER BIT(26)
 #define IXGBE_FLAG_DCB_CAPABLE BIT(27)
 #define IXGBE_FLAG_GENEVE_OFFLOAD_CAPABLE  BIT(28)
+#define IXGBE_FLAG_MDD_ENABLED BIT(29)
 
u32 flags2;
 #define IXGBE_FLAG2_RSC_CAPABLEBIT(0)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_dcb_nl.c 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_dcb_nl.c
index 78c52375acc6..53f260dbfb5f 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_dcb_nl.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_dcb_nl.c
@@ -379,10 +379,22 @@ static u8 ixgbe_dcbnl_set_all(struct net_device *netdev)
} else {
hw->mac.ops.fc_enable(hw);
}
+   /* Disable MDD before updating SRRCTL, because modifying the
+* SRRCTL register while the queue is enabled will generate an
+* MDD event.
+*/
+   if (adapter->num_vfs && hw->mac.ops.disable_mdd &&
+   (adapter->flags & IXGBE_FLAG_MDD_ENABLED))
+   hw->mac.ops.disable_mdd(hw);
 
ixgbe_set_rx_drop_en(adapter);
 
-   ret = DCB_HW_CHG;
+   if (adapter->num_vfs && hw->mac.ops.enable_mdd &&
+   (adapter->flags & IXGBE_FLAG_MDD_ENABLED))
+   hw->mac.ops.enable_mdd(hw);
+
+   if (ret != DCB_HW_CHG_RST)
+   ret = DCB_HW_CHG;
}
 
 #ifdef IXGBE_FCOE
@@ -634,8 +646,19 @@ static int ixgbe_dcbnl_ieee_setpfc(struct net_device *dev,
else
err = hw->mac.ops.fc_enable(hw);
 
+   /* Disable MDD before updating SRRCTL, because modifying the SRRCTL
+* register while the queue is enabled will generate an MDD event.
+*/
+   if (adapter->num_vfs && hw->mac.ops.disable_mdd &&
+   (adapter->flags & IXGBE_FLAG_MDD_ENABLED))
+   hw->mac.ops.disable_mdd(hw);
+
ixgbe_set_rx_drop_en(adapter);
 
+   if (adapter->num_vfs && hw->mac.ops.enable_mdd &&
+   (adapter->flags & IXGBE_FLAG_MDD_ENABLED))
+   hw->mac.ops.enable_mdd(hw);
+
return err;
 }
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 72c565712a5f..e10a4d6d5391 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -157,6 +157,8 @@ static const char ixgbe_gstrings_test[][ETH_GSTRING_LEN] = {
 static const char ixgbe_priv_flags_strings[][ETH_GSTRING_LEN] = {
 #define IXGBE_PRIV_FLAGS_LEGACY_RX BIT(0)
"legacy-rx",
+#define IXGBE_PRIV_FLAG_MDD_ENABLEDBIT(1)
+   "mdd",
 };
 
 #define IXGBE_PRIV_FLAGS_STR_LEN ARRAY_SIZE(ixgbe_priv_flags_strings)
@@ -3420,6 +3422,9 @@ static u32 ixgbe_get_priv_flags(struct net_device *netdev)
struct ixgbe_adapter *adapter = netdev_priv(netdev);
u32 priv_flags = 0;
 
+   if (adapter->flags & IXGBE_FLAG_MDD_ENABLED)
+   priv_flags |= IXGBE_PRIV_FLAG_MDD_ENABLED;
+
if (adapter->flags2 & IXGBE_FLAG2_RX_LEGACY)
priv_flags |= IXGBE_PRIV_FLAGS_LEGACY_RX;
 
@@ -3430,13 +3435,19 @@ static int ixgbe_set_priv_flags(struct