| Message ID | 20241014090720.189898-1-tariqt@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] macsec: Fix use-after-free while sending the offloading packet | expand |
2024-10-14, 12:07:20 +0300, Tariq Toukan wrote: > From: Jianbo Liu <jianbol@nvidia.com> > > KASAN reports the following UAF. The metadata_dst, which is used to > store the SCI value for macsec offload, is already freed by > metadata_dst_free() in macsec_free_netdev(), while driver still use it > for sending the packet. > > To fix this issue, dst_release() is used instead to release > metadata_dst. So it is not freed instantly in macsec_free_netdev() if > still referenced by skb. Ok. Then that packet is going to get dropped when it reaches the driver, right? At this point the TXSA we need shouldn't be configured anymore, so the driver/NIC won't be able to handle that skb. It would be bad if we just sent the packet unencrypted. > Fixes: 0a28bfd4971f ("net/macsec: Add MACsec skb_metadata_dst Tx Data path support") > Signed-off-by: Jianbo Liu <jianbol@nvidia.com> > Reviewed-by: Patrisious Haddad <phaddad@nvidia.com> > Reviewed-by: Chris Mi <cmi@nvidia.com> > Signed-off-by: Tariq Toukan <tariqt@nvidia.com> > --- > drivers/net/macsec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c > index 12d1b205f6d1..7076dedfa3be 100644 > --- a/drivers/net/macsec.c > +++ b/drivers/net/macsec.c > @@ -3817,7 +3817,7 @@ static void macsec_free_netdev(struct net_device *dev) > struct macsec_dev *macsec = macsec_priv(dev); > > if (macsec->secy.tx_sc.md_dst) nit: dst_release checks that dst is not NULL, so we don't need this test that I added in commit c52add61c27e ("macsec: don't free NULL metadata_dst") > - metadata_dst_free(macsec->secy.tx_sc.md_dst); > + dst_release(&macsec->secy.tx_sc.md_dst->dst); > free_percpu(macsec->stats); > free_percpu(macsec->secy.tx_sc.stats); > > -- > 2.44.0 >
On 10/15/2024 4:56 PM, Sabrina Dubroca wrote: > 2024-10-14, 12:07:20 +0300, Tariq Toukan wrote: >> From: Jianbo Liu <jianbol@nvidia.com> >> >> KASAN reports the following UAF. The metadata_dst, which is used to >> store the SCI value for macsec offload, is already freed by >> metadata_dst_free() in macsec_free_netdev(), while driver still use it >> for sending the packet. >> >> To fix this issue, dst_release() is used instead to release >> metadata_dst. So it is not freed instantly in macsec_free_netdev() if >> still referenced by skb. > > Ok. Then that packet is going to get dropped when it reaches the > driver, right? At this point the TXSA we need shouldn't be configured I think so because dst's output should be updated. But the problem here is dst free is delayed by RCU, which causes UAF. > anymore, so the driver/NIC won't be able to handle that skb. It would > be bad if we just sent the packet unencrypted. > > >> Fixes: 0a28bfd4971f ("net/macsec: Add MACsec skb_metadata_dst Tx Data path support") >> Signed-off-by: Jianbo Liu <jianbol@nvidia.com> >> Reviewed-by: Patrisious Haddad <phaddad@nvidia.com> >> Reviewed-by: Chris Mi <cmi@nvidia.com> >> Signed-off-by: Tariq Toukan <tariqt@nvidia.com> >> --- >> drivers/net/macsec.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c >> index 12d1b205f6d1..7076dedfa3be 100644 >> --- a/drivers/net/macsec.c >> +++ b/drivers/net/macsec.c >> @@ -3817,7 +3817,7 @@ static void macsec_free_netdev(struct net_device *dev) >> struct macsec_dev *macsec = macsec_priv(dev); >> >> if (macsec->secy.tx_sc.md_dst) > > nit: dst_release checks that dst is not NULL, so we don't need this > test that I added in commit c52add61c27e ("macsec: don't free NULL > metadata_dst") Good point. I will remove this test in the next version. Thanks! Jianbo > >> - metadata_dst_free(macsec->secy.tx_sc.md_dst); >> + dst_release(&macsec->secy.tx_sc.md_dst->dst); >> free_percpu(macsec->stats); >> free_percpu(macsec->secy.tx_sc.stats); >> >> -- >> 2.44.0 >> >
2024-10-15, 17:57:59 +0800, Jianbo Liu wrote: > > > On 10/15/2024 4:56 PM, Sabrina Dubroca wrote: > > 2024-10-14, 12:07:20 +0300, Tariq Toukan wrote: > > > From: Jianbo Liu <jianbol@nvidia.com> > > > > > > KASAN reports the following UAF. The metadata_dst, which is used to > > > store the SCI value for macsec offload, is already freed by > > > metadata_dst_free() in macsec_free_netdev(), while driver still use it > > > for sending the packet. > > > > > > To fix this issue, dst_release() is used instead to release > > > metadata_dst. So it is not freed instantly in macsec_free_netdev() if > > > still referenced by skb. > > > > Ok. Then that packet is going to get dropped when it reaches the > > driver, right? At this point the TXSA we need shouldn't be configured > > I think so because dst's output should be updated. What updates the dst when we're deleting the macsec device? And this is just a metadata_dst, it's only useful to hold the SCI. But I guess we would have the same issue when the macsec device still exists but the TXSA is gone, so hopefully this is handled well by all drivers. > But the problem here is > dst free is delayed by RCU, which causes UAF. To be clear, I'm not objecting to the patch, I'm wondering about other related issues once we fix that.
On 10/15/2024 6:28 PM, Sabrina Dubroca wrote: > 2024-10-15, 17:57:59 +0800, Jianbo Liu wrote: >> >> >> On 10/15/2024 4:56 PM, Sabrina Dubroca wrote: >>> 2024-10-14, 12:07:20 +0300, Tariq Toukan wrote: >>>> From: Jianbo Liu <jianbol@nvidia.com> >>>> >>>> KASAN reports the following UAF. The metadata_dst, which is used to >>>> store the SCI value for macsec offload, is already freed by >>>> metadata_dst_free() in macsec_free_netdev(), while driver still use it >>>> for sending the packet. >>>> >>>> To fix this issue, dst_release() is used instead to release >>>> metadata_dst. So it is not freed instantly in macsec_free_netdev() if >>>> still referenced by skb. >>> >>> Ok. Then that packet is going to get dropped when it reaches the >>> driver, right? At this point the TXSA we need shouldn't be configured >> >> I think so because dst's output should be updated. > > What updates the dst when we're deleting the macsec device? And this > is just a metadata_dst, it's only useful to hold the SCI. > You are right. It's not updated. > But I guess we would have the same issue when the macsec device still > exists but the TXSA is gone, so hopefully this is handled well by all > drivers. > And for now, I'd rather focus on fixing the kernel crash caused by UAF. > >> But the problem here is >> dst free is delayed by RCU, which causes UAF. > > To be clear, I'm not objecting to the patch, I'm wondering about other > related issues once we fix that. OK. I will send v2 later. Thanks! Jianbo >
2024-10-15, 21:46:26 +0800, Jianbo Liu wrote: > > > On 10/15/2024 6:28 PM, Sabrina Dubroca wrote: > > 2024-10-15, 17:57:59 +0800, Jianbo Liu wrote: > > > > > > > > > On 10/15/2024 4:56 PM, Sabrina Dubroca wrote: > > > > 2024-10-14, 12:07:20 +0300, Tariq Toukan wrote: > > > > > From: Jianbo Liu <jianbol@nvidia.com> > > > > > > > > > > KASAN reports the following UAF. The metadata_dst, which is used to > > > > > store the SCI value for macsec offload, is already freed by > > > > > metadata_dst_free() in macsec_free_netdev(), while driver still use it > > > > > for sending the packet. > > > > > > > > > > To fix this issue, dst_release() is used instead to release > > > > > metadata_dst. So it is not freed instantly in macsec_free_netdev() if > > > > > still referenced by skb. > > > > > > > > Ok. Then that packet is going to get dropped when it reaches the > > > > driver, right? At this point the TXSA we need shouldn't be configured > > > > > > I think so because dst's output should be updated. > > > > What updates the dst when we're deleting the macsec device? And this > > is just a metadata_dst, it's only useful to hold the SCI. > > > > You are right. It's not updated. > > > But I guess we would have the same issue when the macsec device still > > exists but the TXSA is gone, so hopefully this is handled well by all > > drivers. > > > > And for now, I'd rather focus on fixing the kernel crash caused by UAF. Ok, but please take a look at it very soon. Sending packets unencrypted when they should be encrypted can be just as bad as crashing the system.
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index 12d1b205f6d1..7076dedfa3be 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -3817,7 +3817,7 @@ static void macsec_free_netdev(struct net_device *dev) struct macsec_dev *macsec = macsec_priv(dev); if (macsec->secy.tx_sc.md_dst) - metadata_dst_free(macsec->secy.tx_sc.md_dst); + dst_release(&macsec->secy.tx_sc.md_dst->dst); free_percpu(macsec->stats); free_percpu(macsec->secy.tx_sc.stats);