lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CA+FuTScQW+1JyN0ROjgY+QWAw-ZWg0iztt5s2S6-Sqtj6XSqQQ@mail.gmail.com>
Date:   Thu, 24 Jun 2021 18:01:27 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Andreas Roeseler <andreas.a.roeseler@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        yoshfuji@...ux-ipv6.org, dsahern@...nel.org, kuba@...nel.org,
        willemdebruijn.kernel@...il.com
Subject: Re: [PATCH net-next V2 1/1] ipv6: ICMPV6: add response to ICMPV6 RFC
 8335 PROBE messages

On Thu, Jun 24, 2021 at 3:04 PM Andreas Roeseler
<andreas.a.roeseler@...il.com> wrote:
>
> This patch builds off of commit 2b246b2569cd2ac6ff700d0dce56b8bae29b1842
> and adds functionality to respond to ICMPV6 PROBE requests.

No need to structure as a series (1/1) if a single patch, btw.

> Add icmp_build_probe function to construct PROBE requests for both
> ICMPV4 and ICMPV6.
>
> Modify icmpv6_rcv to detect ICMPV6 PROBE messages and call the
> icmpv6_echo_reply handler.
>
> Modify icmpv6_echo_reply to build a PROBE response message based on the
> queried interface.
>
> This patch has been tested using a branch of the iputils git repo which can
> be found here: https://github.com/Juniper-Clinic-2020/iputils/tree/probe-request
>
> Signed-off-by: Andreas Roeseler <andreas.a.roeseler@...il.com>
> ---
> Changes:
> v1 -> v2:
> Suggested by: Willem de Bruijn <willemdebruijn.kernel@...il.com>
> - Do not add sysctl for ICMPV6 PROBE control and instead use existing
>   ICMPV4 sysctl.
> - Add icmp_build_probe function to construct PROBE responses for both
>   ICMPV4 and ICMPV6.
> ---
>  include/net/icmp.h |   1 +
>  net/ipv4/icmp.c    | 101 +++++++++++++++++++++++++++------------------
>  net/ipv6/icmp.c    |  24 +++++++++--
>  3 files changed, 83 insertions(+), 43 deletions(-)
>
> diff --git a/include/net/icmp.h b/include/net/icmp.h
> index fd84adc47963..caddf4a59ad1 100644
> --- a/include/net/icmp.h
> +++ b/include/net/icmp.h
> @@ -57,5 +57,6 @@ int icmp_rcv(struct sk_buff *skb);
>  int icmp_err(struct sk_buff *skb, u32 info);
>  int icmp_init(void);
>  void icmp_out_count(struct net *net, unsigned char type);
> +bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr);
>
>  #endif /* _ICMP_H */
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 0a57f1892e7e..27a6fa29e2d3 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -977,56 +977,37 @@ static bool icmp_redirect(struct sk_buff *skb)
>         return true;
>  }
>
> -/*
> - *     Handle ICMP_ECHO ("ping") and ICMP_EXT_ECHO ("PROBE") requests.
> +/*     Helper for icmp_echo and icmpv6_echo_reply.
> + *     Searches for net_device that matches PROBE interface identifier
> + *             and builds PROBE reply message in icmphdr.
>   *
> - *     RFC 1122: 3.2.2.6 MUST have an echo server that answers ICMP echo
> - *               requests.
> - *     RFC 1122: 3.2.2.6 Data received in the ICMP_ECHO request MUST be
> - *               included in the reply.
> - *     RFC 1812: 4.3.3.6 SHOULD have a config option for silently ignoring
> - *               echo requests, MUST have default=NOT.
> - *     RFC 8335: 8 MUST have a config option to enable/disable ICMP
> - *               Extended Echo Functionality, MUST be disabled by default
> - *     See also WRT handling of options once they are done and working.
> + *     Returns false if PROBE responses are disabled via sysctl
>   */
>
> -static bool icmp_echo(struct sk_buff *skb)
> +bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)

It's a bit unusual, but I suggest moving this helper below icmp_echo,
to reduce patch size and make the patch a bit more readable:

$ git show --stat | grep changed
 3 files changed, 83 insertions(+), 43 deletions(-)

After move:

$ git diff HEAD~1 --stat | grep changed
 3 files changed, 61 insertions(+), 22 deletions(-)


>  {
>         struct icmp_ext_hdr *ext_hdr, _ext_hdr;
>         struct icmp_ext_echo_iio *iio, _iio;
> -       struct icmp_bxm icmp_param;
> +       struct net *net = dev_net(skb->dev);
>         struct net_device *dev;
>         char buff[IFNAMSIZ];
> -       struct net *net;
>         u16 ident_len;
>         u8 status;
>
> -       net = dev_net(skb_dst(skb)->dev);
> -       /* should there be an ICMP stat for ignored echos? */
> -       if (net->ipv4.sysctl_icmp_echo_ignore_all)
> -               return true;
> -
> -       icmp_param.data.icmph      = *icmp_hdr(skb);
> -       icmp_param.skb             = skb;
> -       icmp_param.offset          = 0;
> -       icmp_param.data_len        = skb->len;
> -       icmp_param.head_len        = sizeof(struct icmphdr);
> -
> -       if (icmp_param.data.icmph.type == ICMP_ECHO) {
> -               icmp_param.data.icmph.type = ICMP_ECHOREPLY;
> -               goto send_reply;
> -       }
>         if (!net->ipv4.sysctl_icmp_echo_enable_probe)
> -               return true;
> +               return false;
> +
>         /* We currently only support probing interfaces on the proxy node
>          * Check to ensure L-bit is set
>          */
> -       if (!(ntohs(icmp_param.data.icmph.un.echo.sequence) & 1))
> -               return true;
> +       if (!(ntohs(icmphdr->un.echo.sequence) & 1))
> +               return false;
>         /* Clear status bits in reply message */
> -       icmp_param.data.icmph.un.echo.sequence &= htons(0xFF00);
> -       icmp_param.data.icmph.type = ICMP_EXT_ECHOREPLY;
> +       icmphdr->un.echo.sequence &= htons(0xFF00);
> +       if (icmphdr->type == ICMP_EXT_ECHO)
> +               icmphdr->type = ICMP_EXT_ECHOREPLY;
> +       else
> +               icmphdr->type = ICMPV6_EXT_ECHO_REPLY;
>         ext_hdr = skb_header_pointer(skb, 0, sizeof(_ext_hdr), &_ext_hdr);
>         /* Size of iio is class_type dependent.
>          * Only check header here and assign length based on ctype in the switch statement
> @@ -1087,8 +1068,8 @@ static bool icmp_echo(struct sk_buff *skb)
>                 goto send_mal_query;
>         }
>         if (!dev) {
> -               icmp_param.data.icmph.code = ICMP_EXT_CODE_NO_IF;
> -               goto send_reply;
> +               icmphdr->code = ICMP_EXT_CODE_NO_IF;
> +               return true;
>         }
>         /* Fill bits in reply message */
>         if (dev->flags & IFF_UP)
> @@ -1098,13 +1079,53 @@ static bool icmp_echo(struct sk_buff *skb)
>         if (!list_empty(&rcu_dereference(dev->ip6_ptr)->addr_list))
>                 status |= ICMP_EXT_ECHOREPLY_IPV6;
>         dev_put(dev);
> -       icmp_param.data.icmph.un.echo.sequence |= htons(status);
> +       icmphdr->un.echo.sequence |= htons(status);
> +       return true;
> +send_mal_query:
> +       icmphdr->code = ICMP_EXT_CODE_MAL_QUERY;
> +       return true;
> +}

Needs EXPORT_SYMBOL_GPL to use it from IPv6 when compiled as a module.

> +
> +/*
> + *     Handle ICMP_ECHO ("ping") and ICMP_EXT_ECHO ("PROBE") requests.
> + *
> + *     RFC 1122: 3.2.2.6 MUST have an echo server that answers ICMP echo
> + *               requests.
> + *     RFC 1122: 3.2.2.6 Data received in the ICMP_ECHO request MUST be
> + *               included in the reply.
> + *     RFC 1812: 4.3.3.6 SHOULD have a config option for silently ignoring
> + *               echo requests, MUST have default=NOT.
> + *     RFC 8335: 8 MUST have a config option to enable/disable ICMP
> + *               Extended Echo Functionality, MUST be disabled by default
> + *     See also WRT handling of options once they are done and working.
> + */
> +
> +static bool icmp_echo(struct sk_buff *skb)
> +{
> +       struct icmp_bxm icmp_param;
> +       struct net *net;
> +
> +       net = dev_net(skb_dst(skb)->dev);
> +       /* should there be an ICMP stat for ignored echos? */
> +       if (net->ipv4.sysctl_icmp_echo_ignore_all)
> +               return true;
> +
> +       icmp_param.data.icmph      = *icmp_hdr(skb);
> +       icmp_param.skb             = skb;
> +       icmp_param.offset          = 0;
> +       icmp_param.data_len        = skb->len;
> +       icmp_param.head_len        = sizeof(struct icmphdr);
> +
> +       if (icmp_param.data.icmph.type == ICMP_ECHO) {
> +               icmp_param.data.icmph.type = ICMP_ECHOREPLY;
> +               goto send_reply;
> +       }
> +
> +       if (!icmp_build_probe(skb, &icmp_param.data.icmph))
> +               return true;

This control flow can now be simplified:

  if (icmp_param.data.icmph.type == ICMP_ECHO)
               icmp_param.data.icmph.type = ICMP_ECHOREPLY;
  else if !icmp_build_probe(skb, &icmp_param.data.icmph))
               return true;

