[PATCH net] icmp: fix icmp_ext_echo_iio parsing in icmp_build_probe

Subsystems: networking [general], networking [ipv4/ipv6], the rest

STALE1770d

5 messages, 2 authors, 2021-10-14 · open the first message on its own page

[PATCH net] icmp: fix icmp_ext_echo_iio parsing in icmp_build_probe

From: Xin Long <lucien.xin@gmail.com>
Date: 2021-10-12 08:43:13

In icmp_build_probe(), the icmp_ext_echo_iio parsing should be done
step by step and skb_header_pointer() return value should always be
checked, this patch fixes 3 places in there:

  - On case ICMP_EXT_ECHO_CTYPE_NAME, it should only copy ident.name
    from skb by skb_header_pointer(), its len is ident_len. Besides,
    the return value of skb_header_pointer() should always be checked.

  - On case ICMP_EXT_ECHO_CTYPE_INDEX, move ident_len check ahead of
    skb_header_pointer(), and also do the return value check for
    skb_header_pointer().

  - On case ICMP_EXT_ECHO_CTYPE_ADDR, before accessing iio->ident.addr.
    ctype3_hdr.addrlen, skb_header_pointer() should be called first,
    then check its return value and ident_len.
    On subcases ICMP_AFI_IP and ICMP_AFI_IP6, also do check for ident.
    addr.ctype3_hdr.addrlen and skb_header_pointer()'s return value.
    On subcase ICMP_AFI_IP, the len for skb_header_pointer() should be
    "sizeof(iio->extobj_hdr) + sizeof(iio->ident.addr.ctype3_hdr) +
    sizeof(struct in_addr)" or "ident_len".

Fixes: d329ea5bd884 ("icmp: add response to RFC 8335 PROBE messages")
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/ipv4/icmp.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 8b30cadff708..818c79266c48 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -1061,38 +1061,48 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
 	dev = NULL;
 	switch (iio->extobj_hdr.class_type) {
 	case ICMP_EXT_ECHO_CTYPE_NAME:
-		iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
 		if (ident_len >= IFNAMSIZ)
 			goto send_mal_query;
+		iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+					 ident_len, &_iio);
+		if (!iio)
+			goto send_mal_query;
 		memset(buff, 0, sizeof(buff));
 		memcpy(buff, &iio->ident.name, ident_len);
 		dev = dev_get_by_name(net, buff);
 		break;
 	case ICMP_EXT_ECHO_CTYPE_INDEX:
-		iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-					 sizeof(iio->ident.ifindex), &_iio);
 		if (ident_len != sizeof(iio->ident.ifindex))
 			goto send_mal_query;
+		iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+					 ident_len, &_iio);
+		if (!iio)
+			goto send_mal_query;
 		dev = dev_get_by_index(net, ntohl(iio->ident.ifindex));
 		break;
 	case ICMP_EXT_ECHO_CTYPE_ADDR:
-		if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-				 iio->ident.addr.ctype3_hdr.addrlen)
+		iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+					 sizeof(iio->ident.addr.ctype3_hdr), &_iio);
+		if (!iio || ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
+					 iio->ident.addr.ctype3_hdr.addrlen)
 			goto send_mal_query;
 		switch (ntohs(iio->ident.addr.ctype3_hdr.afi)) {
 		case ICMP_AFI_IP:
+			if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in_addr))
+				goto send_mal_query;
 			iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-						 sizeof(struct in_addr), &_iio);
-			if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-					 sizeof(struct in_addr))
+						 ident_len, &_iio);
+			if (!iio)
 				goto send_mal_query;
 			dev = ip_dev_find(net, iio->ident.addr.ip_addr.ipv4_addr);
 			break;
 #if IS_ENABLED(CONFIG_IPV6)
 		case ICMP_AFI_IP6:
-			iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
-			if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-					 sizeof(struct in6_addr))
+			if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in6_addr))
+				goto send_mal_query;
+			iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+						 ident_len, &_iio);
+			if (!iio)
 				goto send_mal_query;
 			dev = ipv6_stub->ipv6_dev_find(net, &iio->ident.addr.ip_addr.ipv6_addr, dev);
 			dev_hold(dev);
-- 
2.27.0

