[PATCH net] macsec: Fix use-after-free while sending the offloading packet

[PATCH net] macsec: Fix use-after-free while sending the offloading packet

[Tariq Toukan]

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.

 BUG: KASAN: slab-use-after-free in mlx5e_xmit+0x1e8f/0x4190 [mlx5_core]
 Read of size 2 at addr ffff88813e42e038 by task kworker/7:2/714
 [...]
 Workqueue: mld mld_ifc_work
 Call Trace:
  <TASK>
  dump_stack_lvl+0x51/0x60
  print_report+0xc1/0x600
  kasan_report+0xab/0xe0
  mlx5e_xmit+0x1e8f/0x4190 [mlx5_core]
  dev_hard_start_xmit+0x120/0x530
  sch_direct_xmit+0x149/0x11e0
  __qdisc_run+0x3ad/0x1730
  __dev_queue_xmit+0x1196/0x2ed0
  vlan_dev_hard_start_xmit+0x32e/0x510 [8021q]
  dev_hard_start_xmit+0x120/0x530
  __dev_queue_xmit+0x14a7/0x2ed0
  macsec_start_xmit+0x13e9/0x2340
  dev_hard_start_xmit+0x120/0x530
  __dev_queue_xmit+0x14a7/0x2ed0
  ip6_finish_output2+0x923/0x1a70
  ip6_finish_output+0x2d7/0x970
  ip6_output+0x1ce/0x3a0
  NF_HOOK.constprop.0+0x15f/0x190
  mld_sendpack+0x59a/0xbd0
  mld_ifc_work+0x48a/0xa80
  process_one_work+0x5aa/0xe50
  worker_thread+0x79c/0x1290
  kthread+0x28f/0x350
  ret_from_fork+0x2d/0x70
  ret_from_fork_asm+0x11/0x20
  </TASK>

 Allocated by task 3922:
  kasan_save_stack+0x20/0x40
  kasan_save_track+0x10/0x30
  __kasan_kmalloc+0x77/0x90
  __kmalloc_noprof+0x188/0x400
  metadata_dst_alloc+0x1f/0x4e0
  macsec_newlink+0x914/0x1410
  __rtnl_newlink+0xe08/0x15b0
  rtnl_newlink+0x5f/0x90
  rtnetlink_rcv_msg+0x667/0xa80
  netlink_rcv_skb+0x12c/0x360
  netlink_unicast+0x551/0x770
  netlink_sendmsg+0x72d/0xbd0
  __sock_sendmsg+0xc5/0x190
  ____sys_sendmsg+0x52e/0x6a0
  ___sys_sendmsg+0xeb/0x170
  __sys_sendmsg+0xb5/0x140
  do_syscall_64+0x4c/0x100
  entry_SYSCALL_64_after_hwframe+0x4b/0x53

 Freed by task 4011:
  kasan_save_stack+0x20/0x40
  kasan_save_track+0x10/0x30
  kasan_save_free_info+0x37/0x50
  poison_slab_object+0x10c/0x190
  __kasan_slab_free+0x11/0x30
  kfree+0xe0/0x290
  macsec_free_netdev+0x3f/0x140
  netdev_run_todo+0x450/0xc70
  rtnetlink_rcv_msg+0x66f/0xa80
  netlink_rcv_skb+0x12c/0x360
  netlink_unicast+0x551/0x770
  netlink_sendmsg+0x72d/0xbd0
  __sock_sendmsg+0xc5/0x190
  ____sys_sendmsg+0x52e/0x6a0
  ___sys_sendmsg+0xeb/0x170
  __sys_sendmsg+0xb5/0x140
  do_syscall_64+0x4c/0x100
  entry_SYSCALL_64_after_hwframe+0x4b/0x53

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)
-		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

Re: [PATCH net] macsec: Fix use-after-free while sending the offloading packet

[Sabrina Dubroca]

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
> 

-- 
Sabrina

Re: [PATCH net] macsec: Fix use-after-free while sending the offloading packet

[Jianbo Liu]

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
>>
> 

Re: [PATCH net] macsec: Fix use-after-free while sending the offloading packet

[Sabrina Dubroca]

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.

-- 
Sabrina

Re: [PATCH net] macsec: Fix use-after-free while sending the offloading packet

[Jianbo Liu]

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

>

Re: [PATCH net] macsec: Fix use-after-free while sending the offloading packet

[Sabrina Dubroca]

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.

-- 
Sabrina