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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
---
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






[Index of Archives]     [Netfitler Users]     [Berkeley Packet Filter]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux