[OpenWrt-Devel,RFC] kernel: disable EAP local hack when using group_fwd_mask

By default bridges will not forward frame with destination 01-80-C2-00-00-03
(ie wired EAP frames). You can allow forward using:
echo 8 > /sys/class/net/brX/bridge/group_fwd_mask

EAP frames over wireless are using the AP MAC address as destination,
and 640-bridge-only-accept-EAP-locally.patch hack is there to prevent
bridges from forwarding these EAP frames

Disable this hack when the administrator allow 01-80-C2-00-00-03 forward,
so that all EAP frames are allowed to be forwarded

Signed-off-by: Etienne Champetier <champetier.etienne@gmail.com>
---
 .../generic/hack-4.14/640-bridge-only-accept-EAP-locally.patch  | 2 +-
 .../generic/hack-4.19/640-bridge-only-accept-EAP-locally.patch  | 2 +-
 .../generic/hack-4.9/640-bridge-only-accept-EAP-locally.patch   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Etienne Champetier <champetier.etienne@gmail.com> [2019-07-26 19:23:02]:

Hi,

I've noticed your request for feedback on IRC.

> and 640-bridge-only-accept-EAP-locally.patch hack is there to prevent
> bridges from forwarding these EAP frames

it would be nice to know if this patch is still needed so we could possibly
remove[1] it from 4.19 kernel.

> -+	if (skb->protocol == htons(ETH_P_PAE))
> ++	if (skb->protocol == htons(ETH_P_PAE) && !(br->group_fwd_mask & (1u << 3)))
>  +		return br_pass_frame_up(skb);

This usage of magic numbers is usually a warn sign to me, so I went ahead and
read the surrounding code and it seems to me, that you probably wanted
something like this instead:

  u16 fwd_mask = p->br->group_fwd_mask_required;
  fwd_mask |= p->br->group_fwd_mask;
  const unsigned char *dest = eth_hdr(skb)->h_dest;

  if (skb->protocol == htons(ETH_P_PAE) && !(fwd_mask & (1u << dest[5])))
          return br_pass_frame_up(skb);

1. https://patchwork.ozlabs.org/patch/884518/

-- ynezz

Hi Petr,

Le jeu. 1 août 2019 à 01:51, Petr Štetiar <ynezz@true.cz> a écrit :
>
> Etienne Champetier <champetier.etienne@gmail.com> [2019-07-26 19:23:02]:
>
> Hi,
>
> I've noticed your request for feedback on IRC.
Thanks for taking some time

>
> > and 640-bridge-only-accept-EAP-locally.patch hack is there to prevent
> > bridges from forwarding these EAP frames
>
> it would be nice to know if this patch is still needed so we could possibly
> remove[1] it from 4.19 kernel.

