[PATCH net] gianfar: Reject out-of-range RX NFC locations

Currently the driver only uses location values to maintain an ordered
list of filters.  There is nothing to stop the list becoming longer
than the filer hardware can support - the driver will report an error,
but will not roll back the change!

Make it reject location values >= MAX_FILER_IDX, consistent with the
range that gfar_get_cls_all() reports.

Signed-off-by: Ben Hutchings <redacted>
---
[Re-sent to what I hope is a current address for Sebastian.]

This has not been tested in any way, as I don't have a suitable compiler
installed.  Sebastian, please could you review this?

Ben.

 drivers/net/ethernet/freescale/gianfar_ethtool.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 5890f4b..5a3b2e5 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

@@ -1692,8 +1692,9 @@ static int gfar_set_nfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
 		ret = gfar_set_hash_opts(priv, cmd);
 		break;
 	case ETHTOOL_SRXCLSRLINS:
-		if (cmd->fs.ring_cookie != RX_CLS_FLOW_DISC &&
-			cmd->fs.ring_cookie >= priv->num_rx_queues) {
+		if ((cmd->fs.ring_cookie != RX_CLS_FLOW_DISC &&
+		     cmd->fs.ring_cookie >= priv->num_rx_queues) ||
+		    cmd->fs.location >= MAX_FILER_IDX) {
 			ret = -EINVAL;
 			break;
 		}

-- 
1.7.4.4


-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

On Sat, 2011-12-17 at 01:05 +0000, Ben Hutchings wrote:

Currently the driver only uses location values to maintain an ordered
list of filters.  There is nothing to stop the list becoming longer
than the filer hardware can support - the driver will report an error,
but will not roll back the change!

Sure that it does not do the rollback?
In case of to much filters it should work this way:
# Convert all in temp table
# Compress temp table
# Write out temp table => not enough space => discard temp table &
remove filter from list 

Make it reject location values >= MAX_FILER_IDX, consistent with the
range that gfar_get_cls_all() reports.

Signed-off-by: Ben Hutchings <redacted>

Reviewed-by: Sebastian Pöhn <redacted>

---
[Re-sent to what I hope is a current address for Sebastian.]

This has not been tested in any way, as I don't have a suitable compiler
installed.  Sebastian, please could you review this?

Unfortunately I haven't too ...

On Sat, 2011-12-17 at 09:39 +0100, Sebastian Pöhn wrote:

On Sat, 2011-12-17 at 01:05 +0000, Ben Hutchings wrote:

quoted

Currently the driver only uses location values to maintain an ordered
list of filters.  There is nothing to stop the list becoming longer
than the filer hardware can support - the driver will report an error,
but will not roll back the change!

Sure that it does not do the rollback?
In case of to much filters it should work this way:
# Convert all in temp table
# Compress temp table
# Write out temp table => not enough space => discard temp table &
remove filter from list 

You're right, I must have misread the code previously.  Therefore, I
withdraw this patch for the time being.

quoted

Make it reject location values >= MAX_FILER_IDX, consistent with the
range that gfar_get_cls_all() reports.

I do however think that this should be consistent, and I would like to
reserve some 'special' location values.  But, for compatibility, it may
be necessary to use some other method to distinguish them (e.g. another
flow_type flag.)

Ben.

quoted

Signed-off-by: Ben Hutchings <redacted>

Reviewed-by: Sebastian Pöhn <redacted>

quoted

---
[Re-sent to what I hope is a current address for Sebastian.]

This has not been tested in any way, as I don't have a suitable compiler
installed.  Sebastian, please could you review this?

Unfortunately I haven't too ...

quoted

Ben.

 drivers/net/ethernet/freescale/gianfar_ethtool.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 5890f4b..5a3b2e5 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

@@ -1692,8 +1692,9 @@ static int gfar_set_nfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
 		ret = gfar_set_hash_opts(priv, cmd);
 		break;
 	case ETHTOOL_SRXCLSRLINS:
-		if (cmd->fs.ring_cookie != RX_CLS_FLOW_DISC &&
-			cmd->fs.ring_cookie >= priv->num_rx_queues) {
+		if ((cmd->fs.ring_cookie != RX_CLS_FLOW_DISC &&
+		     cmd->fs.ring_cookie >= priv->num_rx_queues) ||
+		    cmd->fs.location >= MAX_FILER_IDX) {
 			ret = -EINVAL;
 			break;
 		}

-- 
1.7.4.4

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.