netdev - Re: [PATCH net-next] icmp: support rfc 4884

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Jun 2020 17:36:58 -0700
From:   Tom Herbert <tom@...bertland.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next] icmp: support rfc 4884

On Mon, Jun 29, 2020 at 4:07 PM Eric Dumazet <eric.dumazet@...il.com> wrote:
>
>
>
> On 6/29/20 2:30 PM, Willem de Bruijn wrote:
> > On Mon, Jun 29, 2020 at 5:15 PM Eric Dumazet <eric.dumazet@...il.com> wrote:
> >>
> >>
> >>
> >> On 6/29/20 9:57 AM, Willem de Bruijn wrote:
> >>> From: Willem de Bruijn <willemb@...gle.com>
> >>>
> >>> ICMP messages may include an extension structure after the original
> >>> datagram. RFC 4884 standardized this behavior.
> >>>
> >>> It introduces an explicit original datagram length field in the ICMP
> >>> header to delineate the original datagram from the extension struct.
> >>>
> >>> Return this field when reading an ICMP error from the error queue.
> >>
> >> RFC mentions a 'length' field of 8 bits, your patch chose to export the whole
> >> second word of icmp header.
> >>
> >> Why is this field mapped to a prior one (icmp_hdr(skb)->un.gateway) ?
> >>
> >> Should we add an element in the union to make this a little bit more explicit/readable ?
> >>
> >> diff --git a/include/uapi/linux/icmp.h b/include/uapi/linux/icmp.h
> >> index 5589eeb791ca580bb182e1dc38c05eab1c75adb9..427ed5a6765316a4c1e2fa06f3b6618447c01564 100644
> >> --- a/include/uapi/linux/icmp.h
> >> +++ b/include/uapi/linux/icmp.h
> >> @@ -76,6 +76,7 @@ struct icmphdr {
> >>                 __be16  sequence;
> >>         } echo;
> >>         __be32  gateway;
> >> +       __be32  second_word; /* RFC 4884 4.[123] : <unused:8>,<length:8>,<mtu:16> */
> >>         struct {
> >>                 __be16  __unused;
> >>                 __be16  mtu;
> >
> > Okay. How about a variant of the existing struct frag?
> >
> > @@ -80,6 +80,11 @@ struct icmphdr {
> >                 __be16  __unused;
> >                 __be16  mtu;
> >         } frag;
> > +       struct {
> > +               __u8    __unused;
> > +               __u8    length;
> > +               __be16  mtu;
> > +       } rfc_4884;
> >         __u8    reserved[4];
> >    } un;
> >
>
> Sure, but my point was later in the code :
>
> >>> +     if (inet_sk(sk)->recverr_rfc4884)
> >>> +             info = ntohl(icmp_hdr(skb)->un.gateway);
> >>
> >> ntohl(icmp_hdr(skb)->un.second_word);
>
> If you leave there "info = ntohl(icmp_hdr(skb)->un.gateway)" it is a bit hard for someone
> reading linux kernel code to understand why we do this.
>
It's also potentially problematic. The other bits are Unused, which
isn't the same thing as necessarily being zero. Userspace might assume
that info is the length without checking its bounded.

>