[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9bc4b04b-a3cc-4e58-4c73-1d77b7ed05da@redhat.com>
Date: Tue, 3 Dec 2019 15:49:54 -0500
From: Laura Abbott <labbott@...hat.com>
To: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc: Pablo Neira Ayuso <pablo@...filter.org>,
Jozsef Kadlecsik <kadlec@...filter.org>,
Florian Westphal <fw@...len.de>,
"David S. Miller" <davem@...emloft.net>,
netfilter-devel@...r.kernel.org, coreteam@...filter.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH] netfilter: nf_flow_table_offload: Correct memcpy size for
flow_overload_mangle
On 12/3/19 12:01 PM, Marcelo Ricardo Leitner wrote:
> 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@...hat.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.
>
Thanks for the review. I took another look at fixing this and I
think it might be better for the maintainer or someone who is more
familiar with the code to fix this. I ended up down a rabbit
hole trying to get the types to work and I wasn't confident about
the casting.
Thanks,
Laura
>> }
>>
>> static inline struct flow_action_entry *
>> --
>> 2.21.0
>>
>
Powered by blists - more mailing lists