Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 3 Dec 2019 14:01:14 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
To:     Laura Abbott <labbott@redhat.com>
Cc:     Pablo Neira Ayuso <pablo@netfilter.org>,
        Jozsef Kadlecsik <kadlec@netfilter.org>,
        Florian Westphal <fw@strlen.de>,
        "David S. Miller" <davem@davemloft.net>,
        netfilter-devel@vger.kernel.org, coreteam@netfilter.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH] netfilter: nf_flow_table_offload: Correct memcpy size
 for flow_overload_mangle
Message-ID: <20191203170114.GB377782@localhost.localdomain>
References: <20191203160345.24743-1-labbott@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191203160345.24743-1-labbott@redhat.com>
User-Agent: Mutt/1.12.1 (2019-06-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Dec 03, 2019 at 11:03:45AM -0500, Laura Abbott wrote:
> The sizes for memcpy in flow_offload_mangle don't match
> the source variables, leading to overflow errors on some
> build configurations:
> 
> In function 'memcpy',
>     inlined from 'flow_offload_mangle' at net/netfilter/nf_flow_table_offload.c:112:2,
>     inlined from 'flow_offload_port_dnat' at net/netfilter/nf_flow_table_offload.c:373:2,
>     inlined from 'nf_flow_rule_route_ipv4' at net/netfilter/nf_flow_table_offload.c:424:3:
> ./include/linux/string.h:376:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
>   376 |    __read_overflow2();
>       |    ^~~~~~~~~~~~~~~~~~
> make[2]: *** [scripts/Makefile.build:266: net/netfilter/nf_flow_table_offload.o] Error 1
> 
> Fix this by using the corresponding type.
> 
> Fixes: c29f74e0df7a ("netfilter: nf_flow_table: hardware offload support")
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> Seen on a Fedora powerpc little endian build with -O3 but it looks like
> it is correctly catching an error with doing a memcpy outside the source
> variable.

Hi,

It is right but the fix is not. In that call trace:

flow_offload_port_dnat() {
...
        u32 mask = ~htonl(0xffff);
        __be16 port;
...
        flow_offload_mangle(entry, flow_offload_l4proto(flow), offset,
	                            (u8 *)&port, (u8 *)&mask);
}

port should have a 32b storage as well, and aligned with the mask.

> ---
>  net/netfilter/nf_flow_table_offload.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> index c54c9a6cc981..526f894d0bdb 100644
> --- a/net/netfilter/nf_flow_table_offload.c
> +++ b/net/netfilter/nf_flow_table_offload.c
> @@ -108,8 +108,8 @@ static void flow_offload_mangle(struct flow_action_entry *entry,
>  	entry->id = FLOW_ACTION_MANGLE;
>  	entry->mangle.htype = htype;
>  	entry->mangle.offset = offset;
> -	memcpy(&entry->mangle.mask, mask, sizeof(u32));
> -	memcpy(&entry->mangle.val, value, sizeof(u32));
                                   ^^^^^         ^^^ which is &port in the call above
> +	memcpy(&entry->mangle.mask, mask, sizeof(u8));
> +	memcpy(&entry->mangle.val, value, sizeof(u8));

This fix would cause it to copy only the first byte, which is not the
intention.

>  }
>  
>  static inline struct flow_action_entry *
> -- 
> 2.21.0
>