Re: [PATCH net] icmp: fix icmp_ext_echo_iio parsing in icmp_build_probe

From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-10-13 16:31:22

On Tue, 12 Oct 2021 04:43:07 -0400 Xin Long wrote:
In icmp_build_probe(), the icmp_ext_echo_iio parsing should be done
step by step and skb_header_pointer() return value should always be
checked, this patch fixes 3 places in there:

  - On case ICMP_EXT_ECHO_CTYPE_NAME, it should only copy ident.name
    from skb by skb_header_pointer(), its len is ident_len. Besides,
    the return value of skb_header_pointer() should always be checked.

  - On case ICMP_EXT_ECHO_CTYPE_INDEX, move ident_len check ahead of
    skb_header_pointer(), and also do the return value check for
    skb_header_pointer().

  - On case ICMP_EXT_ECHO_CTYPE_ADDR, before accessing iio->ident.addr.
    ctype3_hdr.addrlen, skb_header_pointer() should be called first,
    then check its return value and ident_len.
    On subcases ICMP_AFI_IP and ICMP_AFI_IP6, also do check for ident.
    addr.ctype3_hdr.addrlen and skb_header_pointer()'s return value.
    On subcase ICMP_AFI_IP, the len for skb_header_pointer() should be
    "sizeof(iio->extobj_hdr) + sizeof(iio->ident.addr.ctype3_hdr) +
    sizeof(struct in_addr)" or "ident_len".

Fixes: d329ea5bd884 ("icmp: add response to RFC 8335 PROBE messages")
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
quoted hunk
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 8b30cadff708..818c79266c48 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -1061,38 +1061,48 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
 	dev = NULL;
 	switch (iio->extobj_hdr.class_type) {
 	case ICMP_EXT_ECHO_CTYPE_NAME:
-		iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
 		if (ident_len >= IFNAMSIZ)
 			goto send_mal_query;
+		iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+					 ident_len, &_iio);
+		if (!iio)
+			goto send_mal_query;
 		memset(buff, 0, sizeof(buff));
 		memcpy(buff, &iio->ident.name, ident_len);
 		dev = dev_get_by_name(net, buff);
 		break;
 	case ICMP_EXT_ECHO_CTYPE_INDEX:
-		iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-					 sizeof(iio->ident.ifindex), &_iio);
 		if (ident_len != sizeof(iio->ident.ifindex))
 			goto send_mal_query;
+		iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+					 ident_len, &_iio);
+		if (!iio)
+			goto send_mal_query;
 		dev = dev_get_by_index(net, ntohl(iio->ident.ifindex));
 		break;
 	case ICMP_EXT_ECHO_CTYPE_ADDR:
-		if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-				 iio->ident.addr.ctype3_hdr.addrlen)
+		iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+					 sizeof(iio->ident.addr.ctype3_hdr), &_iio);
+		if (!iio || ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
+					 iio->ident.addr.ctype3_hdr.addrlen)
 			goto send_mal_query;
 		switch (ntohs(iio->ident.addr.ctype3_hdr.afi)) {
 		case ICMP_AFI_IP:
+			if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in_addr))
+				goto send_mal_query;
 			iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-						 sizeof(struct in_addr), &_iio);
-			if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-					 sizeof(struct in_addr))
+						 ident_len, &_iio);
+			if (!iio)
 				goto send_mal_query;
 			dev = ip_dev_find(net, iio->ident.addr.ip_addr.ipv4_addr);
 			break;
 #if IS_ENABLED(CONFIG_IPV6)
 		case ICMP_AFI_IP6:
-			iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
-			if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-					 sizeof(struct in6_addr))
+			if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in6_addr))
+				goto send_mal_query;
+			iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+						 ident_len, &_iio);
+			if (!iio)
 				goto send_mal_query;
 			dev = ipv6_stub->ipv6_dev_find(net, &iio->ident.addr.ip_addr.ipv6_addr, dev);
 			dev_hold(dev);
If I'm reading this right we end up with the same skb_header_pointer
call 4 times, every path ends up calling:

 skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) + ident_len, &_iio);

and looks like the skb does not get modified in between so these calls
are equivalent.

