| Message ID | 20240419011740.333714-3-rrameshbabu@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Resolve security issue in MACsec offload Rx datapath | expand |
2024-04-18, 18:17:16 -0700, Rahul Rameshbabu wrote: > diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c > index 0206b84284ab..679302ef1cd9 100644 > --- a/drivers/net/macsec.c > +++ b/drivers/net/macsec.c > @@ -991,6 +991,19 @@ static struct macsec_rx_sc *find_rx_sc_rtnl(struct macsec_secy *secy, sci_t sci) > return NULL; > } > > +static __u8 macsec_offload_pkt_type(const u8 *h_dest, const u8 *ndev_broadcast) > + nit: empty line shouldn't be here > +{ > + if (is_multicast_ether_addr_64bits(h_dest)) { > + if (ether_addr_equal_64bits(h_dest, ndev_broadcast)) > + return PACKET_BROADCAST; > + else > + return PACKET_MULTICAST; > + } > + > + return PACKET_HOST; > +} > + > static enum rx_handler_result handle_not_macsec(struct sk_buff *skb) > { > /* Deliver to the uncontrolled port by default */ > @@ -999,10 +1012,12 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb) > struct metadata_dst *md_dst; > struct macsec_rxh_data *rxd; > struct macsec_dev *macsec; > + bool is_macsec_md_dst; > > rcu_read_lock(); > rxd = macsec_data_rcu(skb->dev); > md_dst = skb_metadata_dst(skb); > + is_macsec_md_dst = md_dst && md_dst->type == METADATA_MACSEC; > > list_for_each_entry_rcu(macsec, &rxd->secys, secys) { > struct sk_buff *nskb; > @@ -1014,13 +1029,40 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb) > */ > if (macsec_is_offloaded(macsec) && netif_running(ndev)) { > struct macsec_rx_sc *rx_sc = NULL; Please move this into the "if (is_macsec_md_dst)" block below, since it's no longer used outside. > + const struct macsec_ops *ops; > > - if (md_dst && md_dst->type == METADATA_MACSEC) > - rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci); > + ops = macsec_get_ops(macsec, NULL); > > - if (md_dst && md_dst->type == METADATA_MACSEC && !rx_sc) > + if (ops->rx_uses_md_dst && !is_macsec_md_dst) > continue; > > + if (is_macsec_md_dst) { > + /* All drivers that implement MACsec offload > + * support using skb metadata destinations must > + * indicate that they do so. > + */ > + DEBUG_NET_WARN_ON_ONCE(!ops->rx_uses_md_dst); > + rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci); > + if (!rx_sc) > + continue; > + /* device indicated macsec offload occurred */ > + skb->dev = ndev; > + skb->pkt_type = macsec_offload_pkt_type( > + hdr->h_dest, ndev->broadcast); > + ret = RX_HANDLER_ANOTHER; > + goto out; > + } > + > + /* 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. > if (ether_addr_equal_64bits(hdr->h_dest, > ndev->dev_addr)) { > /* exact match, divert skb to this port */
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. >> if (ether_addr_equal_64bits(hdr->h_dest, >> ndev->dev_addr)) { >> /* exact match, divert skb to this port */ -- Thanks, Rahul Rameshbabu
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 :/
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
2024-04-22, 22:55:02 -0700, Rahul Rameshbabu wrote: > 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? Forcing a fallback to SW could be considered a breakage because of the performance regression, so I don't think we can turn this on by default. Then I would simply reject offload on those devices. We could have a compat mode that does the SW fallback you suggest. I don't know if it would be used. > > (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. I thought about doing exactly that, and then dropped the idea because it would only help with newer kernels.
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index 0206b84284ab..679302ef1cd9 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -991,6 +991,19 @@ static struct macsec_rx_sc *find_rx_sc_rtnl(struct macsec_secy *secy, sci_t sci) return NULL; } +static __u8 macsec_offload_pkt_type(const u8 *h_dest, const u8 *ndev_broadcast) + +{ + if (is_multicast_ether_addr_64bits(h_dest)) { + if (ether_addr_equal_64bits(h_dest, ndev_broadcast)) + return PACKET_BROADCAST; + else + return PACKET_MULTICAST; + } + + return PACKET_HOST; +} + static enum rx_handler_result handle_not_macsec(struct sk_buff *skb) { /* Deliver to the uncontrolled port by default */ @@ -999,10 +1012,12 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb) struct metadata_dst *md_dst; struct macsec_rxh_data *rxd; struct macsec_dev *macsec; + bool is_macsec_md_dst; rcu_read_lock(); rxd = macsec_data_rcu(skb->dev); md_dst = skb_metadata_dst(skb); + is_macsec_md_dst = md_dst && md_dst->type == METADATA_MACSEC; list_for_each_entry_rcu(macsec, &rxd->secys, secys) { struct sk_buff *nskb; @@ -1014,13 +1029,40 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb) */ if (macsec_is_offloaded(macsec) && netif_running(ndev)) { struct macsec_rx_sc *rx_sc = NULL; + const struct macsec_ops *ops; - if (md_dst && md_dst->type == METADATA_MACSEC) - rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci); + ops = macsec_get_ops(macsec, NULL); - if (md_dst && md_dst->type == METADATA_MACSEC && !rx_sc) + if (ops->rx_uses_md_dst && !is_macsec_md_dst) continue; + if (is_macsec_md_dst) { + /* All drivers that implement MACsec offload + * support using skb metadata destinations must + * indicate that they do so. + */ + DEBUG_NET_WARN_ON_ONCE(!ops->rx_uses_md_dst); + rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci); + if (!rx_sc) + continue; + /* device indicated macsec offload occurred */ + skb->dev = ndev; + skb->pkt_type = macsec_offload_pkt_type( + hdr->h_dest, ndev->broadcast); + ret = RX_HANDLER_ANOTHER; + goto out; + } + + /* 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 (ether_addr_equal_64bits(hdr->h_dest, ndev->dev_addr)) { /* exact match, divert skb to this port */ @@ -1036,14 +1078,11 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb) break; nskb->dev = ndev; - if (ether_addr_equal_64bits(hdr->h_dest, - ndev->broadcast)) - nskb->pkt_type = PACKET_BROADCAST; - else - nskb->pkt_type = PACKET_MULTICAST; + nskb->pkt_type = macsec_offload_pkt_type( + hdr->h_dest, ndev->broadcast); __netif_rx(nskb); - } else if (rx_sc || ndev->flags & IFF_PROMISC) { + } else if (ndev->flags & IFF_PROMISC) { skb->dev = ndev; skb->pkt_type = PACKET_HOST; ret = RX_HANDLER_ANOTHER;