From 39245f63fe1276b846b8a1c06c76bc5f2d016e81 Mon Sep 17 00:00:00 2001
From: "chao.an" <anchao@xiaomi.com>
Date: Thu, 20 May 2021 18:16:59 +0800
Subject: [PATCH] net/icmp: fix race condition in icmp recvmsg

Signed-off-by: chao.an <anchao@xiaomi.com>
---
 net/icmp/icmp_recvmsg.c     | 169 +++++++++++++++++-----------------
 net/icmpv6/icmpv6_recvmsg.c | 176 ++++++++++++++++++------------------
 2 files changed, 175 insertions(+), 170 deletions(-)

diff --git a/net/icmp/icmp_recvmsg.c b/net/icmp/icmp_recvmsg.c
index 6fa661f2b1..013d7fe5e3 100644
--- a/net/icmp/icmp_recvmsg.c
+++ b/net/icmp/icmp_recvmsg.c
@@ -378,6 +378,8 @@ ssize_t icmp_recvmsg(FAR struct socket *psock, FAR struct msghdr *msg,
         }
     }
 
+  net_lock();
+
   /* We cannot receive a response from a device until a request has been
    * sent to the devivce.
    */
@@ -389,39 +391,6 @@ ssize_t icmp_recvmsg(FAR struct socket *psock, FAR struct msghdr *msg,
       goto errout;
     }
 
-  /* Check if there is buffered read-ahead data for this socket.  We may have
-   * already received the response to previous command.
-   */
-
-  if (!IOB_QEMPTY(&conn->readahead))
-    {
-      return icmp_readahead(conn, buf, len,
-                            (FAR struct sockaddr_in *)from, fromlen);
-    }
-
-  /* Handle non-blocking ICMP sockets */
-
-  if (_SS_ISNONBLOCK(psock->s_flags) || (flags & MSG_DONTWAIT) != 0)
-    {
-      return -EAGAIN;
-    }
-
-  /* Initialize the state structure */
-
-  memset(&state, 0, sizeof(struct icmp_recvfrom_s));
-
-  /* This semaphore is used for signaling and, hence, should not have
-   * priority inheritance enabled.
-   */
-
-  nxsem_init(&state.recv_sem, 0, 0);
-  nxsem_set_protocol(&state.recv_sem, SEM_PRIO_NONE);
-
-  state.recv_sock   = psock;    /* The IPPROTO_ICMP socket instance */
-  state.recv_result = -ENOMEM;  /* Assume allocation failure */
-  state.recv_buf    = buf;      /* Location to return the response */
-  state.recv_buflen = len;      /* Size of the response */
-
   /* Get the device that was used to send the ICMP request. */
 
   dev = conn->dev;
@@ -432,68 +401,102 @@ ssize_t icmp_recvmsg(FAR struct socket *psock, FAR struct msghdr *msg,
       goto errout;
     }
 
-  net_lock();
+  /* Check if there is buffered read-ahead data for this socket.  We may have
+   * already received the response to previous command.
+   */
 
-  /* Set up the callback */
-
-  state.recv_cb = icmp_callback_alloc(dev, conn);
-  if (state.recv_cb != NULL)
+  if (!IOB_QEMPTY(&conn->readahead))
     {
-      state.recv_cb->flags = (ICMP_NEWDATA | NETDEV_DOWN);
-      state.recv_cb->priv  = (FAR void *)&state;
-      state.recv_cb->event = recvfrom_eventhandler;
+      ret = icmp_readahead(conn, buf, len,
+                           (FAR struct sockaddr_in *)from, fromlen);
+    }
+  else if (_SS_ISNONBLOCK(psock->s_flags) || (flags & MSG_DONTWAIT) != 0)
+    {
+      /* Handle non-blocking ICMP sockets */
 
-      /* Wait for either the response to be received or for timeout to
-       * occur. net_timedwait will also terminate if a signal is received.
+      ret = -EAGAIN;
+    }
+  else
+    {
+      /* Initialize the state structure */
+
+      memset(&state, 0, sizeof(struct icmp_recvfrom_s));
+
+      /* This semaphore is used for signaling and, hence, should not have
+       * priority inheritance enabled.
        */
 
-      ret = net_timedwait(&state.recv_sem, _SO_TIMEOUT(psock->s_rcvtimeo));
-      if (ret < 0)
+      nxsem_init(&state.recv_sem, 0, 0);
+      nxsem_set_protocol(&state.recv_sem, SEM_PRIO_NONE);
+
+      state.recv_sock   = psock;    /* The IPPROTO_ICMP socket instance */
+      state.recv_result = -ENOMEM;  /* Assume allocation failure */
+      state.recv_buf    = buf;      /* Location to return the response */
+      state.recv_buflen = len;      /* Size of the response */
+
+      /* Set up the callback */
+
+      state.recv_cb = icmp_callback_alloc(dev, conn);
+      if (state.recv_cb != NULL)
         {
-          state.recv_result = ret;
+          state.recv_cb->flags = (ICMP_NEWDATA | NETDEV_DOWN);
+          state.recv_cb->priv  = (FAR void *)&state;
+          state.recv_cb->event = recvfrom_eventhandler;
+
+          /* Wait for either the response to be received or for timeout to
+           * occur. net_timedwait will also terminate if a signal is
+           * received.
+           */
+
+          ret = net_timedwait(&state.recv_sem,
+                              _SO_TIMEOUT(psock->s_rcvtimeo));
+          if (ret < 0)
+            {
+              state.recv_result = ret;
+            }
+
+          icmp_callback_free(dev, conn, state.recv_cb);
         }
 
-      icmp_callback_free(dev, conn, state.recv_cb);
+      /* Return the negated error number in the event of a failure, or the
+       * number of bytes received on success.
+       */
+
+      if (state.recv_result < 0)
+        {
+          nerr("ERROR: Return error=%d\n", state.recv_result);
+          ret = state.recv_result;
+          goto errout;
+        }
+
+      if (from != NULL)
+        {
+          inaddr             = (FAR struct sockaddr_in *)from;
+          inaddr->sin_family = AF_INET;
+          inaddr->sin_port   = 0;
+
+          net_ipv4addr_copy(inaddr->sin_addr.s_addr, state.recv_from);
+        }
+
+      ret = state.recv_result;
+
+      /* If there a no further outstanding requests,
+       * make sure that the request struct is left pristine.
+       */
+
+errout:
+      if (conn->nreqs < 1)
+        {
+          conn->id    = 0;
+          conn->nreqs = 0;
+          conn->dev   = NULL;
+
+          iob_free_queue(&conn->readahead, IOBUSER_NET_SOCK_ICMP);
+        }
     }
 
   net_unlock();
 
