[PATCH net 0/4] net: dsa: Use strncpy() for ethtool::get_strings

[PATCH net 0/4] net: dsa: Use strncpy() for ethtool::get_strings

[Florian Fainelli]

Hi all,

After turning on KASAN on one of my systems, I started getting lots of out of
bounds errors while fetching a given port's statistics, and indeed using
memcpy() is unsafe for copying strings, so let's use strncpy() instead.

Florian Fainelli (4):
  net: dsa: b53: Use strncpy() for ethtool::get_strings
  net: dsa: loop: Use strncpy() for ethtool::get_strings
  net: dsa: microchip: Utilize strncpy() for ethtool::get_strings
  net: dsa: mv88e6xxx: Utilize strncpy() for ethtool::get_strings

 drivers/net/dsa/b53/b53_common.c       | 4 ++--
 drivers/net/dsa/dsa_loop.c             | 4 ++--
 drivers/net/dsa/microchip/ksz_common.c | 4 ++--
 drivers/net/dsa/mv88e6xxx/chip.c       | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.14.1

[PATCH net 1/4] net: dsa: b53: Use strncpy() for ethtool::get_strings

[Florian Fainelli]

Do not use memcpy() which is not safe, but instead use strncpy() which
will make sure that the string is NUL terminated (in the Linux
implementation) if the string is smaller than the length specified. This
fixes KASAN out of bounds warnings while fetching port statistics.

Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index db830a1141d9..c64ebb82df83 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -814,8 +814,8 @@ void b53_get_strings(struct dsa_switch *ds, int port, uint8_t *data)
 	unsigned int i;
 
 	for (i = 0; i < mib_size; i++)
-		memcpy(data + i * ETH_GSTRING_LEN,
-		       mibs[i].name, ETH_GSTRING_LEN);
+		strncpy(data + i * ETH_GSTRING_LEN,
+			mibs[i].name, ETH_GSTRING_LEN);
 }
 EXPORT_SYMBOL(b53_get_strings);
 
-- 
2.14.1

[PATCH net 2/4] net: dsa: loop: Use strncpy() for ethtool::get_strings

[Florian Fainelli]

Do not use memcpy() which is not safe, but instead use strncpy() which
will make sure that the string is NUL terminated (in the Linux
implementation) if the string is smaller than the length specified. This
fixes KASAN out of bounds warnings while fetching port statistics.

Fixes: 484c01720d84 ("net: dsa: loop: Implement ethtool statistics")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/dsa_loop.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c
index 7aa84ee4e771..2e7e1cdbd2e9 100644
--- a/drivers/net/dsa/dsa_loop.c
+++ b/drivers/net/dsa/dsa_loop.c
@@ -97,8 +97,8 @@ static void dsa_loop_get_strings(struct dsa_switch *ds, int port, uint8_t *data)
 	unsigned int i;
 
 	for (i = 0; i < __DSA_LOOP_CNT_MAX; i++)
-		memcpy(data + i * ETH_GSTRING_LEN,
-		       ps->ports[port].mib[i].name, ETH_GSTRING_LEN);
+		strncpy(data + i * ETH_GSTRING_LEN,
+			ps->ports[port].mib[i].name, ETH_GSTRING_LEN);
 }
 
 static void dsa_loop_get_ethtool_stats(struct dsa_switch *ds, int port,
-- 
2.14.1

[PATCH net 3/4] net: dsa: microchip: Utilize strncpy() for ethtool::get_strings

[Florian Fainelli]

Do not use memcpy() which is not safe, but instead use strncpy() which
will make sure that the string is NUL terminated (in the Linux
implementation) if the string is smaller than the length specified. This
fixes KASAN out of bounds warnings while fetching port statistics.

Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 663b0d5b982b..db7f5c8c1dcb 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -449,8 +449,8 @@ static void ksz_get_strings(struct dsa_switch *ds, int port, uint8_t *buf)
 	int i;
 
 	for (i = 0; i < TOTAL_SWITCH_COUNTER_NUM; i++) {
-		memcpy(buf + i * ETH_GSTRING_LEN, mib_names[i].string,
-		       ETH_GSTRING_LEN);
+		strncpy(buf + i * ETH_GSTRING_LEN, mib_names[i].string,
+			ETH_GSTRING_LEN);
 	}
 }
 
-- 
2.14.1

RE: [PATCH net 3/4] net: dsa: microchip: Utilize strncpy() for ethtool::get_strings

[David Laight]

From: Florian Fainelli
>
> Do not use memcpy() which is not safe, but instead use strncpy() which
> will make sure that the string is NUL terminated (in the Linux
> implementation) if the string is smaller than the length specified. This
> fixes KASAN out of bounds warnings while fetching port statistics.

You really ought to use a copy function that will truncate the
string if it is too long.
Just assuming the string isn't too long is asking for trouble.
You might (almost) just use strcpy().

strlcpy() will probably work best here.

	David

Re: [PATCH net 3/4] net: dsa: microchip: Utilize strncpy() for ethtool::get_strings

