[PATCH v2] net: dsa: mv88e6xxx: hwtstamp: fix potential negative array index read

_port_ is being used as index to array port_hwtstamp before verifying
it is a non-negative number and a valid index at line 209 and 258:

if (port < 0 || port >= mv88e6xxx_num_ports(chip))

Fix this by checking _port_ before using it as index to array
port_hwtstamp.

Addresses-Coverity-ID: 1465287 ("Negative array index read")
Addresses-Coverity-ID: 1465291 ("Negative array index read")
Fixes: c6fe0ad2c349 ("net: dsa: mv88e6xxx: add rx/tx timestamping support")
Signed-off-by: Gustavo A. R. Silva <redacted>
---
Changes in v2:
 -Fix the same issue in mv88e6xxx_should_tstamp.
 -Update commit message.

 drivers/net/dsa/mv88e6xxx/hwtstamp.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index b251d53..5a665aa 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c

@@ -200,8 +200,8 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int port,
 				struct ifreq *ifr)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
-	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
-	struct hwtstamp_config *config = &ps->tstamp_config;
+	struct mv88e6xxx_port_hwtstamp *ps;
+	struct hwtstamp_config *config;
 
 	if (!chip->info->ptp_support)
 		return -EOPNOTSUPP;

@@ -209,6 +209,9 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int port,
 	if (port < 0 || port >= mv88e6xxx_num_ports(chip))
 		return -EINVAL;
 
+	ps = &chip->port_hwtstamp[port];
+	config = &ps->tstamp_config;
+
 	return copy_to_user(ifr->ifr_data, config, sizeof(*config)) ?
 		-EFAULT : 0;
 }

@@ -249,7 +252,7 @@ static u8 *parse_ptp_header(struct sk_buff *skb, unsigned int type)
 static u8 *mv88e6xxx_should_tstamp(struct mv88e6xxx_chip *chip, int port,
 				   struct sk_buff *skb, unsigned int type)
 {
-	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
+	struct mv88e6xxx_port_hwtstamp *ps;
 	u8 *hdr;
 
 	if (!chip->info->ptp_support)

@@ -262,6 +265,7 @@ static u8 *mv88e6xxx_should_tstamp(struct mv88e6xxx_chip *chip, int port,
 	if (!hdr)
 		return NULL;
 
+	ps = &chip->port_hwtstamp[port];
 	if (!test_bit(MV88E6XXX_HWTSTAMP_ENABLED, &ps->state))
 		return NULL;
 

-- 
2.7.4

On Thu, Feb 15, 2018 at 12:31:39PM -0600, Gustavo A. R. Silva wrote:

_port_ is being used as index to array port_hwtstamp before verifying
it is a non-negative number and a valid index at line 209 and 258:

if (port < 0 || port >= mv88e6xxx_num_ports(chip))

Fix this by checking _port_ before using it as index to array
port_hwtstamp.

Addresses-Coverity-ID: 1465287 ("Negative array index read")
Addresses-Coverity-ID: 1465291 ("Negative array index read")
Fixes: c6fe0ad2c349 ("net: dsa: mv88e6xxx: add rx/tx timestamping support")
Signed-off-by: Gustavo A. R. Silva <redacted>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

On Thu, Feb 15, 2018 at 12:31:39PM -0600, Gustavo A. R. Silva wrote:

_port_ is being used as index to array port_hwtstamp before verifying
it is a non-negative number and a valid index at line 209 and 258:

if (port < 0 || port >= mv88e6xxx_num_ports(chip))

Fix this by checking _port_ before using it as index to array
port_hwtstamp.

NAK.   Port is already known to be valid in the callers.

See:

*** net/dsa/slave.c:  dsa_slave_ioctl[266]
*** net/dsa/slave.c:  dsa_skb_tx_timestamp[416]
*** net/dsa/dsa.c:    dsa_skb_defer_rx_timestamp[152]
 

Addresses-Coverity-ID: 1465287 ("Negative array index read")
Addresses-Coverity-ID: 1465291 ("Negative array index read")

Please check the code before posting.  These false positives are
really annoying.

Thanks,
Richard

On Fri, Feb 16, 2018 at 07:48:46AM -0800, Richard Cochran wrote:

On Thu, Feb 15, 2018 at 12:31:39PM -0600, Gustavo A. R. Silva wrote:

quoted

_port_ is being used as index to array port_hwtstamp before verifying
it is a non-negative number and a valid index at line 209 and 258:

if (port < 0 || port >= mv88e6xxx_num_ports(chip))

Fix this by checking _port_ before using it as index to array
port_hwtstamp.

NAK.   Port is already known to be valid in the callers.

Then we should take out the check. It is probably this check which is
causing the false positives.

	Andrew

On Fri, Feb 16, 2018 at 07:48:46AM -0800, Richard Cochran wrote:

On Thu, Feb 15, 2018 at 12:31:39PM -0600, Gustavo A. R. Silva wrote:

quoted

_port_ is being used as index to array port_hwtstamp before verifying
it is a non-negative number and a valid index at line 209 and 258:

if (port < 0 || port >= mv88e6xxx_num_ports(chip))

Fix this by checking _port_ before using it as index to array
port_hwtstamp.

NAK.   Port is already known to be valid in the callers.

And so the real bug is the pointless range checking tests.  I would
welcome patches to remove those.

Thanks,
Richard

On 02/16/2018 09:56 AM, Richard Cochran wrote:

On Fri, Feb 16, 2018 at 07:48:46AM -0800, Richard Cochran wrote:

quoted

On Thu, Feb 15, 2018 at 12:31:39PM -0600, Gustavo A. R. Silva wrote:

quoted

_port_ is being used as index to array port_hwtstamp before verifying
it is a non-negative number and a valid index at line 209 and 258:

if (port < 0 || port >= mv88e6xxx_num_ports(chip))

Fix this by checking _port_ before using it as index to array
port_hwtstamp.

NAK.   Port is already known to be valid in the callers.

And so the real bug is the pointless range checking tests.  I would
welcome patches to remove those.

I just sent a patch for this.

Thank you both, Andrew and Richard for the feedback.
--
Gustavo