netdev - Re: [PATCH net-next 4/4] act_mirred: use ACT

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: <a61342740ba2a4b3c6481d2133889e93dc788eef.camel@redhat.com>
Date:   Wed, 18 Jul 2018 12:05:50 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Eyal Birger <eyal.birger@...il.com>
Subject: Re: [PATCH net-next 4/4] act_mirred: use ACT_REDIRECT when possible

Hi,

On Tue, 2018-07-17 at 10:24 -0700, Cong Wang wrote:
> If you goal is to get rid of skb_clone(), why not just do the following?
> 
>         if (tcf_mirred_is_act_redirect(m_eaction)) {
>                 skb2 = skb;
>         } else {
>                 skb2 = skb_clone(skb, GFP_ATOMIC);
>                 if (!skb2)
>                         goto out;
>         }
> 
> For redirect, we return TC_ACT_SHOT, so upper layer should not
> touch the skb after that.
> 
> What am I missing here?

With ACT_SHOT caller/upper layer will free the skb, too. We will have
an use after free (from either the upper layer and the xmit device).
Similar issues with STOLEN, TRAP, etc.

In the past, Changli Gao attempted to avoid the clone incrementing the
skb usage count:

commit 210d6de78c5d7c785fc532556cea340e517955e1
Author: Changli Gao <xiaosuo@...il.com>
Date:   Thu Jun 24 16:25:12 2010 +0000

    act_mirred: don't clone skb when skb isn't shared

but some/many device drivers expect an skb usage count of 1, and that
caused ooops and was revered.

I think the only other option (beyond re-using ACT_MIRROR) is adding
another action value, and let the upper layer re-inject the packet
while handling such action (similar to what ACT_MIRROR currently does,
but preserving the current mirred semantic).

Cheers,

Paolo