So why don't we instead consolidate the paths:
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 8b30cadff708..efa2ec1a85bf 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -1057,11 +1057,15 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
 	if (ntohs(iio->extobj_hdr.length) <= sizeof(iio->extobj_hdr))
 		goto send_mal_query;
 	ident_len = ntohs(iio->extobj_hdr.length) - sizeof(iio->extobj_hdr);
+	iio = skb_header_pointer(skb, sizeof(_ext_hdr),
+				 sizeof(iio->extobj_hdr) + ident_len, &_iio);
+	if (!iio)
+		goto send_mal_query;
+
 	status = 0;
 	dev = NULL;
 	switch (iio->extobj_hdr.class_type) {
 	case ICMP_EXT_ECHO_CTYPE_NAME:
-		iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
 		if (ident_len >= IFNAMSIZ)
 			goto send_mal_query;
 		memset(buff, 0, sizeof(buff));
@@ -1069,8 +1073,6 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
 		dev = dev_get_by_name(net, buff);
 		break;
 	case ICMP_EXT_ECHO_CTYPE_INDEX:
-		iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-					 sizeof(iio->ident.ifindex), &_iio);
 		if (ident_len != sizeof(iio->ident.ifindex))
 			goto send_mal_query;
 		dev = dev_get_by_index(net, ntohl(iio->ident.ifindex));
@@ -1081,18 +1083,13 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
 			goto send_mal_query;
 		switch (ntohs(iio->ident.addr.ctype3_hdr.afi)) {
 		case ICMP_AFI_IP:
-			iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-						 sizeof(struct in_addr), &_iio);
-			if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-					 sizeof(struct in_addr))
+			if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in_addr))
 				goto send_mal_query;
 			dev = ip_dev_find(net, iio->ident.addr.ip_addr.ipv4_addr);
 			break;
 #if IS_ENABLED(CONFIG_IPV6)
 		case ICMP_AFI_IP6:
-			iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
-			if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-					 sizeof(struct in6_addr))
+			if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in6_addr))
 				goto send_mal_query;
 			dev = ipv6_stub->ipv6_dev_find(net, &iio->ident.addr.ip_addr.ipv6_addr, dev);
 			dev_hold(dev);
-- 
2.31.1

Re: [PATCH net] icmp: fix icmp_ext_echo_iio parsing in icmp_build_probe

From: Xin Long <lucien.xin@gmail.com>
Date: 2021-10-14 03:39:31

On Thu, Oct 14, 2021 at 12:31 AM Jakub Kicinski [off-list ref] wrote:
On Tue, 12 Oct 2021 04:43:07 -0400 Xin Long wrote:
quoted
In icmp_build_probe(), the icmp_ext_echo_iio parsing should be done
step by step and skb_header_pointer() return value should always be
checked, this patch fixes 3 places in there:

  - On case ICMP_EXT_ECHO_CTYPE_NAME, it should only copy ident.name
    from skb by skb_header_pointer(), its len is ident_len. Besides,
    the return value of skb_header_pointer() should always be checked.

  - On case ICMP_EXT_ECHO_CTYPE_INDEX, move ident_len check ahead of
    skb_header_pointer(), and also do the return value check for
    skb_header_pointer().

  - On case ICMP_EXT_ECHO_CTYPE_ADDR, before accessing iio->ident.addr.
    ctype3_hdr.addrlen, skb_header_pointer() should be called first,
    then check its return value and ident_len.
    On subcases ICMP_AFI_IP and ICMP_AFI_IP6, also do check for ident.
    addr.ctype3_hdr.addrlen and skb_header_pointer()'s return value.
    On subcase ICMP_AFI_IP, the len for skb_header_pointer() should be
    "sizeof(iio->extobj_hdr) + sizeof(iio->ident.addr.ctype3_hdr) +
    sizeof(struct in_addr)" or "ident_len".

Fixes: d329ea5bd884 ("icmp: add response to RFC 8335 PROBE messages")
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
quoted
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 8b30cadff708..818c79266c48 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -1061,38 +1061,48 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
      dev = NULL;
      switch (iio->extobj_hdr.class_type) {
      case ICMP_EXT_ECHO_CTYPE_NAME:
-             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
              if (ident_len >= IFNAMSIZ)
                      goto send_mal_query;
+             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+                                      ident_len, &_iio);
+             if (!iio)
+                     goto send_mal_query;
              memset(buff, 0, sizeof(buff));
              memcpy(buff, &iio->ident.name, ident_len);
              dev = dev_get_by_name(net, buff);
              break;
      case ICMP_EXT_ECHO_CTYPE_INDEX:
