From: Justin Forbes <jmforbes@linuxtx.org>
To: Laura Abbott <labbott@redhat.com>
Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>,
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, LKML <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
Date: Fri, 6 Dec 2019 16:58:30 -0600 [thread overview]
Message-ID: <CAFxkdAraVz6mbQ3OFRGF3DmfWMDNzuXd+HJ14ypex6bMm-oCGw@mail.gmail.com> (raw)
In-Reply-To: <9bc4b04b-a3cc-4e58-4c73-1d77b7ed05da@redhat.com>
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
next prev parent reply other threads:[~2019-12-06 22:58 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-03 16:03 [PATCH] netfilter: nf_flow_table_offload: Correct memcpy size for flow_overload_mangle Laura Abbott
2019-12-03 17:01 ` Marcelo Ricardo Leitner
2019-12-03 20:49 ` Laura Abbott
2019-12-06 22:58 ` Justin Forbes [this message]
2019-12-07 17:38 ` Pablo Neira Ayuso
2019-12-09 17:56 ` Justin Forbes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFxkdAraVz6mbQ3OFRGF3DmfWMDNzuXd+HJ14ypex6bMm-oCGw@mail.gmail.com \
--to=jmforbes@linuxtx.org \
--cc=coreteam@netfilter.org \
--cc=davem@davemloft.net \
--cc=fw@strlen.de \
--cc=kadlec@netfilter.org \
--cc=keescook@chromium.org \
--cc=labbott@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=marcelo.leitner@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=netfilter-devel@vger.kernel.org \
--cc=pablo@netfilter.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).