Re: [PATCH net-next V5 6/6] icmp: add response to RFC 8335 PROBE messages

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Wed, Mar 24, 2021 at 2:20 PM Andreas Roeseler
<andreas.a.roeseler@gmail.com> wrote:
>
> Modify the icmp_rcv function to check PROBE messages and call icmp_echo
> if a PROBE request is detected.
>
> Modify the existing icmp_echo function to respond ot both ping and PROBE
> requests.
>
> This was tested using a custom modification to the iputils package and
> wireshark. It supports IPV4 probing by name, ifindex, and probing by
> both IPV4 and IPV6 addresses. It currently does not support responding
> to probes off the proxy node (see RFC 8335 Section 2).
>
> The modification to the iputils package is still in development and can
> be found here: https://github.com/Juniper-Clinic-2020/iputils.git. It
> supports full sending functionality of PROBE requests, but currently
> does not parse the response messages, which is why Wireshark is required
> to verify the sent and recieved PROBE messages. The modification adds
> the ``-e'' flag to the command which allows the user to specify the
> interface identifier to query the probed host. An example usage would be
> <./ping -4 -e 1 [destination]> to send a PROBE request of ifindex 1 to the
> destination node.
>
> Signed-off-by: Andreas Roeseler <andreas.a.roeseler@gmail.com>

>  static bool icmp_echo(struct sk_buff *skb)
>  {
> +       struct icmp_ext_hdr *ext_hdr, _ext_hdr;
> +       struct icmp_ext_echo_iio *iio, _iio;
> +       struct icmp_bxm icmp_param;
> +       struct net_device *dev;
> +       char buff[IFNAMSIZ];
>         struct net *net;
> +       u16 ident_len;
> +       u8 status;
>
>         net = dev_net(skb_dst(skb)->dev);
> -       if (!net->ipv4.sysctl_icmp_echo_ignore_all) {
> -               struct icmp_bxm icmp_param;
> +       /* 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);
>
> -               icmp_param.data.icmph      = *icmp_hdr(skb);
> +       if (icmp_param.data.icmph.type == ICMP_ECHO) {
>                 icmp_param.data.icmph.type = ICMP_ECHOREPLY;
> -               icmp_param.skb             = skb;
> -               icmp_param.offset          = 0;
> -               icmp_param.data_len        = skb->len;
> -               icmp_param.head_len        = sizeof(struct icmphdr);
> -               icmp_reply(&icmp_param, skb);
> +               goto send_reply;
>         }
> -       /* should there be an ICMP stat for ignored echos? */
> -       return true;
> +       if (!net->ipv4.sysctl_icmp_echo_enable_probe)
> +               return true;
> +       /* 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;
> +       /* Clear status bits in reply message */
> +       icmp_param.data.icmph.un.echo.sequence &= htons(0xFF00);
> +       icmp_param.data.icmph.type = ICMP_EXT_ECHOREPLY;
> +       ext_hdr = skb_header_pointer(skb, 0, sizeof(_ext_hdr), &_ext_hdr);
> +       iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);

This requires that the packet holds a full sizeof(_iio) that is
capable of storing an ipv6 address regardless of the class_type. That
is not required by the spec, I assume.

If not requiring that, do have to do bounds checking for each
individual case, e.g., that an ifname fits in the packet if that may
be shorter than IFNAMSIZ.

> +       if (!ext_hdr || !iio)
> +               goto send_mal_query;
> +       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);

As asked in v3: this can have negative overflow?

> +       status = 0;
> +       dev = NULL;
> +       switch (iio->extobj_hdr.class_type) {
> +       case EXT_ECHO_CTYPE_NAME:
> +               if (ident_len >= IFNAMSIZ)
> +                       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 EXT_ECHO_CTYPE_INDEX:
> +               if (ident_len != sizeof(iio->ident.ifindex))
> +                       goto send_mal_query;
> +               dev = dev_get_by_index(net, ntohl(iio->ident.ifindex));
> +               break;
> +       case EXT_ECHO_CTYPE_ADDR:
> +               if (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 (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
> +                                        sizeof(struct in_addr))
> +                               goto send_mal_query;
> +                       dev = ip_dev_find(net, iio->ident.addr.ip_addr.ipv4_addr.s_addr);
> +                       break;
> +#if IS_ENABLED(CONFIG_IPV6)
> +               case ICMP_AFI_IP6:
> +                       if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
> +                                        sizeof(struct in6_addr))
> +                               goto send_mal_query;
> +                       rcu_read_lock();
> +                       dev = ipv6_stub->ipv6_dev_find(net, &iio->ident.addr.ip_addr.ipv6_addr, dev);
> +                       if (dev)
> +                               dev_hold(dev);
> +                       rcu_read_unlock();

This rcu read-size critical is not needed, because the entire receive
path is wrapped in such a section. See, for instance,
netif_receive_skb_core.

Either that, or the __in_dev_get_rcu and rcu_deference below would
require a similar critical section.