| Message ID | 20250322143314.1806893-1-b.spranger@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ti: icssg-prueth: Check return value to avoid a kernel oops | expand |
Hi, Did you actually get a kernel oops? If yes, which part of code produces the oops. On 22/03/2025 16:33, Benedikt Spranger wrote: > of_get_ethdev_address() may fail, do not set a MAC address and return Even if it fails we do set a random MAC address and do not return error. So above statement is false. > an error. The icssg-prueth driver ignores that failure and try to validate > the MAC address. This let to a NULL pointer dereference in this case. > > Check the return value of of_get_ethdev_address() before validating the > MAC address. If of_get_ethdev_address() fails the netdev address will remain zero (as it was zero initialized during allocation) so is_valid_ether_addr() will fail as well. > > Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de> > --- > drivers/net/ethernet/ti/icssg/icssg_prueth.c | 2 +- > drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c > index 9a75733e3f8f..273615c8923c 100644 > --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c > +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c > @@ -1152,7 +1152,7 @@ static int prueth_netdev_init(struct prueth *prueth, > > /* get mac address from DT and set private and netdev addr */ > ret = of_get_ethdev_address(eth_node, ndev); > - if (!is_valid_ether_addr(ndev->dev_addr)) { > + if (ret || !is_valid_ether_addr(ndev->dev_addr)) { > eth_hw_addr_random(ndev); > dev_warn(prueth->dev, "port %d: using random MAC addr: %pM\n", > port, ndev->dev_addr); > diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c b/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c > index 64a19ff39562..61c5e10e7077 100644 > --- a/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c > +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c > @@ -862,7 +862,7 @@ static int prueth_netdev_init(struct prueth *prueth, > > /* get mac address from DT and set private and netdev addr */ > ret = of_get_ethdev_address(eth_node, ndev); > - if (!is_valid_ether_addr(ndev->dev_addr)) { > + if (ret || !is_valid_ether_addr(ndev->dev_addr)) { > eth_hw_addr_random(ndev); > dev_warn(prueth->dev, "port %d: using random MAC addr: %pM\n", > port, ndev->dev_addr);
On Sun, 23 Mar 2025 09:19:35 +0200 Roger Quadros <rogerq@kernel.org> wrote: > Did you actually get a kernel oops? Yes. And I would like to attach the kernel output, but I do not have access to the board ATM. > If yes, which part of code produces the oops. I get an NULL pointer dereference in is_multicast_ether_addr(). It happens here: u32 a = *(const u32 *)addr; > Even if it fails we do set a random MAC address and do not return > error. So above statement is false. I doubt that. of_get_ethdev_address() do not set a random MAC address in case of a failure. It simply returns -ENODEV. Since is_valid_ether_addr() fails with a NULL pointer dereference in is_multicast_ether_addr() on the other hand, no random MAC address is set. > > Check the return value of of_get_ethdev_address() before validating > > the MAC address. > > If of_get_ethdev_address() fails the netdev address will remain zero > (as it was zero initialized during allocation) so > is_valid_ether_addr() will fail as well. Yes. It will fail to. But is_valid_ether_addr() is not called any more. Due to the if statement is_valid_ether_addr() is only called, if of_get_ethdev_address() exits with 0 aka success. In case of a failure the if statement is true and there is no call to is_valid_ether_addr(). Regards Bene
On 23/03/2025 17:18, Benedikt Spranger wrote: > On Sun, 23 Mar 2025 09:19:35 +0200 > Roger Quadros <rogerq@kernel.org> wrote: > >> Did you actually get a kernel oops? > Yes. And I would like to attach the kernel output, but I do not have > access to the board ATM. > >> If yes, which part of code produces the oops. > I get an NULL pointer dereference in is_multicast_ether_addr(). > It happens here: > > u32 a = *(const u32 *)addr; But this should not happen. Because ndev->addr (pointer) should not be zero. Driver allocated ndev with alloc_etherdev_mq() which allocates memory for ndev->addr using dev_addr_init(dev)). > >> Even if it fails we do set a random MAC address and do not return >> error. So above statement is false. > I doubt that. of_get_ethdev_address() do not set a random MAC address > in case of a failure. It simply returns -ENODEV. Since > is_valid_ether_addr() fails with a NULL pointer dereference in > is_multicast_ether_addr() on the other hand, no random MAC address is > set. What I meant was we set random address using eth_hw_addr_random(). > >>> Check the return value of of_get_ethdev_address() before validating >>> the MAC address. >> >> If of_get_ethdev_address() fails the netdev address will remain zero >> (as it was zero initialized during allocation) so >> is_valid_ether_addr() will fail as well. > Yes. It will fail to. But is_valid_ether_addr() is not called any more. > > Due to the if statement is_valid_ether_addr() is only called, if > of_get_ethdev_address() exits with 0 aka success. In case of a failure > the if statement is true and there is no call to is_valid_ether_addr(). > > Regards > Bene
On Mon, 24 Mar 2025 16:44:20 +0200 Roger Quadros <rogerq@kernel.org> wrote: > On 23/03/2025 17:18, Benedikt Spranger wrote: > > On Sun, 23 Mar 2025 09:19:35 +0200 > > Roger Quadros <rogerq@kernel.org> wrote: > > > >> Did you actually get a kernel oops? > > Yes. And I would like to attach the kernel output, but I do not have > > access to the board ATM. > > > >> If yes, which part of code produces the oops. > > I get an NULL pointer dereference in is_multicast_ether_addr(). > > It happens here: > > > > u32 a = *(const u32 *)addr; > > But this should not happen. Because ndev->addr (pointer) should not > be zero. Driver allocated ndev with alloc_etherdev_mq() which > allocates memory for ndev->addr using dev_addr_init(dev)). Emphasis on *should* :) OK, got your point. Dig deeper into that. > >> Even if it fails we do set a random MAC address and do not return > >> error. So above statement is false. > > I doubt that. of_get_ethdev_address() do not set a random MAC > > address in case of a failure. It simply returns -ENODEV. Since > > is_valid_ether_addr() fails with a NULL pointer dereference in > > is_multicast_ether_addr() on the other hand, no random MAC address > > is set. > > What I meant was we set random address using eth_hw_addr_random(). But that happens after the failing check. So evaluating the return of of_get_ethdev_address() seem to be a good thing in the first place. I my understanding (for now) it is nessesary to check both: the return of of_get_ethdev_address() *and* !is_valid_ether_addr(). If any of these checks fail eth_hw_addr_random() should be called and therefore a random MAC address be set. Regards Bene
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c index 9a75733e3f8f..273615c8923c 100644 --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c @@ -1152,7 +1152,7 @@ static int prueth_netdev_init(struct prueth *prueth, /* get mac address from DT and set private and netdev addr */ ret = of_get_ethdev_address(eth_node, ndev); - if (!is_valid_ether_addr(ndev->dev_addr)) { + if (ret || !is_valid_ether_addr(ndev->dev_addr)) { eth_hw_addr_random(ndev); dev_warn(prueth->dev, "port %d: using random MAC addr: %pM\n", port, ndev->dev_addr); diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c b/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c index 64a19ff39562..61c5e10e7077 100644 --- a/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c @@ -862,7 +862,7 @@ static int prueth_netdev_init(struct prueth *prueth, /* get mac address from DT and set private and netdev addr */ ret = of_get_ethdev_address(eth_node, ndev); - if (!is_valid_ether_addr(ndev->dev_addr)) { + if (ret || !is_valid_ether_addr(ndev->dev_addr)) { eth_hw_addr_random(ndev); dev_warn(prueth->dev, "port %d: using random MAC addr: %pM\n", port, ndev->dev_addr);
of_get_ethdev_address() may fail, do not set a MAC address and return an error. The icssg-prueth driver ignores that failure and try to validate the MAC address. This let to a NULL pointer dereference in this case. Check the return value of of_get_ethdev_address() before validating the MAC address. Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de> --- drivers/net/ethernet/ti/icssg/icssg_prueth.c | 2 +- drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)