-             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-                                      sizeof(iio->ident.ifindex), &_iio);
              if (ident_len != sizeof(iio->ident.ifindex))
                      goto send_mal_query;
+             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+                                      ident_len, &_iio);
+             if (!iio)
+                     goto send_mal_query;
              dev = dev_get_by_index(net, ntohl(iio->ident.ifindex));
              break;
      case ICMP_EXT_ECHO_CTYPE_ADDR:
-             if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                              iio->ident.addr.ctype3_hdr.addrlen)
+             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+                                      sizeof(iio->ident.addr.ctype3_hdr), &_iio);
+             if (!iio || ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
+                                      iio->ident.addr.ctype3_hdr.addrlen)
                      goto send_mal_query;
              switch (ntohs(iio->ident.addr.ctype3_hdr.afi)) {
              case ICMP_AFI_IP:
+                     if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in_addr))
+                             goto send_mal_query;
                      iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-                                              sizeof(struct in_addr), &_iio);
-                     if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                                      sizeof(struct in_addr))
+                                              ident_len, &_iio);
+                     if (!iio)
                              goto send_mal_query;
                      dev = ip_dev_find(net, iio->ident.addr.ip_addr.ipv4_addr);
                      break;
 #if IS_ENABLED(CONFIG_IPV6)
              case ICMP_AFI_IP6:
-                     iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
-                     if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                                      sizeof(struct in6_addr))
+                     if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in6_addr))
+                             goto send_mal_query;
+                     iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+                                              ident_len, &_iio);
+                     if (!iio)
                              goto send_mal_query;
                      dev = ipv6_stub->ipv6_dev_find(net, &iio->ident.addr.ip_addr.ipv6_addr, dev);
                      dev_hold(dev);
If I'm reading this right we end up with the same skb_header_pointer
call 4 times, every path ends up calling:

 skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) + ident_len, &_iio);

and looks like the skb does not get modified in between so these calls
are equivalent.
This looks much clearer.

I was worrying that ident_len might be too big, and ident_len should be verified
before calling skb_header_pointer(ident_len).
But it should be okay as skb_header_pointer() will return NULL if ident_len is
too big.

Will post v2 with the suggestion, thanks!
quoted hunk
So why don't we instead consolidate the paths:
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 8b30cadff708..efa2ec1a85bf 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -1057,11 +1057,15 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
        if (ntohs(iio->extobj_hdr.length) <= sizeof(iio->extobj_hdr))
                goto send_mal_query;
        ident_len = ntohs(iio->extobj_hdr.length) - sizeof(iio->extobj_hdr);
+       iio = skb_header_pointer(skb, sizeof(_ext_hdr),
+                                sizeof(iio->extobj_hdr) + ident_len, &_iio);
+       if (!iio)
+               goto send_mal_query;
+
        status = 0;
        dev = NULL;
        switch (iio->extobj_hdr.class_type) {
        case ICMP_EXT_ECHO_CTYPE_NAME:
-               iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
                if (ident_len >= IFNAMSIZ)
                        goto send_mal_query;
                memset(buff, 0, sizeof(buff));
@@ -1069,8 +1073,6 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
                dev = dev_get_by_name(net, buff);
                break;
        case ICMP_EXT_ECHO_CTYPE_INDEX:
-               iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-                                        sizeof(iio->ident.ifindex), &_iio);
                if (ident_len != sizeof(iio->ident.ifindex))
                        goto send_mal_query;
                dev = dev_get_by_index(net, ntohl(iio->ident.ifindex));
@@ -1081,18 +1083,13 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
                        goto send_mal_query;
                switch (ntohs(iio->ident.addr.ctype3_hdr.afi)) {
                case ICMP_AFI_IP:
-                       iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-                                                sizeof(struct in_addr), &_iio);
-                       if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                                        sizeof(struct in_addr))
+                       if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in_addr))
                                goto send_mal_query;
                        dev = ip_dev_find(net, iio->ident.addr.ip_addr.ipv4_addr);
                        break;
 #if IS_ENABLED(CONFIG_IPV6)
                case ICMP_AFI_IP6:
