Re: [PATCH net-next 2/3] macsec: Detect if Rx skb is macsec-related for offloading devices that update md_dst

[Sabrina Dubroca <sd@queasysnail.net>]

2024-04-19, 11:01:20 -0700, Rahul Rameshbabu wrote:
> On Fri, 19 Apr, 2024 17:05:52 +0200 Sabrina Dubroca <sd@queasysnail.net> wrote:
> > 2024-04-18, 18:17:16 -0700, Rahul Rameshbabu wrote:
> <snip>
> >> +			/* This datapath is insecure because it is unable to
> >> +			 * enforce isolation of broadcast/multicast traffic and
> >> +			 * unicast traffic with promiscuous mode on the macsec
> >> +			 * netdev. Since the core stack has no mechanism to
> >> +			 * check that the hardware did indeed receive MACsec
> >> +			 * traffic, it is possible that the response handling
> >> +			 * done by the MACsec port was to a plaintext packet.
> >> +			 * This violates the MACsec protocol standard.
> >> +			 */
> >> +			DEBUG_NET_WARN_ON_ONCE(true);
> >
> > If you insist on this warning (and I'm not convinced it's useful,
> > since if the HW is already built and cannot inform the driver, there's
> > nothing the driver implementer can do), I would move it somewhere into
> > the config path. macsec_update_offload would be a better location for
> > this kind of warning (maybe with a pr_warn (not limited to debug
> > configs) saying something like "MACsec offload on devices that don't
> > support md_dst are insecure: they do not provide proper isolation of
> > traffic"). The comment can stay here.
> >
> 
> I do not like the warning either. I left it mainly if it needed further
> discussion on the mailing list. Will remove it in my next revision. That
> said, it may make sense to advertise rx_uses_md_dst over netlink to
> annotate what macsec offload path a device uses? Just throwing out an
> idea here.

Maybe. I was also thinking about adding a way to restrict offloading
only to devices with rx_uses_md_dst.

(Slightly related) I also find it annoying that users have to tell the
kernel whether to use PHY or MAC offload, but have no way to know
which one their HW supports. That should probably have been an
implementation detail that didn't need to be part of uapi :/

-- 
Sabrina