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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <89ccd2ac-5cb8-46e1-86c0-efc741ff18c9@nvidia.com>
Date: Tue, 15 Oct 2024 17:57:59 +0800
From: Jianbo Liu <jianbol@...dia.com>
To: Sabrina Dubroca <sd@...asysnail.net>, Tariq Toukan <tariqt@...dia.com>
CC: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	<netdev@...r.kernel.org>, Saeed Mahameed <saeedm@...dia.com>, Gal Pressman
	<gal@...dia.com>, Leon Romanovsky <leonro@...dia.com>, Patrisious Haddad
	<phaddad@...dia.com>, Chris Mi <cmi@...dia.com>
Subject: Re: [PATCH net] macsec: Fix use-after-free while sending the
 offloading packet



On 10/15/2024 4:56 PM, Sabrina Dubroca wrote:
> 2024-10-14, 12:07:20 +0300, Tariq Toukan wrote:
>> From: Jianbo Liu <jianbol@...dia.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@...dia.com>
>> Reviewed-by: Patrisious Haddad <phaddad@...dia.com>
>> Reviewed-by: Chris Mi <cmi@...dia.com>
>> Signed-off-by: Tariq Toukan <tariqt@...dia.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
>>
>