-  /* Return the negated error number in the event of a failure, or the
-   * number of bytes received on success.
-   */
-
-  if (state.recv_result < 0)
-    {
-      nerr("ERROR: Return error=%d\n", state.recv_result);
-      ret = state.recv_result;
-      goto errout;
-    }
-
-  if (from != NULL)
-    {
-      inaddr             = (FAR struct sockaddr_in *)from;
-      inaddr->sin_family = AF_INET;
-      inaddr->sin_port   = 0;
-
-      net_ipv4addr_copy(inaddr->sin_addr.s_addr, state.recv_from);
-    }
-
-  ret = state.recv_result;
-
-  /* If there a no further outstanding requests, make sure that the request
-   * struct is left pristine.
-   */
-
-errout:
-  if (conn->nreqs < 1)
-    {
-      conn->id    = 0;
-      conn->nreqs = 0;
-      conn->dev   = NULL;
-
-      iob_free_queue(&conn->readahead, IOBUSER_NET_SOCK_ICMP);
-    }
-
   return ret;
 }
 
diff --git a/net/icmpv6/icmpv6_recvmsg.c b/net/icmpv6/icmpv6_recvmsg.c
index ac8839f386..0c764a771e 100644
--- a/net/icmpv6/icmpv6_recvmsg.c
+++ b/net/icmpv6/icmpv6_recvmsg.c
@@ -385,6 +385,8 @@ ssize_t icmpv6_recvmsg(FAR struct socket *psock, FAR struct msghdr *msg,
         }
     }
 
+  net_lock();
+
   /* We cannot receive a response from a device until a request has been
    * sent to the devivce.
    */
@@ -396,39 +398,6 @@ ssize_t icmpv6_recvmsg(FAR struct socket *psock, FAR struct msghdr *msg,
       goto errout;
     }
 
-  /* Check if there is buffered read-ahead data for this socket.  We may have
-   * already received the response to previous command.
-   */
-
-  if (!IOB_QEMPTY(&conn->readahead))
-    {
-      return icmpv6_readahead(conn, buf, len,
-                            (FAR struct sockaddr_in6 *)from, fromlen);
-    }
-
-  /* Handle non-blocking ICMP sockets */
-
-  if (_SS_ISNONBLOCK(psock->s_flags) || (flags & MSG_DONTWAIT) != 0)
-    {
-      return -EAGAIN;
-    }
-
-  /* Initialize the state structure */
-
-  memset(&state, 0, sizeof(struct icmpv6_recvfrom_s));
-
-  /* This semaphore is used for signaling and, hence, should not have
-   * priority inheritance enabled.
-   */
-
-  nxsem_init(&state.recv_sem, 0, 0);
-  nxsem_set_protocol(&state.recv_sem, SEM_PRIO_NONE);
-
-  state.recv_sock   = psock;    /* The IPPROTO_ICMP6 socket instance */
-  state.recv_result = -ENOMEM;  /* Assume allocation failure */
-  state.recv_buf    = buf;      /* Location to return the response */
-  state.recv_buflen = len;      /* Size of the response */
-
   /* Get the device that was used to send the ICMPv6 request. */
 
   dev = conn->dev;
@@ -439,72 +408,105 @@ ssize_t icmpv6_recvmsg(FAR struct socket *psock, FAR struct msghdr *msg,
       goto errout;
     }
 
-  net_lock();
+  /* Check if there is buffered read-ahead data for this socket.  We may have
+   * already received the response to previous command.
+   */
 
