Received-SPF: pass (google.com: domain of linux-kernel+bounces-4395-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <ada94c85-7898-405f-8aa5-54da801109dc@intel.com>
Date: Mon, 18 Dec 2023 13:08:16 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [net PATCH v2] i40e: fix use-after-free in i40e_aqc_add_filters()
Content-Language: en-US
To: Ke Xiao <xiaoke@sangfor.com.cn>, <jesse.brandeburg@intel.com>,
	<anthony.l.nguyen@intel.com>, <davem@davemloft.net>, <edumazet@google.com>,
	<kuba@kernel.org>, <pabeni@redhat.com>
CC: <dinghui@sangfor.com.cn>, <zhudi2@huawei.com>, <zhangrui182@huawei.com>,
	<shannon.nelson@amd.com>, <jan.sokolowski@intel.com>, <horms@kernel.org>,
	<intel-wired-lan@lists.osuosl.org>, <netdev@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
References: <20231218070850.15870-1-xiaoke@sangfor.com.cn>
From: Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20231218070850.15870-1-xiaoke@sangfor.com.cn>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SnAyS1poNzNQYkE0d1l0ZlVvelhtUVlxQU1KZHNQNngra09lSk5DY1ZtaE93?=
 =?utf-8?B?Ym42T2kvdi93UmxXUmNMaHNOdEFZcHNYYVN6Zk15ZFVLSEtEclhnNnlwOWI4?=
 =?utf-8?B?NkwvNy91UW92d0tkT0dKTzNtRFdUOHFWcndjaVVRRFNDU3hvSXIyVjBhWmRD?=
 =?utf-8?B?cW9OekU4a3ErcUdkQU9wNFdra3NzWjFiam8vaDBkcVQ2OS9TNFZKTGFVSWlR?=
 =?utf-8?B?WTdoc0s4TThpZ2hBK3J0S3FhK2dKcllHa0dOQis1OHg0MCtYVVNjUHB2RHc5?=
 =?utf-8?B?WDg1VTV4YjMwZnVYMmRzT1Z2WkErMVZ5V3lpbHpFWjhtN1hkV243N0dac2lm?=
 =?utf-8?B?aFRGTXFhRHVYUGQ4WWJJRWF1MlBmTTlYMjFIeGs4Wk1kRFZaWTQ4TDdGcVJJ?=
 =?utf-8?B?UUZkM1hPUDZzSjhPZS9HbS9lWFZjVGN3aTk4bXZjMXZGcm5aRVpKaE9oMlVx?=
 =?utf-8?B?NCtjc1lPckVqKzZoL2k1SmZPQjZ1Rmh4SlZEaDFzendTUFRZbnVnWmIxTmd4?=
 =?utf-8?B?MmJqUHk2UjNGV3pqa2NhUHUrUXBNOFNMTFRRZDlzVUI1dnZNKzUyUzFBT1U3?=
 =?utf-8?B?WWpjZTNHY0k4dEt0MlVjNXlzQ29oaEFKcWkrKzZlRGQyS290WUc3QnJkdEY0?=
 =?utf-8?B?eXlwM2treEx1Ym5ONXErdmNwNzRVSkxFRnd0Qjk4RjQ2SEUxMWFiK2J5NFZ2?=
 =?utf-8?B?MHduOWlaZWx0cW9nemJCaUFzSEVJRE5TQ1JOaHVXUVRZVDNqYVhWNFBNRXd1?=
 =?utf-8?B?S25VZ3hYR0JNbXRGZlp1Ym1lTXhOSXplV2QvT0tsaTIxa1BrOFFMSlJFWEVq?=
 =?utf-8?B?NDZzbzF0L0VHSm41bVM0M0JialJwYnFwZDlSa0JTMDJnU0pnR3pPZ0MzZ1F3?=
 =?utf-8?B?bjA2TEpBMjdxQ0pQb0NXNkFJQjg3cktzTTBCd0pYQXN3UWo0OFlNL21ZR0py?=
 =?utf-8?B?cVVMd3ZZYmhCb0liS0t4MUpQdGlkaU1vRTIxNlQ4cTJUUFNZOVBPU3JnOWJI?=
 =?utf-8?B?MkQ3MW0yaDE3a3JQazIvVDlKRjVYL0tQWndBYmZIVktUTExlTUl3ZDFOMWpw?=
 =?utf-8?B?N2s2NWhnWDNWdW1BM2c3NXdNbllFREJkaXNCaUtXeTJLMzJRTndkWUsxSkU1?=
 =?utf-8?B?SHVyYWExVnk2VWxMRlRESDBPc1ZhNktncHdKSWoyZGx3U0FoS1ZtU1JVdTZN?=
 =?utf-8?B?TUxmVHM4L2xRRFZRZU4vTXVsS0hVZ2FZd0JxaDlPRHBXSU9tT1I5RVVmZFNC?=
 =?utf-8?B?Y0o4QWlJTlcvb1VScDFXVmQyZWhILzhCNzEvbjJuMDdhYkpON21WSVJHdDhU?=
 =?utf-8?B?SWdwSGxJMnhwTlA5MGw0NmZCVlBZcCtJNVdXcldrWDg4aGkzVU9vTWtVdlFG?=
 =?utf-8?B?TEZpdGprS016b2YzaGR5enlNanNNdkdMRklmY2NrWjBHcTdDM29Ub3ZQUG42?=
 =?utf-8?B?dDc3SmgveW1BZ2ZYNXlUaXVzbC9qMjV3ZkMrcCt3QVBDUkdVQzNJeFhnT015?=
 =?utf-8?B?emFEeVdjYkRGRGZvNmd0NTJMMXVTeXoxM1VZY1BLQnQrRHQ1UDZHWGJUQTlw?=
 =?utf-8?B?MlRVRXh3TDA3U3hpQTVUakthQS9adkxFN3RSWi85MTBMOFZnMkExQlEvcmh0?=
 =?utf-8?B?SGRrVitJV1p3amVNT0lxZGVNcFV4R3E0WjlBYks3Zmgrd2ZGRWtVWStCd1Fz?=
 =?utf-8?B?cG12M001U2l5cTBhZld2eWxaTk1NWGZTUld4c3o1czJ4SHRncmptOVl5WU11?=
 =?utf-8?B?YzltZ09Ca2RyRlZaY1BNejdNWGFjS3FrQ0tyOUc4dUc2VkNtaFNpVEtSWUZm?=
 =?utf-8?B?ME4xVVhXWlEwQ3BrSThEakw2NmpUOVlSN3NPb1VFS3Rtajk5d1phcW5lMHRr?=
 =?utf-8?B?WnVRYWFWeGRMZ0Qrdlh1ZU9GV3Q3Umk0TU5ETnVsSE1nRXFpSFFMbDZOckRZ?=
 =?utf-8?B?cVpIa2tZdnN5L1J4VmNnNXk3Q2lZZUxDNE1zdlAzMUlIRTF4b29HZEhsVFpQ?=
 =?utf-8?B?anA2UEtRVnc0bkhjcVlFNk1HdTIzcDRYSU9EZGZQOUlLd0tLWkdpK0NBMHpM?=
 =?utf-8?B?Ti84ZUVqK0RoeGR1MkVkRkN1K3F6bjQ4aDA3RlkxM1NjR2NqanYrTUxrSnZG?=
 =?utf-8?B?bkFWaXgvdXpSeTE5Q1M5azhyMldRSmpHM1ZKM0pxQ3grcjEyNlc4ZEtRV3J1?=
 =?utf-8?B?Y0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6666db1f-ed64-4de4-043e-08dc000d74ad
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Dec 2023 21:08:17.2956
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: lUT/T4vooFfMcMmYaxNnWPgqnYwlNXYbF88e6z7/UGEPVed1ec05txNdbhaXOhhEPHTWsTLU0YmLIclQhNdLsyyNsWNCYhyIyTQL/2iAqY4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7531
X-OriginatorOrg: intel.com


On 12/17/2023 11:08 PM, Ke Xiao wrote:
> Commit 3116f59c12bd ("i40e: fix use-after-free in
> i40e_sync_filters_subtask()") avoided use-after-free issues,
> by increasing refcount during update the VSI filter list to
> the HW. However, it missed the unicast situation.
> 
> When deleting an unicast FDB entry, the i40e driver will release
> the mac_filter, and i40e_service_task will concurrently request
> firmware to add the mac_filter, which will lead to the following
> use-after-free issue.
> 
> Fix again for both netdev->uc and netdev->mc.
> 

Thanks for fixing this!

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> BUG: KASAN: use-after-free in i40e_aqc_add_filters+0x55c/0x5b0 [i40e]
> Read of size 2 at addr ffff888eb3452d60 by task kworker/8:7/6379
> 
> CPU: 8 PID: 6379 Comm: kworker/8:7 Kdump: loaded Tainted: G
> Workqueue: i40e i40e_service_task [i40e]
> Call Trace:
>  dump_stack+0x71/0xab
>  print_address_description+0x6b/0x290
>  kasan_report+0x14a/0x2b0
>  i40e_aqc_add_filters+0x55c/0x5b0 [i40e]
>  i40e_sync_vsi_filters+0x1676/0x39c0 [i40e]
>  i40e_service_task+0x1397/0x2bb0 [i40e]
>  process_one_work+0x56a/0x11f0
>  worker_thread+0x8f/0xf40
>  kthread+0x2a0/0x390
>  ret_from_fork+0x1f/0x40
> 
> Allocated by task 21948:
>  kasan_kmalloc+0xa6/0xd0
>  kmem_cache_alloc_trace+0xdb/0x1c0
>  i40e_add_filter+0x11e/0x520 [i40e]
>  i40e_addr_sync+0x37/0x60 [i40e]
>  __hw_addr_sync_dev+0x1f5/0x2f0
>  i40e_set_rx_mode+0x61/0x1e0 [i40e]
>  dev_uc_add_excl+0x137/0x190
>  i40e_ndo_fdb_add+0x161/0x260 [i40e]
>  rtnl_fdb_add+0x567/0x950
>  rtnetlink_rcv_msg+0x5db/0x880
>  netlink_rcv_skb+0x254/0x380
>  netlink_unicast+0x454/0x610
>  netlink_sendmsg+0x747/0xb00
>  sock_sendmsg+0xe2/0x120
>  __sys_sendto+0x1ae/0x290
>  __x64_sys_sendto+0xdd/0x1b0
>  do_syscall_64+0xa0/0x370
>  entry_SYSCALL_64_after_hwframe+0x65/0xca
> 
> Freed by task 21948:
>  __kasan_slab_free+0x137/0x190
>  kfree+0x8b/0x1b0
>  __i40e_del_filter+0x116/0x1e0 [i40e]
>  i40e_del_mac_filter+0x16c/0x300 [i40e]
>  i40e_addr_unsync+0x134/0x1b0 [i40e]
>  __hw_addr_sync_dev+0xff/0x2f0
>  i40e_set_rx_mode+0x61/0x1e0 [i40e]
>  dev_uc_del+0x77/0x90
>  rtnl_fdb_del+0x6a5/0x860
>  rtnetlink_rcv_msg+0x5db/0x880
>  netlink_rcv_skb+0x254/0x380
>  netlink_unicast+0x454/0x610
>  netlink_sendmsg+0x747/0xb00
>  sock_sendmsg+0xe2/0x120
>  __sys_sendto+0x1ae/0x290
>  __x64_sys_sendto+0xdd/0x1b0
>  do_syscall_64+0xa0/0x370
>  entry_SYSCALL_64_after_hwframe+0x65/0xca
> 
> Fixes: 3116f59c12bd ("i40e: fix use-after-free in i40e_sync_filters_subtask()")
> Fixes: 41c445ff0f48 ("i40e: main driver core")
> Signed-off-by: Ke Xiao <xiaoke@sangfor.com.cn>
> Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
> Cc: Di Zhu <zhudi2@huawei.com>
> Reviewed-by: Jan Sokolowski <jan.sokolowski@intel.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> ---
> v2:
> - Order local variable declarations in Reverse Christmas Tree (RCT)
> 
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 1ab8dbe2d880..d5633a440cca 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -107,12 +107,18 @@ static struct workqueue_struct *i40e_wq;
>  static void netdev_hw_addr_refcnt(struct i40e_mac_filter *f,
>  				  struct net_device *netdev, int delta)
>  {
> +	struct netdev_hw_addr_list *ha_list;
>  	struct netdev_hw_addr *ha;
>  
>  	if (!f || !netdev)
>  		return;
>  
> -	netdev_for_each_mc_addr(ha, netdev) {
> +	if (is_unicast_ether_addr(f->macaddr) || is_link_local_ether_addr(f->macaddr))
> +		ha_list = &netdev->uc;
> +	else
> +		ha_list = &netdev->mc;
> +
> +	netdev_hw_addr_list_for_each(ha, ha_list) {
>  		if (ether_addr_equal(ha->addr, f->macaddr)) {
>  			ha->refcount += delta;
>  			if (ha->refcount <= 0)