[Florian Fainelli]

On 03/02/2018 02:51 AM, David Laight wrote:
> From: Florian Fainelli
>>
>> Do not use memcpy() which is not safe, but instead use strncpy() which
>> will make sure that the string is NUL terminated (in the Linux
>> implementation) if the string is smaller than the length specified. This
>> fixes KASAN out of bounds warnings while fetching port statistics.
> 
> You really ought to use a copy function that will truncate the
> string if it is too long.
> Just assuming the string isn't too long is asking for trouble.
> You might (almost) just use strcpy().
> 
> strlcpy() will probably work best here.

Right, or if we actually do size the statistics string to be
ETH_GSTRING_LEN bytes, memcpy() can be used, provided that the strings
are initialized correctly (which they are).
-- 
Florian

[PATCH net 4/4] net: dsa: mv88e6xxx: Utilize strncpy() for ethtool::get_strings

[Florian Fainelli]

Do not use memcpy() which is not safe, but instead use strncpy() which
will make sure that the string is NUL terminated (in the Linux
implementation) if the string is smaller than the length specified. This
fixes KASAN out of bounds warnings while fetching port statistics.

Fixes: f5e2ed022dff ("dsa: mv88e6xxx: Add Second back of statistics")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index eb328bade225..bec7540aae2b 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -636,8 +636,8 @@ static void mv88e6xxx_stats_get_strings(struct mv88e6xxx_chip *chip,
 	for (i = 0, j = 0; i < ARRAY_SIZE(mv88e6xxx_hw_stats); i++) {
 		stat = &mv88e6xxx_hw_stats[i];
 		if (stat->type & types) {
-			memcpy(data + j * ETH_GSTRING_LEN, stat->string,
-			       ETH_GSTRING_LEN);
+			strncpy(data + j * ETH_GSTRING_LEN, stat->string,
+				ETH_GSTRING_LEN);
 			j++;
 		}
 	}
-- 
2.14.1

Re: [PATCH net 4/4] net: dsa: mv88e6xxx: Utilize strncpy() for ethtool::get_strings

[Andrew Lunn]

On Thu, Mar 01, 2018 at 04:25:29PM -0800, Florian Fainelli wrote:
> Do not use memcpy() which is not safe, but instead use strncpy() which
> will make sure that the string is NUL terminated (in the Linux
> implementation) if the string is smaller than the length specified. This
> fixes KASAN out of bounds warnings while fetching port statistics.
> 
> Fixes: f5e2ed022dff ("dsa: mv88e6xxx: Add Second back of statistics")

I'm sure it goes back much further than that.

> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

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

    Andrew

Re: [PATCH net 4/4] net: dsa: mv88e6xxx: Utilize strncpy() for ethtool::get_strings

[Florian Fainelli]

On 03/01/2018 07:08 PM, Andrew Lunn wrote:
> On Thu, Mar 01, 2018 at 04:25:29PM -0800, Florian Fainelli wrote:
>> Do not use memcpy() which is not safe, but instead use strncpy() which
>> will make sure that the string is NUL terminated (in the Linux
>> implementation) if the string is smaller than the length specified. This
>> fixes KASAN out of bounds warnings while fetching port statistics.
>>
>> Fixes: f5e2ed022dff ("dsa: mv88e6xxx: Add Second back of statistics")
> 
> I'm sure it goes back much further than that.

You are right, it appears that I used the most recent commit that
changed the stats last.

This is not actually needed per-se here because the string is defined to
be ETH_GSTRING_LEN bytes, so unlike b53, we are not copying past the
buffer, in fact only the first patch is really necessary.

Thanks!

> 
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
>     Andrew
> 

-- 
Florian

Re: [PATCH net 0/4] net: dsa: Use strncpy() for ethtool::get_strings

[Florian Fainelli]

Hi David,

On 03/01/2018 04:25 PM, Florian Fainelli wrote:
> Hi all,
> 
> After turning on KASAN on one of my systems, I started getting lots of out of
> bounds errors while fetching a given port's statistics, and indeed using
> memcpy() is unsafe for copying strings, so let's use strncpy() instead.

Looks like only patch 1 is necessary, but there are more drivers with
the same pattern under drivers/net/phy: marvell.c, micrel.c and
bcm-phy-lib.c, so I will submit a v2 with those fixed.

> 
> Florian Fainelli (4):
>   net: dsa: b53: Use strncpy() for ethtool::get_strings
>   net: dsa: loop: Use strncpy() for ethtool::get_strings
>   net: dsa: microchip: Utilize strncpy() for ethtool::get_strings
>   net: dsa: mv88e6xxx: Utilize strncpy() for ethtool::get_strings
> 
>  drivers/net/dsa/b53/b53_common.c       | 4 ++--
>  drivers/net/dsa/dsa_loop.c             | 4 ++--
>  drivers/net/dsa/microchip/ksz_common.c | 4 ++--
>  drivers/net/dsa/mv88e6xxx/chip.c       | 4 ++--
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 

-- 
Florian