netdev - Re: [PATCH net 1/2] ip_tunnel: Fix a memory corruption in ip_tunnel

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]

Message-ID: <CALnjE+pkaAwiSp_S=TE70L+QvyZr1TzEC48Dnz9+8Hy3fbnOyQ@mail.gmail.com>
Date:	Fri, 27 Sep 2013 19:34:59 -0700
From:	Pravin Shelar <pshelar@...ira.com>
To:	Steffen Klassert <steffen.klassert@...unet.com>
Cc:	David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net 1/2] ip_tunnel: Fix a memory corruption in ip_tunnel_xmit

On Fri, Sep 27, 2013 at 12:45 AM, Steffen Klassert
<steffen.klassert@...unet.com> wrote:
> On Thu, Sep 26, 2013 at 11:25:01AM -0700, Pravin Shelar wrote:
>> On Thu, Sep 26, 2013 at 1:25 AM, Steffen Klassert
>> <steffen.klassert@...unet.com> wrote:
>> > On Wed, Sep 25, 2013 at 09:55:50AM -0700, Pravin Shelar wrote:
>> >> On Tue, Sep 24, 2013 at 10:54 PM, Steffen Klassert
>> >> <steffen.klassert@...unet.com> wrote:
>> >> > We might extend the used aera of a skb beyond the total
>> >> > headroom when we install the ipip header. Fix this by
>> >> > calling skb_cow_head() unconditionally.
>> >> >
>> >> It is better to call skb_cow_head() from ipip_tunnel_xmit() as it is
>> >> consistent with gre.
>> >
>> > I think this would just move the bug from ipip to gre. ipgre_xmit()
>> > uses dev->needed_headroom which is based on the guessed output device
>> > in ip_tunnel_bind_dev(). If the device we get from the route lookup
>> > in ip_tunnel_xmit() is different from the guessed one and the resulting
>> > max_headroom is bigger than dev->needed_headroom, we run into that bug
>> > because skb_cow_head() will not be called with the updated
>> > dev->needed_headroom.
>> >
>> Thats why ip_tunnel_xmit() update dev->needed_headroom.
>> Just to be clear I was talking abt calling skb_cow_head with
>> dev->needed_headroom in ipip_xmit and leave ip_tunnel_xmit as it is.
>> So that most of cases we will not need to adjust headroom in ip_tunnel
>> xmit.
>
> skb_cow_head() does not do much if there is enough headroom, so
> calling it here is uncritical. But we should adjust the headroom
> as soon as we know that it is insufficient.
>
ok.

> Also, I really wonder how you want to adjust the headroom in
> ipip_tunnel_xmit() to a correct value. We know the needed
> headroom after the route lookup in ip_tunnel_xmit() and
> we have to adust it here because ip_tunnel_xmit() calls
> iptunnel_xmit() which does a __skb_push() before it
> installs the IP header.
>
> Please keep in mind tat this is a bug fix that might be interesting
> for stable too, we should try to keep the changes at a minimum.
>
> Another thing that I noticed, with commit 0e6fbc5b
> (ip_tunnels: extend iptunnel_xmit()) you moved the IP header
> installation to iptunnel_xmit() and changed skb_push()
> to __skb_push(). This made this bug quite hard to track
> down because instead of triggering a skb under panic,
> it did a silent memory corruption and crashed at random
> other places. Maybe we should change this back to skb_push().

All callers of iptunnel_xmit() are required to setup sufficient
headroom. So skb_push check are not necessary.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html