Message ID | 20191203160345.24743-1-labbott@redhat.com |
---|---|
State | Awaiting Upstream |
Delegated to: | David Miller |
Headers | show |
Series | netfilter: nf_flow_table_offload: Correct memcpy size for flow_overload_mangle | expand |
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 >
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. Thanks, Laura >> } >> >> static inline struct flow_action_entry * >> -- >> 2.21.0 >> >
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. Thanks, Justin
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.
On Sat, Dec 7, 2019 at 11:38 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > 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. > This patch does in fact fix the build issue. Thanks, Justin
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)); + memcpy(&entry->mangle.mask, mask, sizeof(u8)); + memcpy(&entry->mangle.val, value, sizeof(u8)); } static inline struct flow_action_entry *
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. --- net/netfilter/nf_flow_table_offload.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)