-  /* Set up the callback */
-
-  state.recv_cb = icmpv6_callback_alloc(dev, conn);
-  if (state.recv_cb)
+  if (!IOB_QEMPTY(&conn->readahead))
     {
-      state.recv_cb->flags = (ICMPv6_NEWDATA | NETDEV_DOWN);
-      state.recv_cb->priv  = (FAR void *)&state;
-      state.recv_cb->event = recvfrom_eventhandler;
+      ret = icmpv6_readahead(conn, buf, len,
+                             (FAR struct sockaddr_in6 *)from, fromlen);
+    }
+  else if (_SS_ISNONBLOCK(psock->s_flags) || (flags & MSG_DONTWAIT) != 0)
+    {
+      /* Handle non-blocking ICMP sockets */
 
-      /* Wait for either the response to be received or for timeout to
-       * occur. (1) net_timedwait will also terminate if a signal is
-       * received, (2) interrupts may be disabled!  They will be re-enabled
-       * while the task sleeps and automatically re-enabled when the task
-       * restarts.
+      ret = -EAGAIN;
+    }
+  else
+    {
+      /* Initialize the state structure */
+
+      memset(&state, 0, sizeof(struct icmpv6_recvfrom_s));
+
+      /* This semaphore is used for signaling and, hence, should not have
+       * priority inheritance enabled.
        */
 
-      ret = net_timedwait(&state.recv_sem, _SO_TIMEOUT(psock->s_rcvtimeo));
-      if (ret < 0)
+      nxsem_init(&state.recv_sem, 0, 0);
+      nxsem_set_protocol(&state.recv_sem, SEM_PRIO_NONE);
+
+      state.recv_sock   = psock;    /* The IPPROTO_ICMP6 socket instance */
+      state.recv_result = -ENOMEM;  /* Assume allocation failure */
+      state.recv_buf    = buf;      /* Location to return the response */
+      state.recv_buflen = len;      /* Size of the response */
+
+      /* Set up the callback */
+
+      state.recv_cb = icmpv6_callback_alloc(dev, conn);
+      if (state.recv_cb)
         {
-          state.recv_result = ret;
+          state.recv_cb->flags = (ICMPv6_NEWDATA | NETDEV_DOWN);
+          state.recv_cb->priv  = (FAR void *)&state;
+          state.recv_cb->event = recvfrom_eventhandler;
+
+          /* Wait for either the response to be received or for timeout to
+           * occur. (1) net_timedwait will also terminate if a signal is
+           * received, (2) interrupts may be disabled!  They will be
+           * re-enabled while the task sleeps and automatically re-enabled
+           * when the task restarts.
+           */
+
+          ret = net_timedwait(&state.recv_sem,
+                              _SO_TIMEOUT(psock->s_rcvtimeo));
+          if (ret < 0)
+            {
+              state.recv_result = ret;
+            }
+
+          icmpv6_callback_free(dev, conn, state.recv_cb);
         }
 
-      icmpv6_callback_free(dev, conn, state.recv_cb);
+      /* Return the negated error number in the event of a failure, or the
+       * number of bytes received on success.
+       */
+
+      if (state.recv_result < 0)
+        {
+          nerr("ERROR: Return error=%d\n", state.recv_result);
+          ret = state.recv_result;
+          goto errout;
+        }
+
+      if (from != NULL)
+        {
+          inaddr              = (FAR struct sockaddr_in6 *)from;
+          inaddr->sin6_family = AF_INET6;
+          inaddr->sin6_port   = 0;
+
+          net_ipv6addr_copy(inaddr->sin6_addr.s6_addr16,
+                            state.recv_from.s6_addr16);
+        }
+
+      ret = state.recv_result;
+
+      /* If there a no further outstanding requests,
+       * make sure that the request struct is left pristine.
+       */
+
+errout:
+      if (conn->nreqs < 1)
+        {
+          conn->id    = 0;
+          conn->nreqs = 0;
+          conn->dev   = NULL;
+
+          iob_free_queue(&conn->readahead, IOBUSER_NET_SOCK_ICMPv6);
+        }
     }
 
   net_unlock();
 
-  /* Return the negated error number in the event of a failure, or the
-   * number of bytes received on success.
-   */
-
-  if (state.recv_result < 0)
-    {
-      nerr("ERROR: Return error=%d\n", state.recv_result);
-      ret = state.recv_result;
-      goto errout;
-    }
-
-  if (from != NULL)
-    {
-      inaddr              = (FAR struct sockaddr_in6 *)from;
-      inaddr->sin6_family = AF_INET6;
-      inaddr->sin6_port   = 0;
-
-      net_ipv6addr_copy(inaddr->sin6_addr.s6_addr16,
-                        state.recv_from.s6_addr16);
-    }
-
-  ret = state.recv_result;
-
-  /* If there a no further outstanding requests, make sure that the request
-   * struct is left pristine.
-   */
-
-errout:
-  if (conn->nreqs < 1)
-    {
-      conn->id    = 0;
-      conn->nreqs = 0;
-      conn->dev   = NULL;
-
-      iob_free_queue(&conn->readahead, IOBUSER_NET_SOCK_ICMPv6);
-    }
-
   return ret;
 }