-                       iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
-                       if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                                        sizeof(struct in6_addr))
+                       if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in6_addr))
                                goto send_mal_query;
                        dev = ipv6_stub->ipv6_dev_find(net, &iio->ident.addr.ip_addr.ipv6_addr, dev);
                        dev_hold(dev);
--
2.31.1

Re: [PATCH net] icmp: fix icmp_ext_echo_iio parsing in icmp_build_probe

From: Xin Long <lucien.xin@gmail.com>
Date: 2021-10-14 03:59:56

On Thu, Oct 14, 2021 at 11:39 AM Xin Long [off-list ref] wrote:
On Thu, Oct 14, 2021 at 12:31 AM Jakub Kicinski [off-list ref] wrote:
quoted
On Tue, 12 Oct 2021 04:43:07 -0400 Xin Long wrote:
quoted
In icmp_build_probe(), the icmp_ext_echo_iio parsing should be done
step by step and skb_header_pointer() return value should always be
checked, this patch fixes 3 places in there:

  - On case ICMP_EXT_ECHO_CTYPE_NAME, it should only copy ident.name
    from skb by skb_header_pointer(), its len is ident_len. Besides,
    the return value of skb_header_pointer() should always be checked.

  - On case ICMP_EXT_ECHO_CTYPE_INDEX, move ident_len check ahead of
    skb_header_pointer(), and also do the return value check for
    skb_header_pointer().

  - On case ICMP_EXT_ECHO_CTYPE_ADDR, before accessing iio->ident.addr.
    ctype3_hdr.addrlen, skb_header_pointer() should be called first,
    then check its return value and ident_len.
    On subcases ICMP_AFI_IP and ICMP_AFI_IP6, also do check for ident.
    addr.ctype3_hdr.addrlen and skb_header_pointer()'s return value.
    On subcase ICMP_AFI_IP, the len for skb_header_pointer() should be
    "sizeof(iio->extobj_hdr) + sizeof(iio->ident.addr.ctype3_hdr) +
    sizeof(struct in_addr)" or "ident_len".

Fixes: d329ea5bd884 ("icmp: add response to RFC 8335 PROBE messages")
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
quoted
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 8b30cadff708..818c79266c48 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -1061,38 +1061,48 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
      dev = NULL;
      switch (iio->extobj_hdr.class_type) {
      case ICMP_EXT_ECHO_CTYPE_NAME:
-             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
              if (ident_len >= IFNAMSIZ)
                      goto send_mal_query;
+             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+                                      ident_len, &_iio);
+             if (!iio)
+                     goto send_mal_query;
              memset(buff, 0, sizeof(buff));
              memcpy(buff, &iio->ident.name, ident_len);
              dev = dev_get_by_name(net, buff);
              break;
      case ICMP_EXT_ECHO_CTYPE_INDEX:
-             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-                                      sizeof(iio->ident.ifindex), &_iio);
              if (ident_len != sizeof(iio->ident.ifindex))
                      goto send_mal_query;
+             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+                                      ident_len, &_iio);
+             if (!iio)
+                     goto send_mal_query;
              dev = dev_get_by_index(net, ntohl(iio->ident.ifindex));
              break;
      case ICMP_EXT_ECHO_CTYPE_ADDR:
-             if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                              iio->ident.addr.ctype3_hdr.addrlen)
+             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+                                      sizeof(iio->ident.addr.ctype3_hdr), &_iio);
+             if (!iio || ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
+                                      iio->ident.addr.ctype3_hdr.addrlen)
                      goto send_mal_query;
              switch (ntohs(iio->ident.addr.ctype3_hdr.afi)) {
              case ICMP_AFI_IP:
+                     if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in_addr))
+                             goto send_mal_query;
                      iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-                                              sizeof(struct in_addr), &_iio);
