| Message ID | 20240419105332.2430179-1-edumazet@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] icmp: prevent possible NULL dereferences from icmp_build_probe() | expand |
On Fri, 19 Apr 2024 10:53:32 +0000 Eric Dumazet wrote: > + in6_dev = __in6_dev_get(dev); > + if (in6_dev && !list_empty(&in6_dev->addr_list)) There's got to be some conditional include somewhere because this seems to break cut-down builds (presumably IPv6=n): net/ipv4/icmp.c: In function ‘icmp_build_probe’: net/ipv4/icmp.c:1125:19: error: implicit declaration of function ‘__in6_dev_get’; did you mean ‘in_dev_get’? [-Werror=implicit-function-declaration] 1125 | in6_dev = __in6_dev_get(dev); | ^~~~~~~~~~~~~ | in_dev_get net/ipv4/icmp.c:1125:17: error: assignment to ‘struct inet6_dev *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion] 1125 | in6_dev = __in6_dev_get(dev); | ^
On Fri, Apr 19, 2024 at 3:45 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 19 Apr 2024 10:53:32 +0000 Eric Dumazet wrote: > > + in6_dev = __in6_dev_get(dev); > > + if (in6_dev && !list_empty(&in6_dev->addr_list)) > > There's got to be some conditional include somewhere because this seems > to break cut-down builds (presumably IPv6=n): > > > net/ipv4/icmp.c: In function ‘icmp_build_probe’: > net/ipv4/icmp.c:1125:19: error: implicit declaration of function ‘__in6_dev_get’; did you mean ‘in_dev_get’? [-Werror=implicit-function-declaration] > 1125 | in6_dev = __in6_dev_get(dev); > | ^~~~~~~~~~~~~ > | in_dev_get > net/ipv4/icmp.c:1125:17: error: assignment to ‘struct inet6_dev *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion] > 1125 | in6_dev = __in6_dev_get(dev); > | ^ > -- > pw-bot: cr Ah right, __in6_dev_get() is not defined for CONFIG_IPV6=n... Thanks.
On Fri, Apr 19, 2024 at 3:51 PM Eric Dumazet <edumazet@google.com> wrote: > > On Fri, Apr 19, 2024 at 3:45 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Fri, 19 Apr 2024 10:53:32 +0000 Eric Dumazet wrote: > > > + in6_dev = __in6_dev_get(dev); > > > + if (in6_dev && !list_empty(&in6_dev->addr_list)) > > > > There's got to be some conditional include somewhere because this seems > > to break cut-down builds (presumably IPv6=n): > > > > > > net/ipv4/icmp.c: In function ‘icmp_build_probe’: > > net/ipv4/icmp.c:1125:19: error: implicit declaration of function ‘__in6_dev_get’; did you mean ‘in_dev_get’? [-Werror=implicit-function-declaration] > > 1125 | in6_dev = __in6_dev_get(dev); > > | ^~~~~~~~~~~~~ > > | in_dev_get > > net/ipv4/icmp.c:1125:17: error: assignment to ‘struct inet6_dev *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion] > > 1125 | in6_dev = __in6_dev_get(dev); > > | ^ > > -- > > pw-bot: cr > > Ah right, __in6_dev_get() is not defined for CONFIG_IPV6=n... Nope, I just have to add one include, that was conditionally pulled. diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c index e1aaad4bf09cd43d9f3b376416b79a8b2c0a63ca..437e782b9663bb59acb900c0558137ddd401cd02 100644 --- a/net/ipv4/icmp.c +++ b/net/ipv4/icmp.c @@ -92,6 +92,7 @@ #include <net/inet_common.h> #include <net/ip_fib.h> #include <net/l3mdev.h> +#include <net/addrconf.h> /* * Build xmit assembly blocks
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c index e63a3bf99617627e17669f9b3aaee1cbbf178ebf..e1aaad4bf09cd43d9f3b376416b79a8b2c0a63ca 100644 --- a/net/ipv4/icmp.c +++ b/net/ipv4/icmp.c @@ -1032,6 +1032,8 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr) struct icmp_ext_hdr *ext_hdr, _ext_hdr; struct icmp_ext_echo_iio *iio, _iio; struct net *net = dev_net(skb->dev); + struct inet6_dev *in6_dev; + struct in_device *in_dev; struct net_device *dev; char buff[IFNAMSIZ]; u16 ident_len; @@ -1115,10 +1117,15 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr) /* Fill bits in reply message */ if (dev->flags & IFF_UP) status |= ICMP_EXT_ECHOREPLY_ACTIVE; - if (__in_dev_get_rcu(dev) && __in_dev_get_rcu(dev)->ifa_list) + + in_dev = __in_dev_get_rcu(dev); + if (in_dev && rcu_access_pointer(in_dev->ifa_list)) status |= ICMP_EXT_ECHOREPLY_IPV4; - if (!list_empty(&rcu_dereference(dev->ip6_ptr)->addr_list)) + + in6_dev = __in6_dev_get(dev); + if (in6_dev && !list_empty(&in6_dev->addr_list)) status |= ICMP_EXT_ECHOREPLY_IPV6; + dev_put(dev); icmphdr->un.echo.sequence |= htons(status); return true;
First problem is a double call to __in_dev_get_rcu(), because the second one could return NULL. if (__in_dev_get_rcu(dev) && __in_dev_get_rcu(dev)->ifa_list) Second problem is a read from dev->ip6_ptr with no NULL check: if (!list_empty(&rcu_dereference(dev->ip6_ptr)->addr_list)) Use the correct RCU API to fix these. Fixes: d329ea5bd884 ("icmp: add response to RFC 8335 PROBE messages") Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Andreas Roeseler <andreas.a.roeseler@gmail.com> --- net/ipv4/icmp.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)