On Mon, 22 Apr, 2024 11:23:05 +0200 Sabrina Dubroca sd@queasysnail.net wrote:
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.
That's an option. Basically, devices that do not support rx_uses_md_dst really only just do SW MACsec but do not return an error if the offload parameter is passed over netlink so user scripts do not break?
(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 :/
We could leave the phy / mac netlink keywords and introduce an "on" option. We deduce whether the device is a phydev or not when on is passed and set the macsec->offload flag based on that. The phy and mac options for offload in ip-macsec can then be deprecated.
-- Thanks,
Rahul Rameshbabu