-                     if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                                      sizeof(struct in_addr))
+                                              ident_len, &_iio);
+                     if (!iio)
                              goto send_mal_query;
                      dev = ip_dev_find(net, iio->ident.addr.ip_addr.ipv4_addr);
                      break;
 #if IS_ENABLED(CONFIG_IPV6)
              case ICMP_AFI_IP6:
-                     iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
-                     if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                                      sizeof(struct in6_addr))
+                     if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in6_addr))
+                             goto send_mal_query;
+                     iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+                                              ident_len, &_iio);
+                     if (!iio)
                              goto send_mal_query;
                      dev = ipv6_stub->ipv6_dev_find(net, &iio->ident.addr.ip_addr.ipv6_addr, dev);
                      dev_hold(dev);
If I'm reading this right we end up with the same skb_header_pointer
call 4 times, every path ends up calling:

 skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) + ident_len, &_iio);

and looks like the skb does not get modified in between so these calls
are equivalent.
This looks much clearer.

I was worrying that ident_len might be too big, and ident_len should be verified
before calling skb_header_pointer(ident_len).
But it should be okay as skb_header_pointer() will return NULL if ident_len is
too big.

Will post v2 with the suggestion, thanks!
quoted
So why don't we instead consolidate the paths:
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 8b30cadff708..efa2ec1a85bf 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -1057,11 +1057,15 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
        if (ntohs(iio->extobj_hdr.length) <= sizeof(iio->extobj_hdr))
                goto send_mal_query;
        ident_len = ntohs(iio->extobj_hdr.length) - sizeof(iio->extobj_hdr);
+       iio = skb_header_pointer(skb, sizeof(_ext_hdr),
+                                sizeof(iio->extobj_hdr) + ident_len, &_iio);
+       if (!iio)
+               goto send_mal_query;
+
[a]
quoted
        status = 0;
        dev = NULL;
        switch (iio->extobj_hdr.class_type) {
        case ICMP_EXT_ECHO_CTYPE_NAME:
-               iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
                if (ident_len >= IFNAMSIZ)
                        goto send_mal_query;
                memset(buff, 0, sizeof(buff));
@@ -1069,8 +1073,6 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
                dev = dev_get_by_name(net, buff);
                break;
        case ICMP_EXT_ECHO_CTYPE_INDEX:
-               iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-                                        sizeof(iio->ident.ifindex), &_iio);
                if (ident_len != sizeof(iio->ident.ifindex))
                        goto send_mal_query;
                dev = dev_get_by_index(net, ntohl(iio->ident.ifindex));
@@ -1081,18 +1083,13 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
                        goto send_mal_query;
But, here is another check:
                if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
                                 iio->ident.addr.ctype3_hdr.addrlen)
                        goto send_mal_query;

Thinking if ident_len < sizeof(iio->ident.addr.ctype3_hdr),
iio->ident.addr.ctype3_hdr.addrlen
access could be overflow. So we may do: if (ident_len <
sizeof(iio->ident.addr.ctype3_hdr)) check
before calling skb_header_pointer() in the place [a] above, like:

        ident_len = ntohs(iio->extobj_hdr.length) - sizeof(iio->extobj_hdr);
        if (ident_len < 4) /* 4 is the minimal size for any class_type */
                goto send_mal_query;
        iio = skb_header_pointer(skb, sizeof(_ext_hdr),
                                 sizeof(iio->extobj_hdr) + ident_len, &_iio);
        if (!iio)
                goto send_mal_query;

what do you think?
quoted
                switch (ntohs(iio->ident.addr.ctype3_hdr.afi)) {
                case ICMP_AFI_IP:
-                       iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-                                                sizeof(struct in_addr), &_iio);
-                       if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                                        sizeof(struct in_addr))
+                       if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in_addr))
                                goto send_mal_query;
                        dev = ip_dev_find(net, iio->ident.addr.ip_addr.ipv4_addr);
                        break;
 #if IS_ENABLED(CONFIG_IPV6)
                case ICMP_AFI_IP6:
-                       iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
-                       if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                                        sizeof(struct in6_addr))
+                       if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in6_addr))
                                goto send_mal_query;
                        dev = ipv6_stub->ipv6_dev_find(net, &iio->ident.addr.ip_addr.ipv6_addr, dev);
                        dev_hold(dev);