I revived this old email thread last week to try to get more
information from Felix
(http://lists.infradead.org/pipermail/openwrt-devel/2019-July/018127.html)
If the fix was to workaround a client bug we might want to keep it,
else maybe we could rework it and add a warn_once if this hack prevent
a EAP frame from being forwarded

>
> > -+    if (skb->protocol == htons(ETH_P_PAE))
> > ++    if (skb->protocol == htons(ETH_P_PAE) && !(br->group_fwd_mask & (1u << 3)))
> >  +            return br_pass_frame_up(skb);
>
> This usage of magic numbers is usually a warn sign to me, so I went ahead and
> read the surrounding code and it seems to me, that you probably wanted
> something like this instead:
>
>   u16 fwd_mask = p->br->group_fwd_mask_required;
>   fwd_mask |= p->br->group_fwd_mask;
>   const unsigned char *dest = eth_hdr(skb)->h_dest;
>
>   if (skb->protocol == htons(ETH_P_PAE) && !(fwd_mask & (1u << dest[5])))
>           return br_pass_frame_up(skb);

Actually no, let me try to explain it again
By default a bridge forward everything except 01-80-C2-00-00-XX, which
includes 01-80-C2-00-00-03, which is the multicast mac for EAP frames.
For wifi, EAP are not using multicast but unicast (it's what I
understood reading various email thread, I haven't double checked
that).

What I think an admin wants when he do
echo 8 > /sys/class/net/brX/bridge/group_fwd_mask
is not to allow 01-80-C2-00-00-03 forwarding, but allow all EAP forwarding.
Your patch would allow multicast EAP forwarding but not unicast EAP
forwarding, except if dest mac ends with 03,
which would be very surprising.

Another approach would be to add a setting like
'disable-eap-local-hack' to enable/disable this hack, what do you
prefer ?

Etienne


>
> 1. https://patchwork.ozlabs.org/patch/884518/
>
> -- ynezz

Etienne Champetier <champetier.etienne@gmail.com> [2019-08-01 07:23:18]:

> If the fix was to workaround a client bug we might want to keep it,

Actually we don't know if it wasnt fixed already upstream as Rafał suggested
in his old removal patch.

> else maybe we could rework it and add a warn_once if this hack prevent
> a EAP frame from being forwarded

This warn_once would have probably saved some time already.

> 'disable-eap-local-hack' to enable/disable this hack, what do you
> prefer ?

I would prefer to remove this patch for 4.19 kernel to find out if it's still
needed. And if it's needed, then it would make sense to discuss this use case
with upstream and make it official feature.

Let's wait for Felix's input as well.

-- ynezz

Hi Petr,

Le jeu. 1 août 2019 à 11:39, Petr Štetiar <ynezz@true.cz> a écrit :
>
> Etienne Champetier <champetier.etienne@gmail.com> [2019-08-01 07:23:18]:
>
> > If the fix was to workaround a client bug we might want to keep it,
>
> Actually we don't know if it wasnt fixed already upstream as Rafał suggested
> in his old removal patch.
>
> > else maybe we could rework it and add a warn_once if this hack prevent
> > a EAP frame from being forwarded
>
> This warn_once would have probably saved some time already.
>
> > 'disable-eap-local-hack' to enable/disable this hack, what do you
> > prefer ?
>
> I would prefer to remove this patch for 4.19 kernel to find out if it's still
> needed.

you mean all versions in master (4.19/4.14/4.9) or really only 4.19 ?

> And if it's needed, then it would make sense to discuss this use case
> with upstream and make it official feature.

I have a small tool/OpenWrt package
(https://github.com/openwrt/packages/blob/master/net/phantap/Makefile)
that I'm presenting at DEF CON in a week, and would really love to
have a way to disable this hack,
so I don't have to tell people that want to test it that they have to
rebuild OpenWrt.
So I'm willing to work on any small patch that workaround my issue and
seems reasonable to everyone in a short time-frame ;)

>
> Let's wait for Felix's input as well.

I'll also try to dig some more in the old repo & bug tracker to see if
there is more info than in the patch & email

Etienne

>
> -- ynezz

Hi Petr,

Le jeu. 1 août 2019 à 12:51, Etienne Champetier
<champetier.etienne@gmail.com> a écrit :
>
> Hi Petr,
>
> Le jeu. 1 août 2019 à 11:39, Petr Štetiar <ynezz@true.cz> a écrit :
> >
> > Etienne Champetier <champetier.etienne@gmail.com> [2019-08-01 07:23:18]:
> >
> > > If the fix was to workaround a client bug we might want to keep it,
> >
> > Actually we don't know if it wasnt fixed already upstream as Rafał suggested
> > in his old removal patch.
> >
> > > else maybe we could rework it and add a warn_once if this hack prevent
> > > a EAP frame from being forwarded
> >
> > This warn_once would have probably saved some time already.
> >
> > > 'disable-eap-local-hack' to enable/disable this hack, what do you
> > > prefer ?
> >
> > I would prefer to remove this patch for 4.19 kernel to find out if it's still
> > needed.
>
> you mean all versions in master (4.19/4.14/4.9) or really only 4.19 ?
>
> > And if it's needed, then it would make sense to discuss this use case
> > with upstream and make it official feature.
>
> I have a small tool/OpenWrt package
> (https://github.com/openwrt/packages/blob/master/net/phantap/Makefile)
> that I'm presenting at DEF CON in a week, and would really love to
> have a way to disable this hack,
> so I don't have to tell people that want to test it that they have to
> rebuild OpenWrt.
> So I'm willing to work on any small patch that workaround my issue and
> seems reasonable to everyone in a short time-frame ;)
>
> >
> > Let's wait for Felix's input as well.

Last email from Felix that I see is from june 12, so I sent an RFCv2
(kernel: add disable_eap_hack sysfs attribute) for consideration

>
> I'll also try to dig some more in the old repo & bug tracker to see if
> there is more info than in the patch & email
>
> Etienne
>
> >
> > -- ynezz