and can remove the send_reply label

>  send_reply:
>         icmp_reply(&icmp_param, skb);
>                 return true;
> -send_mal_query:
> -       icmp_param.data.icmph.code = ICMP_EXT_CODE_MAL_QUERY;
> -       goto send_reply;
>  }
>
>  /*
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index e8398ffb5e35..d32a387b36e7 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -725,6 +725,7 @@ static void icmpv6_echo_reply(struct sk_buff *skb)
>         struct ipcm6_cookie ipc6;
>         u32 mark = IP6_REPLY_MARK(net, skb->mark);
>         bool acast;
> +       u8 type;
>
>         if (ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr) &&
>             net->ipv6.sysctl.icmpv6_echo_ignore_multicast)
> @@ -740,8 +741,16 @@ static void icmpv6_echo_reply(struct sk_buff *skb)
>             !(net->ipv6.sysctl.anycast_src_echo_reply && acast))
>                 saddr = NULL;
>
> +       if (icmph->icmp6_type == ICMPV6_EXT_ECHO_REQUEST) {
> +               if (!net->ipv4.sysctl_icmp_echo_enable_probe)
> +                       return;
> +               type = ICMPV6_EXT_ECHO_REPLY;
> +       } else {
> +               type = ICMPV6_ECHO_REPLY;
> +       }
> +
>         memcpy(&tmp_hdr, icmph, sizeof(tmp_hdr));
> -       tmp_hdr.icmp6_type = ICMPV6_ECHO_REPLY;
> +       tmp_hdr.icmp6_type = type;
>
>         memset(&fl6, 0, sizeof(fl6));
>         if (net->ipv6.sysctl.flowlabel_reflect & FLOWLABEL_REFLECT_ICMPV6_ECHO_REPLIES)
> @@ -752,7 +761,7 @@ static void icmpv6_echo_reply(struct sk_buff *skb)
>         if (saddr)
>                 fl6.saddr = *saddr;
>         fl6.flowi6_oif = icmp6_iif(skb);
> -       fl6.fl6_icmp_type = ICMPV6_ECHO_REPLY;
> +       fl6.fl6_icmp_type = type;
>         fl6.flowi6_mark = mark;
>         fl6.flowi6_uid = sock_net_uid(net, NULL);
>         security_skb_classify_flow(skb, flowi6_to_flowi_common(&fl6));
> @@ -783,13 +792,17 @@ static void icmpv6_echo_reply(struct sk_buff *skb)
>
>         msg.skb = skb;
>         msg.offset = 0;
> -       msg.type = ICMPV6_ECHO_REPLY;
> +       msg.type = type;
>
>         ipcm6_init_sk(&ipc6, np);
>         ipc6.hlimit = ip6_sk_dst_hoplimit(np, &fl6, dst);
>         ipc6.tclass = ipv6_get_dsfield(ipv6_hdr(skb));
>         ipc6.sockc.mark = mark;
>
> +       if (icmph->icmp6_type == ICMPV6_EXT_ECHO_REQUEST)
> +               if (!icmp_build_probe(skb, (struct icmphdr *)&tmp_hdr))
> +                       goto out_dst_release;
> +
>         if (ip6_append_data(sk, icmpv6_getfrag, &msg,
>                             skb->len + sizeof(struct icmp6hdr),
>                             sizeof(struct icmp6hdr), &ipc6, &fl6,
> @@ -912,6 +925,11 @@ static int icmpv6_rcv(struct sk_buff *skb)
>                         icmpv6_echo_reply(skb);
>                 break;
>
> +       case ICMPV6_EXT_ECHO_REQUEST:
> +               if (!net->ipv6.sysctl.icmpv6_echo_ignore_all)
> +                       icmpv6_echo_reply(skb);
> +               break;
> +

This can be a one line change to add the case to the existing case above.

But either way, you might prefer this.

>         case ICMPV6_ECHO_REPLY:
>                 success = ping_rcv(skb);
>                 break;
> --
> 2.32.0
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