--
2.31.1

Re: [PATCH net] icmp: fix icmp_ext_echo_iio parsing in icmp_build_probe

From: Xin Long <lucien.xin@gmail.com>
Date: 2021-10-14 04:08:00

On Thu, Oct 14, 2021 at 11:59 AM Xin Long [off-list ref] wrote:
On Thu, Oct 14, 2021 at 11:39 AM Xin Long [off-list ref] wrote:
quoted
On Thu, Oct 14, 2021 at 12:31 AM Jakub Kicinski [off-list ref] wrote:
quoted
On Tue, 12 Oct 2021 04:43:07 -0400 Xin Long wrote:
quoted
In icmp_build_probe(), the icmp_ext_echo_iio parsing should be done
step by step and skb_header_pointer() return value should always be
checked, this patch fixes 3 places in there:

  - On case ICMP_EXT_ECHO_CTYPE_NAME, it should only copy ident.name
    from skb by skb_header_pointer(), its len is ident_len. Besides,
    the return value of skb_header_pointer() should always be checked.

  - On case ICMP_EXT_ECHO_CTYPE_INDEX, move ident_len check ahead of
    skb_header_pointer(), and also do the return value check for
    skb_header_pointer().

  - On case ICMP_EXT_ECHO_CTYPE_ADDR, before accessing iio->ident.addr.
    ctype3_hdr.addrlen, skb_header_pointer() should be called first,
    then check its return value and ident_len.
    On subcases ICMP_AFI_IP and ICMP_AFI_IP6, also do check for ident.
    addr.ctype3_hdr.addrlen and skb_header_pointer()'s return value.
    On subcase ICMP_AFI_IP, the len for skb_header_pointer() should be
    "sizeof(iio->extobj_hdr) + sizeof(iio->ident.addr.ctype3_hdr) +
    sizeof(struct in_addr)" or "ident_len".

Fixes: d329ea5bd884 ("icmp: add response to RFC 8335 PROBE messages")
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
quoted
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 8b30cadff708..818c79266c48 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -1061,38 +1061,48 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
      dev = NULL;
      switch (iio->extobj_hdr.class_type) {
      case ICMP_EXT_ECHO_CTYPE_NAME:
-             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
              if (ident_len >= IFNAMSIZ)
                      goto send_mal_query;
+             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+                                      ident_len, &_iio);
+             if (!iio)
+                     goto send_mal_query;
              memset(buff, 0, sizeof(buff));
              memcpy(buff, &iio->ident.name, ident_len);
              dev = dev_get_by_name(net, buff);
              break;
      case ICMP_EXT_ECHO_CTYPE_INDEX:
-             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-                                      sizeof(iio->ident.ifindex), &_iio);
              if (ident_len != sizeof(iio->ident.ifindex))
                      goto send_mal_query;
+             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+                                      ident_len, &_iio);
+             if (!iio)
+                     goto send_mal_query;
              dev = dev_get_by_index(net, ntohl(iio->ident.ifindex));
              break;
      case ICMP_EXT_ECHO_CTYPE_ADDR:
-             if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                              iio->ident.addr.ctype3_hdr.addrlen)
+             iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+                                      sizeof(iio->ident.addr.ctype3_hdr), &_iio);
+             if (!iio || ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
+                                      iio->ident.addr.ctype3_hdr.addrlen)
                      goto send_mal_query;
              switch (ntohs(iio->ident.addr.ctype3_hdr.afi)) {
              case ICMP_AFI_IP:
+                     if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in_addr))
+                             goto send_mal_query;
                      iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-                                              sizeof(struct in_addr), &_iio);
-                     if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                                      sizeof(struct in_addr))
+                                              ident_len, &_iio);
+                     if (!iio)
                              goto send_mal_query;
                      dev = ip_dev_find(net, iio->ident.addr.ip_addr.ipv4_addr);
                      break;
 #if IS_ENABLED(CONFIG_IPV6)
              case ICMP_AFI_IP6:
-                     iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
-                     if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                                      sizeof(struct in6_addr))
+                     if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in6_addr))
+                             goto send_mal_query;
+                     iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
+                                              ident_len, &_iio);
+                     if (!iio)
                              goto send_mal_query;
                      dev = ipv6_stub->ipv6_dev_find(net, &iio->ident.addr.ip_addr.ipv6_addr, dev);
                      dev_hold(dev);
If I'm reading this right we end up with the same skb_header_pointer
call 4 times, every path ends up calling:

 skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) + ident_len, &_iio);

and looks like the skb does not get modified in between so these calls
are equivalent.
This looks much clearer.

I was worrying that ident_len might be too big, and ident_len should be verified
before calling skb_header_pointer(ident_len).
But it should be okay as skb_header_pointer() will return NULL if ident_len is
too big.

Will post v2 with the suggestion, thanks!
quoted
So why don't we instead consolidate the paths:
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 8b30cadff708..efa2ec1a85bf 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -1057,11 +1057,15 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
        if (ntohs(iio->extobj_hdr.length) <= sizeof(iio->extobj_hdr))
                goto send_mal_query;
        ident_len = ntohs(iio->extobj_hdr.length) - sizeof(iio->extobj_hdr);
+       iio = skb_header_pointer(skb, sizeof(_ext_hdr),
+                                sizeof(iio->extobj_hdr) + ident_len, &_iio);
+       if (!iio)
+               goto send_mal_query;
+
[a]
quoted
quoted
        status = 0;
        dev = NULL;
        switch (iio->extobj_hdr.class_type) {
        case ICMP_EXT_ECHO_CTYPE_NAME:
-               iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
                if (ident_len >= IFNAMSIZ)
                        goto send_mal_query;
                memset(buff, 0, sizeof(buff));
@@ -1069,8 +1073,6 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
                dev = dev_get_by_name(net, buff);
                break;
        case ICMP_EXT_ECHO_CTYPE_INDEX:
-               iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-                                        sizeof(iio->ident.ifindex), &_iio);
                if (ident_len != sizeof(iio->ident.ifindex))
                        goto send_mal_query;
                dev = dev_get_by_index(net, ntohl(iio->ident.ifindex));
@@ -1081,18 +1083,13 @@ bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
                        goto send_mal_query;
But, here is another check:
                if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
                                 iio->ident.addr.ctype3_hdr.addrlen)
                        goto send_mal_query;

Thinking if ident_len < sizeof(iio->ident.addr.ctype3_hdr),
iio->ident.addr.ctype3_hdr.addrlen
access could be overflow. So we may do: if (ident_len <
sizeof(iio->ident.addr.ctype3_hdr)) check
before calling skb_header_pointer() in the place [a] above, like:

        ident_len = ntohs(iio->extobj_hdr.length) - sizeof(iio->extobj_hdr);
        if (ident_len < 4) /* 4 is the minimal size for any class_type */
                goto send_mal_query;
        iio = skb_header_pointer(skb, sizeof(_ext_hdr),
                                 sizeof(iio->extobj_hdr) + ident_len, &_iio);
        if (!iio)
                goto send_mal_query;

what do you think?
Sorry, seems doing the check,

        case ICMP_EXT_ECHO_CTYPE_ADDR:
-               if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
+               if (ident_len < sizeof(iio->ident.addr.ctype3_hdr) ||
+                   ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
                                 iio->ident.addr.ctype3_hdr.addrlen)
                        goto send_mal_query;

right here makes more sense.
quoted
quoted
                switch (ntohs(iio->ident.addr.ctype3_hdr.afi)) {
                case ICMP_AFI_IP:
-                       iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
-                                                sizeof(struct in_addr), &_iio);
-                       if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                                        sizeof(struct in_addr))
+                       if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in_addr))
                                goto send_mal_query;
                        dev = ip_dev_find(net, iio->ident.addr.ip_addr.ipv4_addr);
                        break;
 #if IS_ENABLED(CONFIG_IPV6)
                case ICMP_AFI_IP6:
-                       iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
-                       if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
-                                        sizeof(struct in6_addr))
+                       if (iio->ident.addr.ctype3_hdr.addrlen != sizeof(struct in6_addr))
                                goto send_mal_query;
                        dev = ipv6_stub->ipv6_dev_find(net, &iio->ident.addr.ip_addr.ipv6_addr, dev);
                        dev_hold(dev);
--
2.31.1
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help