Re: [PATCH] netfilter: nf_flow_table_offload: Correct memcpy size for flow_overload_mangle

[Pablo Neira Ayuso <pablo@netfilter.org>]

[-- Attachment #1: Type: text/plain, Size: 3380 bytes --]

On Fri, Dec 06, 2019 at 04:58:30PM -0600, Justin Forbes wrote:
> On Tue, Dec 3, 2019 at 2:50 PM Laura Abbott <labbott@redhat.com> wrote:
> >
> > 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@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.
> > >
> >
> > 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.
> 
> Any update on this? It is definitely a problem on PPC LE.

I'm attaching a tentative patch to address this problem.

Thanks.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 2089 bytes --]

diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index 30205d57226d..cfa5602f54f5 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -336,23 +336,22 @@ static void flow_offload_port_snat(struct net *net,
 				   struct nf_flow_rule *flow_rule)
 {
 	struct flow_action_entry *entry = flow_action_entry_next(flow_rule);
-	u32 mask = ~htonl(0xffff0000);
-	__be16 port;
+	u32 mask = ~htonl(0xffff0000), port;
 	u32 offset;
 
 	switch (dir) {
 	case FLOW_OFFLOAD_DIR_ORIGINAL:
-		port = flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst_port;
+		port = ntohs(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst_port);
 		offset = 0; /* offsetof(struct tcphdr, source); */
 		break;
 	case FLOW_OFFLOAD_DIR_REPLY:
-		port = flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.src_port;
+		port = ntohs(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.src_port);
 		offset = 0; /* offsetof(struct tcphdr, dest); */
 		break;
 	default:
 		return;
 	}
-
+	port = htonl(port << 16);
 	flow_offload_mangle(entry, flow_offload_l4proto(flow), offset,
 			    (u8 *)&port, (u8 *)&mask);
 }
@@ -363,23 +362,22 @@ static void flow_offload_port_dnat(struct net *net,
 				   struct nf_flow_rule *flow_rule)
 {
 	struct flow_action_entry *entry = flow_action_entry_next(flow_rule);
-	u32 mask = ~htonl(0xffff);
-	__be16 port;
+	u32 mask = ~htonl(0xffff), port;
 	u32 offset;
 
 	switch (dir) {
 	case FLOW_OFFLOAD_DIR_ORIGINAL:
-		port = flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst_port;
+		port = ntohs(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst_port);
 		offset = 0; /* offsetof(struct tcphdr, source); */
 		break;
 	case FLOW_OFFLOAD_DIR_REPLY:
-		port = flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.src_port;
+		port = ntohs(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.src_port);
 		offset = 0; /* offsetof(struct tcphdr, dest); */
 		break;
 	default:
 		return;
 	}
-
+	port = htonl(port);
 	flow_offload_mangle(entry, flow_offload_l4proto(flow), offset,
 			    (u8 *)&port, (u8 *)&mask);
 }