netdev - Re: [PATCH] bridge: use promisc arg instead of skb flags

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK_HC7bOe2KhVnDiG4Z3tpkodiCkewEct7r2gXanjGBC8WwFsQ@mail.gmail.com>
Date: Sun, 6 Oct 2024 10:24:34 -0700
From: Amedeo Baragiola <ingamedeo@...il.com>
To: Nikolay Aleksandrov <razor@...ckwall.org>
Cc: Roopa Prabhu <roopa@...dia.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	bridge@...ts.linux.dev, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Pablo Neira Ayuso <pablo@...filter.org>
Subject: Re: [PATCH] bridge: use promisc arg instead of skb flags

I agree, just patch actually changes the behaviour when a BR_FDB_LOCAL
dst is found and drops the traffic because promisc is *always* set to
false when a BR_FDB_LOCAL dst is found in br_handle_frame_finish().
I guess the problem I was trying to solve was that since the
introduction of the promisc flag we still use brdev->flags &
IFF_PROMISC in br_pass_frame_up() which is essentially the value of
promisc (except in the BR_FDB_LOCAL case above) instead of promisc
itself.

Amedeo


On Sat, Oct 5, 2024 at 7:06 AM Nikolay Aleksandrov <razor@...ckwall.org> wrote:
>
> On 05/10/2024 04:44, Amedeo Baragiola wrote:
> > Since commit 751de2012eaf ("netfilter: br_netfilter: skip conntrack input hook for promisc packets")
> > a second argument (promisc) has been added to br_pass_frame_up which
> > represents whether the interface is in promiscuous mode. However,
> > internally - in one remaining case - br_pass_frame_up checks the device
> > flags derived from skb instead of the argument being passed in.
> > This one-line changes addresses this inconsistency.
> >
> > Signed-off-by: Amedeo Baragiola <ingamedeo@...il.com>
> > ---
> >  net/bridge/br_input.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> > index ceaa5a89b947..156c18f42fa3 100644
> > --- a/net/bridge/br_input.c
> > +++ b/net/bridge/br_input.c
> > @@ -50,8 +50,7 @@ static int br_pass_frame_up(struct sk_buff *skb, bool promisc)
> >        * packet is allowed except in promisc mode when someone
> >        * may be running packet capture.
> >        */
> > -     if (!(brdev->flags & IFF_PROMISC) &&
> > -         !br_allowed_egress(vg, skb)) {
> > +     if (!promisc && !br_allowed_egress(vg, skb)) {
> >               kfree_skb(skb);
> >               return NET_RX_DROP;
> >       }
>
> This is subtle, but it does change behaviour when a BR_FDB_LOCAL dst
> is found it will always drop the traffic after this patch (w/ promisc) if it
> doesn't pass br_allowed_egress(). It would've been allowed before, but current
> situation does make the patch promisc bit inconsistent, i.e. we get
> there because of BR_FDB_LOCAL regardless of the promisc flag.
>
> Because we can have a BR_FDB_LOCAL dst and still pass up such skb because of
> the flag instead of local_rcv (see br_br_handle_frame_finish()).
>
> CCing also Pablo for a second pair of eyes and as the original patch
> author. :)
>
> Pablo WDYT?
>
> Just FYI we definitely want to see all traffic if promisc is set, so
> this patch is a no-go.
>
> Cheers,
>  Nik



-- 
Thanks,
Amedeo