Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
MIME-Version: 1.0
References: <730408.1695292879@warthog.procyon.org.uk> <CANn89i+wUq5R2nFO8eGLp7=8Y5OiJ0fwjR+ES74gk1X4k9r0rw@mail.gmail.com>
In-Reply-To: <CANn89i+wUq5R2nFO8eGLp7=8Y5OiJ0fwjR+ES74gk1X4k9r0rw@mail.gmail.com>
From:   Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date:   Thu, 21 Sep 2023 09:16:48 -0400
Message-ID: <CAF=yD-JhsNCtP7iWCL830=JWwsKHMqo4OMb9NSgReGJK7C=_0w@mail.gmail.com>
Subject: Re: [PATCH net v3] ipv4, ipv6: Fix handling of transhdrlen in __ip{,6}_append_data()
To:     Eric Dumazet <edumazet@google.com>
Cc:     David Howells <dhowells@redhat.com>, netdev@vger.kernel.org,
        syzbot+62cbf263225ae13ff153@syzkaller.appspotmail.com,
        "David S. Miller" <davem@davemloft.net>,
        David Ahern <dsahern@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>, bpf@vger.kernel.org,
        syzkaller-bugs@googlegroups.com, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Sep 21, 2023 at 7:09=E2=80=AFAM Eric Dumazet <edumazet@google.com> =
wrote:
>
> On Thu, Sep 21, 2023 at 12:41=E2=80=AFPM David Howells <dhowells@redhat.c=
om> wrote:
> >
> >
> > Including the transhdrlen in length is a problem when the packet is
> > partially filled (e.g. something like send(MSG_MORE) happened previousl=
y)
> > when appending to an IPv4 or IPv6 packet as we don't want to repeat the
> > transport header or account for it twice.  This can happen under some
> > circumstances, such as splicing into an L2TP socket.
> >
> > The symptom observed is a warning in __ip6_append_data():
> >
> >     WARNING: CPU: 1 PID: 5042 at net/ipv6/ip6_output.c:1800 __ip6_appen=
d_data.isra.0+0x1be8/0x47f0 net/ipv6/ip6_output.c:1800
> >
> > that occurs when MSG_SPLICE_PAGES is used to append more data to an alr=
eady
> > partially occupied skbuff.  The warning occurs when 'copy' is larger th=
an
> > the amount of data in the message iterator.  This is because the reques=
ted
> > length includes the transport header length when it shouldn't.  This ca=
n be
> > triggered by, for example:
> >
> >         sfd =3D socket(AF_INET6, SOCK_DGRAM, IPPROTO_L2TP);
> >         bind(sfd, ...); // ::1
> >         connect(sfd, ...); // ::1 port 7
> >         send(sfd, buffer, 4100, MSG_MORE);
> >         sendfile(sfd, dfd, NULL, 1024);
> >
> > Fix this by only adding transhdrlen into the length if the write queue =
is
> > empty in l2tp_ip6_sendmsg(), analogously to how UDP does things.
> >
> > l2tp_ip_sendmsg() looks like it won't suffer from this problem as it bu=
ilds
> > the UDP packet itself.
> >
> > Fixes: a32e0eec7042 ("l2tp: introduce L2TPv3 IP encapsulation support f=
or IPv6")
> > Reported-by: syzbot+62cbf263225ae13ff153@syzkaller.appspotmail.com
> > Link: https://lore.kernel.org/r/0000000000001c12b30605378ce8@google.com=
/
> > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > cc: Eric Dumazet <edumazet@google.com>
> > cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > cc: "David S. Miller" <davem@davemloft.net>
> > cc: David Ahern <dsahern@kernel.org>
> > cc: Paolo Abeni <pabeni@redhat.com>
> > cc: Jakub Kicinski <kuba@kernel.org>
> > cc: netdev@vger.kernel.org
> > cc: bpf@vger.kernel.org
> > cc: syzkaller-bugs@googlegroups.com
> > ---
>
> Looks safer indeed, thanks to you and Willem !
>
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>