net: ip: guard multicast APIs against NULL args

[walidbadar]

Add NULL checks for iface and addr in IPv4/IPv6 maddr add/rm to prevent potential dereferences.

(gdb) bt
#0  z_impl_k_mutex_lock (mutex=mutex@entry=0x28, timeout=...) at /home/ubuntu/zephyrproject/zephyr/include/zephyr/arch/arm64/lib_helpers.h:73
#1  0x0000000080116158 in k_mutex_lock (mutex=mutex@entry=0x28, timeout=..., timeout@entry=...) at /home/ubuntu/zephyrproject/zephyr/build/zephyr/include/generated/zephyr/syscalls/kernel.h:1080
#2  0x0000000080116164 in net_if_lock (iface=iface@entry=0x0) at /home/ubuntu/zephyrproject/zephyr/include/zephyr/net/net_if.h:782
#3  0x0000000080117770 in net_if_ipv6_maddr_add (iface=<optimized out>, iface@entry=0x0, addr=addr@entry=0x8015e5c0 <z_main_stack+3920>) at /home/ubuntu/zephyrproject/zephyr/subsys/net/ip/net_if.c:2279
#4  0x00000000801012dc in setup_ipv6 () at /home/ubuntu/zephyrproject/zephyr/samples/net/telnet/src/telnet.c:34
#5  main () at /home/ubuntu/zephyrproject/zephyr/samples/net/telnet/src/telnet.c:45
(gdb) c
Continuing.

[pdgendt]

The documentation states that NULL can be returned:

zephyr/include/zephyr/net/net_if.h

Lines 1488 to 1493 in 33d8974

	/**
	* @brief Get the default network interface.
	*
	* @return Default interface or NULL if no interfaces are configured.
	*/
	struct net_if *net_if_get_default(void);

It is up to the caller to validate, no?

diff --git a/samples/net/telnet/src/telnet.c b/samples/net/telnet/src/telnet.c
index 1673485f14b..5f6b22c2f75 100644
--- a/samples/net/telnet/src/telnet.c
+++ b/samples/net/telnet/src/telnet.c
@@ -26,6 +26,11 @@ static void setup_ipv6(void)
 	struct net_in6_addr addr;
 	struct net_if *iface = net_if_get_default();

+	if (iface == NULL) {
+		LOG_ERR("No default interface");
+		return;
+	}
+
 	if (net_addr_pton(NET_AF_INET6, MCAST_IP6ADDR, &addr)) {
 		LOG_ERR("Invalid address: %s", MCAST_IP6ADDR);
 		return;

[rlubos]

It is up to the caller to validate, no?

True, the sample could/should check the interface, but still those NULL checks are fine additions IMO.

[pdgendt]

I get that this is defensive programming, and I'm not blocking in any way. But this is only a fraction of the entire code-base where this is now "fixed".

IMO, the NULL check on the addr doesn't make a lot of sense, but also fine.

The problem is mostly in the documentation/contract, we should make it clear where it is allowed or not allowed to pass NULL pointers.

[walidbadar]

The documentation states that NULL can be returned:

zephyr/include/zephyr/net/net_if.h

Lines 1488 to 1493 in 33d8974

	/**
	* @brief Get the default network interface.
	*
	* @return Default interface or NULL if no interfaces are configured.
	*/
	struct net_if *net_if_get_default(void);

It is up to the caller to validate, no?

diff --git a/samples/net/telnet/src/telnet.c b/samples/net/telnet/src/telnet.c
index 1673485f14b..5f6b22c2f75 100644
--- a/samples/net/telnet/src/telnet.c
+++ b/samples/net/telnet/src/telnet.c
@@ -26,6 +26,11 @@ static void setup_ipv6(void)
 	struct net_in6_addr addr;
 	struct net_if *iface = net_if_get_default();

+	if (iface == NULL) {
+		LOG_ERR("No default interface");
+		return;
+	}
+
 	if (net_addr_pton(NET_AF_INET6, MCAST_IP6ADDR, &addr)) {
 		LOG_ERR("Invalid address: %s", MCAST_IP6ADDR);
 		return;

But in case of caller forgets to validate that NULL pointer and it is passed down, then It is also the responsibility of that API to validate.

[pdgendt]

But in case of caller forgets to validate that NULL pointer and it is passed down, then It is also the responsibility of that API to validate.

Depends, if the API states that NULL isn't a valid value, it's up to the caller to check. In this case, the API does not.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud