diff mbox series

nixge: fix mac address error handling again

Message ID 20211122150322.4043037-1-arnd@kernel.org (mailing list archive)
State Superseded
Commit a68229ca634066975fff6d4780155bd2eb14a82a
Delegated to: Netdev Maintainers
Headers show
Series nixge: fix mac address error handling again | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 2 this patch: 2
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 16 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply, async

Commit Message

Arnd Bergmann Nov. 22, 2021, 3:02 p.m. UTC 
From: Arnd Bergmann <arnd@arndb.de>

The change to eth_hw_addr_set() caused gcc to correctly spot a
bug that was introduced in an earlier incorrect fix:

In file included from include/linux/etherdevice.h:21,
                 from drivers/net/ethernet/ni/nixge.c:7:
In function '__dev_addr_set',
    inlined from 'eth_hw_addr_set' at include/linux/etherdevice.h:319:2,
    inlined from 'nixge_probe' at drivers/net/ethernet/ni/nixge.c:1286:3:
include/linux/netdevice.h:4648:9: error: 'memcpy' reading 6 bytes from a region of size 0 [-Werror=stringop-overread]
 4648 |         memcpy(dev->dev_addr, addr, len);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

As nixge_get_nvmem_address() can return either NULL or an error
pointer, the NULL check is wrong, and we can end up reading from
ERR_PTR(-EOPNOTSUPP), which gcc knows to contain zero readable
bytes.

Make the function always return an error pointer again but fix
the check to match that.

Fixes: f3956ebb3bf0 ("ethernet: use eth_hw_addr_set() instead of ether_addr_copy()")
Fixes: abcd3d6fc640 ("net: nixge: Fix error path for obtaining mac address")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/ethernet/ni/nixge.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Arnd Bergmann Nov. 22, 2021, 3:06 p.m. UTC | #1 
On Mon, Nov 22, 2021 at 4:02 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> The change to eth_hw_addr_set() caused gcc to correctly spot a
> bug that was introduced in an earlier incorrect fix:
>
> In file included from include/linux/etherdevice.h:21,
>                  from drivers/net/ethernet/ni/nixge.c:7:
> In function '__dev_addr_set',
>     inlined from 'eth_hw_addr_set' at include/linux/etherdevice.h:319:2,
>     inlined from 'nixge_probe' at drivers/net/ethernet/ni/nixge.c:1286:3:
> include/linux/netdevice.h:4648:9: error: 'memcpy' reading 6 bytes from a region of size 0 [-Werror=stringop-overread]
>  4648 |         memcpy(dev->dev_addr, addr, len);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> As nixge_get_nvmem_address() can return either NULL or an error
> pointer, the NULL check is wrong, and we can end up reading from
> ERR_PTR(-EOPNOTSUPP), which gcc knows to contain zero readable
> bytes.
>
> Make the function always return an error pointer again but fix
> the check to match that.
>
> Fixes: f3956ebb3bf0 ("ethernet: use eth_hw_addr_set() instead of ether_addr_copy()")
> Fixes: abcd3d6fc640 ("net: nixge: Fix error path for obtaining mac address")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Sorry, I sent out the wrong patch, this version fixes the bug but not the
warning. v2 is the version I actually tested successfully.

       Arnd
patchwork-bot+netdevbpf@kernel.org Nov. 22, 2021, 3:10 p.m. UTC | #2 
Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Mon, 22 Nov 2021 16:02:49 +0100 you wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The change to eth_hw_addr_set() caused gcc to correctly spot a
> bug that was introduced in an earlier incorrect fix:
> 
> In file included from include/linux/etherdevice.h:21,
>                  from drivers/net/ethernet/ni/nixge.c:7:
> In function '__dev_addr_set',
>     inlined from 'eth_hw_addr_set' at include/linux/etherdevice.h:319:2,
>     inlined from 'nixge_probe' at drivers/net/ethernet/ni/nixge.c:1286:3:
> include/linux/netdevice.h:4648:9: error: 'memcpy' reading 6 bytes from a region of size 0 [-Werror=stringop-overread]
>  4648 |         memcpy(dev->dev_addr, addr, len);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> [...]

Here is the summary with links:
  - nixge: fix mac address error handling again
    https://git.kernel.org/netdev/net/c/a68229ca6340

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
index cfeb7620ae20..07a00dd9cfe0 100644
--- a/drivers/net/ethernet/ni/nixge.c
+++ b/drivers/net/ethernet/ni/nixge.c
@@ -1209,7 +1209,7 @@  static void *nixge_get_nvmem_address(struct device *dev)
 
 	cell = nvmem_cell_get(dev, "address");
 	if (IS_ERR(cell))
-		return NULL;
+		return cell;
 
 	mac = nvmem_cell_read(cell, &cell_size);
 	nvmem_cell_put(cell);
@@ -1282,7 +1282,7 @@  static int nixge_probe(struct platform_device *pdev)
 	ndev->max_mtu = NIXGE_JUMBO_MTU;
 
 	mac_addr = nixge_get_nvmem_address(&pdev->dev);
-	if (mac_addr && is_valid_ether_addr(mac_addr)) {
+	if (!IS_ERR(mac_addr) && is_valid_ether_addr(mac_addr)) {
 		eth_hw_addr_set(ndev, mac_addr);
 		kfree(mac_addr);
 	} else {